From 35c960ffab24a6b603bd8a302c6c3bfc18ef38fc Mon Sep 17 00:00:00 2001 From: Marie Idleman Date: Wed, 5 Aug 2026 18:50:03 -0500 Subject: [PATCH 1/2] Keep the plot origin when a render is held past source() A plot created inside `source()` gets its origin from the source context stack, which `source()`'s `defer()` pops on the way out. The origin is snapshotted eagerly at drawing time to survive that, but two things could drop the snapshot before `process_changes()` consumed it: - `hook_mode()` only captured on the `has_changes` false->true edge, so a new page starting while earlier changes were still pending (e.g. under `dev.hold()`) never re-captured after `new_positron_page()` cleared it. - `graphics_on_did_execute_request()` cleared the snapshot at the end of every request, so a render held into a later request lost it. Capture whenever no snapshot is pending, and drop the end-of-request clear, which is redundant now that the new-page clear is the only one that has to hold. Without an origin the Plots pane silently shows no source file, with no way to recover it. Fixes #1334 --- crates/ark/src/console/console_graphics.rs | 1 - crates/ark/src/plots/graphics_device.rs | 13 +++--- crates/ark/tests/integration/plots.rs | 46 ++++++++++++++++++++++ 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/crates/ark/src/console/console_graphics.rs b/crates/ark/src/console/console_graphics.rs index 21e364cdea..6fba69937b 100644 --- a/crates/ark/src/console/console_graphics.rs +++ b/crates/ark/src/console/console_graphics.rs @@ -39,6 +39,5 @@ impl Console { let dc = Rc::clone(self.device_context()); dc.process_changes(self); dc.clear_execution_context(); - dc.clear_pending_origin(); } } diff --git a/crates/ark/src/plots/graphics_device.rs b/crates/ark/src/plots/graphics_device.rs index 56b33ec755..c198c962d4 100644 --- a/crates/ark/src/plots/graphics_device.rs +++ b/crates/ark/src/plots/graphics_device.rs @@ -277,7 +277,7 @@ impl DeviceContext { } /// Clear any unconsumed pending origin. - pub(crate) fn clear_pending_origin(&self) { + fn clear_pending_origin(&self) { self.pending_origin.replace(None); } @@ -338,10 +338,13 @@ impl DeviceContext { let old_has_changes = self.has_changes.get(); self.has_changes.replace(old_has_changes || is_drawing); - // Eagerly capture the plot origin when drawing first starts for this - // change set. The source context stack may be popped before - // `process_changes()` runs, so we snapshot it now while it's available. - if !old_has_changes && is_drawing { + // Eagerly capture the plot origin while the source context stack is still + // available, since `process_changes()` may not run until after `source()` + // popped it. Capture whenever we have no snapshot rather than only on the + // `has_changes` false->true edge, which never comes back around for a new + // page started while earlier changes are still pending (e.g. `dev.hold()`). + let needs_origin = self.pending_origin.borrow().is_none(); + if is_drawing && needs_origin { let ctx = self.capture_execution_context(); let origin = self.capture_plot_origin(&ctx); self.set_pending_origin(origin); diff --git a/crates/ark/tests/integration/plots.rs b/crates/ark/tests/integration/plots.rs index 8feb383672..f30ca73b0b 100644 --- a/crates/ark/tests/integration/plots.rs +++ b/crates/ark/tests/integration/plots.rs @@ -585,6 +585,52 @@ fn test_plot_source_context_stacking() { assert!(result_a.contains(&format!("$origin_uri\n[1] \"{}\"", file_a.uri_id))); } +/// Test that a plot created inside `source()` keeps its origin when rendering +/// is held past the end of the `source()` call. +/// +/// https://github.com/posit-dev/ark/issues/1334 +/// +/// While a hold is active, `process_changes()` deliberately leaves +/// `has_changes` set so it can notify once the hold is released. That leaves +/// `has_changes` already true when the next plot starts drawing, which skips +/// the eager origin capture in `hook_mode()`. By the time the hold is released, +/// `source()` has returned and its `defer()` has popped the source context, so +/// the origin has nothing left to fall back to. +#[test] +fn test_plot_origin_survives_hold_across_source() { + let frontend = DummyArkFrontend::lock(); + + // Two plots under a hold, with no `dev.flush()`, so the notification is + // deferred past the end of `source()`. + let file = SourceFile::new("invisible(dev.hold())\nplot(1:5)\nplot(1:3)\n"); + + let code = format!("source('{}')", file.path); + frontend.send_execute_request(&code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + // Release the hold in a separate request, after source() has returned. + frontend.send_execute_request("invisible(dev.flush())", ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + let display_id = frontend.recv_iopub_display_data_id(); + assert!(!display_id.is_empty()); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + let query = format!(".ps.graphics.get_metadata('{display_id}')"); + frontend.send_execute_request(&query, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + let result = frontend.recv_iopub_execute_result(); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + assert!(result.contains(&format!("$origin_uri\n[1] \"{}\"", file.uri_id))); +} + /// Test that plots rendered with fig-width/fig-height metadata produce /// a PNG at the expected pixel dimensions (inches * 96 DPI). #[test] From d7177a7d7a0a79e9458b670284654aabcccefdcc Mon Sep 17 00:00:00 2001 From: Marie Idleman Date: Wed, 5 Aug 2026 19:03:40 -0500 Subject: [PATCH 2/2] Update pending_origin docs for the new capture condition The field and setter docs still described capturing on the has_changes false->true edge, which the previous commit replaced. --- crates/ark/src/plots/graphics_device.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/crates/ark/src/plots/graphics_device.rs b/crates/ark/src/plots/graphics_device.rs index c198c962d4..4fe45ca3e4 100644 --- a/crates/ark/src/plots/graphics_device.rs +++ b/crates/ark/src/plots/graphics_device.rs @@ -187,10 +187,10 @@ pub(crate) struct DeviceContext { /// provides the file attribution even though the execute_request came from the console. source_context_stack: RefCell>, - /// The plot origin captured eagerly when drawing starts (i.e. when `has_changes` - /// transitions from false to true). This is necessary because the source context - /// stack may be popped before `process_changes()` runs (e.g. `source()` completes - /// before the execute request finishes), so we snapshot the origin at drawing time. + /// The plot origin for the current page, captured eagerly whenever drawing + /// starts and no snapshot is pending. This is necessary because the source + /// context stack may be popped before `process_changes()` runs (e.g. `source()` + /// completes before the execute request finishes). pending_origin: RefCell>>, } @@ -269,9 +269,8 @@ impl DeviceContext { self.source_context_stack.borrow().last().cloned() } - /// Eagerly capture the plot origin so it's available when `process_changes()` runs later. - /// Called when drawing first starts for a change set, since the source context stack - /// may be popped before we get a chance to consume it. + /// Eagerly capture the plot origin so it's available when `process_changes()` + /// runs later, since the source context stack may be popped before then. fn set_pending_origin(&self, origin: Option) { self.pending_origin.replace(Some(origin)); }