Add plot file/origin attribution by jmcphers · Pull Request #1067 · posit-dev/ark

jmcphers · 2026-02-27T17:00:39Z

Companion to posit-dev/positron#12168; adds additional metadata to plots annotating where they originated, to make it easy to navigate to that location. See that PR for more info.

For linewise execution, this borrows from the source location infrastructure @lionel- set up already for debugging; for execution via source(), some slightly more involved work is needed to store the source context so we can get at it later.

DavisVaughan

I'd like to get @lionel-'s opinion on a few questions about where things should live, but otherwise seems good

DavisVaughan · 2026-03-02T16:03:06Z

crates/ark/src/modules/positron/hooks_source.R

+        # is not yet available (e.g. during development with mismatched builds).
+        if (!is.null(source_uri)) {
+            pushed <- tryCatch(
+                { .ps.Call("ps_graphics_push_source_context", source_uri); TRUE },


Can you please reformat with Air? I should probably turn that on as a CI check...

Yeah, it looks like a lot of the .R files are not air-compliant. I went ahead and formatted the whole workspace (LMK if you'd like to keep the changes scoped to just this new file)

DavisVaughan · 2026-03-02T16:56:18Z

crates/ark/src/modules/positron/hooks_source.R

+                error = function(e) FALSE
+            )
+            if (pushed) {
+                on.exit(tryCatch(.ps.Call("ps_graphics_pop_source_context"), error = function(e) NULL), add = TRUE)


We are trying to use defer() instead of on.exit(), which comes with the right defaults

DavisVaughan · 2026-03-02T16:58:17Z

crates/ark/src/modules/positron/hooks_source.R


+        # Try to resolve the file URI early so we can attribute plots to this
+        # source file. This is best-effort; if it fails we proceed without attribution.
+        source_uri <- tryCatch(path_to_file_uri(file), error = function(e) NULL)


I see no reason to not just consistently call it uri everywhere rather than renaming it later on

DavisVaughan · 2026-03-02T17:01:01Z

crates/ark/src/console.rs

-                // Push execution context to graphics device for plot attribution
+                // Push execution context to graphics device for plot attribution.
+                // Extract code_location from the execute request for source file origin.
+                let code_location = exec_req.code_location().log_err().flatten().map(|mut loc| {


@lionel- should code_location() just normalize() the URI internally? It seems like the one other place we call code_location() also normalizes after the fact.

That's a good idea!

code_location() lives in Amalthea and ExtUrl::normalize() is in Ark, so a nice place to implement it would be on the ExtUrl extension trait:

let code_location = ExtUrl::from_execute_request(&exec_req);

Actually I realised while working on #1082 that we should not normalise/canonicalise URIs that flow back to the frontend, because it might think it's a different file. I noticed Positron would open a canonicalised path in a new editor instead of reusing an existing one.

Fixed in the linked PR.

DavisVaughan · 2026-03-02T17:05:10Z

crates/ark/src/plots/graphics_device.rs

+/// Push a source file URI onto the source context stack.
+/// Called from the `source()` hook when entering a sourced file.
+#[harp::register]
+unsafe extern "C-unwind" fn ps_graphics_push_source_context(uri: SEXP) -> anyhow::Result<SEXP> {
+    let uri_str: String = RObject::view(uri).try_into()?;
+    DEVICE_CONTEXT.with_borrow(|cell| cell.push_source_context(uri_str));
+    Ok(harp::r_null())
+}
+
+/// Pop a source file URI from the source context stack.
+/// Called from the `source()` hook when leaving a sourced file.
+#[harp::register]
+unsafe extern "C-unwind" fn ps_graphics_pop_source_context() -> anyhow::Result<SEXP> {
+    DEVICE_CONTEXT.with_borrow(|cell| cell.pop_source_context());
+    Ok(harp::r_null())
+}


IIUC the desire is to be able to query the name of the file that is currently being source()-ed at any time while inside the execution of that file.

That doesn't feel super specific to plots, so I would not be against that being a generic feature that lives in Console instead.

@lionel- what do you think?

That makes sense to me, although I can't think of a reason to do it proactively.

I was initially thinking it may help with transitions of comms to Console thread, as the stack could be made available via the comm context. But the graphics device is not a comm so I don't think that will help there.

Feel free to add the source context to the Console though Jonathan.

In that case it might make sense to include the execute request's location in the stack so that priority resolution is consistently handled.

Thanks, agree that could be helpful but won't do it proactively here!

DavisVaughan · 2026-03-02T17:11:34Z

crates/ark/src/plots/graphics_device.rs

    fn new_positron_page(&self) {
        self.is_new_page.replace(true);
        self.id.replace(Self::new_id());
+        *self.pending_origin.borrow_mut() = None;


Use self.pending_origin.replace(None) like we've done for the other fields?

Here and elsewhere.

crates/ark/src/plots/graphics_device.rs

DavisVaughan · 2026-03-02T17:22:19Z

crates/ark/src/plots/graphics_device.rs

+        // Clear any unconsumed pending origin so it doesn't leak into the
+        // next execute request. `process_changes()` above already consumed it
+        // if this execution produced a new plot; this handles the case where
+        // drawing occurred (e.g. `lines()` inside `source()`) but only as an
+        // update to an existing plot, leaving the pending origin unclaimed.
+        *cell.pending_origin.borrow_mut() = None;


Maybe we want set_pending_origin() and clear_pending_origin() to go along with set_execution_context() and clear_execution_context()? That probably cleans this up a little bit.

DavisVaughan · 2026-03-02T17:23:20Z

crates/ark/src/modules/positron/hooks_source.R

            args$catch.aborts <- NULL
        }

+        # Try to resolve the file URI early so we can attribute plots to this


Since @lionel- is the author of this bit I think he should sign off on the changes in this file

…ribution

Co-authored-by: Davis Vaughan <davis@rstudio.com>

lionel-

That will be a nice feature!

The design is sound and the implementation looks nice.

lionel- · 2026-03-03T13:13:25Z

crates/ark/src/modules/positron/hooks_source.R

+            if (pushed) {
+                defer(tryCatch(
+                    .ps.Call("ps_graphics_pop_source_context"),
+                    error = function(e) NULL
+                ))
+            }


nit: This could equivalently be run unconditionally in the try branch, after the .Call returns.

yep c1f9b56

lionel- · 2026-03-03T13:27:04Z

crates/ark/src/plots/graphics_device.rs

+    /// Checks three sources in priority order:
+    /// 1. Source context stack (inside `source("file.R")`) -- file-level, no range
+    /// 2. Execute request's `code_location` (code run from a file via the IDE) -- with range
+    /// 3. None (code typed at the console)


lionel- · 2026-03-03T13:31:24Z

crates/ark/src/plots/graphics_device.rs

+/// Push a source file URI onto the source context stack.
+/// Called from the `source()` hook when entering a sourced file.
+#[harp::register]
+unsafe extern "C-unwind" fn ps_graphics_push_source_context(uri: SEXP) -> anyhow::Result<SEXP> {
+    let uri_str: String = RObject::view(uri).try_into()?;
+    DEVICE_CONTEXT.with_borrow(|cell| cell.push_source_context(uri_str));
+    Ok(harp::r_null())
+}
+
+/// Pop a source file URI from the source context stack.
+/// Called from the `source()` hook when leaving a sourced file.
+#[harp::register]
+unsafe extern "C-unwind" fn ps_graphics_pop_source_context() -> anyhow::Result<SEXP> {
+    DEVICE_CONTEXT.with_borrow(|cell| cell.pop_source_context());
+    Ok(harp::r_null())
+}


That makes sense to me, although I can't think of a reason to do it proactively.

I was initially thinking it may help with transitions of comms to Console thread, as the stack could be made available via the comm context. But the graphics device is not a comm so I don't think that will help there.

Feel free to add the source context to the Console though Jonathan.

In that case it might make sense to include the execute request's location in the stack so that priority resolution is consistently handled.

lionel- · 2026-03-03T13:35:19Z

crates/amalthea/src/comm/plot_comm.rs


+/// The origin (source) of a plot
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
+pub struct PlotOrigin {


Since the structure is very similar to CodeLocation, I think I would have expected PlotLocation. I guess Origin has a slightly different nuance though, and indicates the original location might have changed.

Yes, it's hard to name this correctly! I think PlotLocation might be misread as "where the plot itself is located" vs "where the plot came from"

lionel- · 2026-03-03T13:40:36Z

crates/ark/src/console.rs

-                // Push execution context to graphics device for plot attribution
+                // Push execution context to graphics device for plot attribution.
+                // Extract code_location from the execute request for source file origin.
+                let code_location = exec_req.code_location().log_err().flatten().map(|mut loc| {


That's a good idea!

code_location() lives in Amalthea and ExtUrl::normalize() is in Ark, so a nice place to implement it would be on the ExtUrl extension trait:

let code_location = ExtUrl::from_execute_request(&exec_req);

lionel- · 2026-03-03T13:42:06Z

crates/ark/tests/plots.rs

        "Metadata should contain kind 'plot', got:\n{result}"
    );
 }
+


Nice to see the new test infra put to good use!

I think it'd be worth testing the use case of sourcing a file A that sources a file B, to exercise the stacked aspect of locations.

good idea! done in 08d50b8

jmcphers added 2 commits February 26, 2026 10:11

initial impl

b137227

add ui comm to dummy frontend

0b54d84

jmcphers requested a review from DavisVaughan February 27, 2026 17:00

jmcphers mentioned this pull request Feb 27, 2026

Plots: Link to plot origin from plots pane posit-dev/positron#12168

Merged

lionel- mentioned this pull request Mar 2, 2026

Migrate existing comm tests to protocol-level tests #1074

Open

DavisVaughan approved these changes Mar 2, 2026

View reviewed changes

DavisVaughan requested a review from lionel- March 2, 2026 17:24

jmcphers and others added 5 commits March 2, 2026 11:02

Merge remote-tracking branch 'origin/main' into feature/plot-file-att…

24484e2

…ribution

update formatting

9a4edbd

code review feedback

865df39

code review feedpack part 2

d49c83f

Update crates/ark/src/plots/graphics_device.rs

0480cf2

Co-authored-by: Davis Vaughan <davis@rstudio.com>

lionel- approved these changes Mar 3, 2026

View reviewed changes

jmcphers added 3 commits March 3, 2026 08:38

test nested source contexts

08d50b8

simplify source context push

c1f9b56

explicit drop context

d51263a

jmcphers merged commit 778da3e into main Mar 3, 2026
8 checks passed

jmcphers deleted the feature/plot-file-attribution branch March 3, 2026 20:27

github-actions bot locked and limited conversation to collaborators Mar 3, 2026

Conversation

jmcphers commented Feb 27, 2026

Uh oh!

DavisVaughan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lionel- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants