Skip to content

Synchronise Positron's Variables pane with debug environment #1052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lionel- merged 5 commits into from
Feb 25, 2026
Merged

Synchronise Positron's Variables pane with debug environment #1052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dd2f2b8
Track console environment in Variables pane
lionel- Feb 20, 2026
43c19f0
Don't update env or bindings in `List` RPC handler
lionel- Feb 21, 2026
25cd812
Remove AGENTS advice about test cleanup
lionel- Feb 21, 2026
f7caaa5
Only buffer Idle messages while waiting for comm messages
lionel- Feb 21, 2026
d15360b
Consume all Variables messages in tests
lionel- Feb 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,6 @@ If changes are needed in these files, that must happen in the separate Positron

- In tests, prefer exact assertions over fuzzy ones. Use `assert_eq!(stack[0].name, "foo()")` rather than `assert!(names.contains(&"foo()"))` when ordering and completeness matter.

- In tests, if you set global state (R options, env vars), clean it up _before_ assertions so it doesn't leak when a test fails.

- When you extract code in a function (or move things around) that function goes _below_ the calling function. A general goal is to be able to read linearly from top to bottom with the relevant context and main logic first. The code should be organised like a call stack. Of course that's not always possible, use best judgement to produce the clearest code organization.

- Keep the main logic as unnested as possible. Favour Rust's `let ... else` syntax to return early or continue a loop in the `else` clause, over `if let`.
Expand Down
136 changes: 94 additions & 42 deletions crates/ark/src/variables/r_variables.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ use libr::ENVSXP;
use stdext::result::ResultExt;
use stdext::spawn;

use crate::console;
use crate::console::Console;
use crate::data_explorer::r_data_explorer::DataObjectEnvInfo;
use crate::data_explorer::r_data_explorer::RDataExplorer;
use crate::data_explorer::summary_stats::summary_stats;
Expand Down Expand Up @@ -95,6 +97,11 @@ pub struct RVariables {
/// Whether we are currently showing the .Last.value variable in the Variables
/// pane.
showing_last_value: bool,

/// Whether we need to send an initial `Refresh` event to the frontend,
/// which is required for frontend initialization. Set on construction,
/// cleared after the first `update()` call.
needs_initial_refresh: bool,
}

impl RVariables {
Expand Down Expand Up @@ -154,6 +161,7 @@ impl RVariables {
version: 0,
show_last_value,
showing_last_value: false,
needs_initial_refresh: true,
};
environment.execution_thread();
});
Expand All @@ -162,24 +170,23 @@ impl RVariables {
pub fn execution_thread(mut self) {
let (prompt_signal_tx, prompt_signal_rx) = unbounded::<()>();

// Schedule the initial environment scan as an idle task. If R is
// already idle this runs promptly, otherwise it runs at next idle.
// This and the prompt-driven `update()` may arrive in either order,
// which is safe: the first sends a `Refresh`, the second is a no-op.
let initial_signal_tx = prompt_signal_tx.clone();
r_task::spawn_idle(move |_| async move {
initial_signal_tx.send(()).log_err();
});

// Register a handler for console prompt events
let listen_id = EVENTS.console_prompt.listen({
move |_| {
log::info!("Got console prompt signal.");
prompt_signal_tx.send(()).unwrap();
prompt_signal_tx.send(()).log_err();
}
});

// Perform the initial environment scan and deliver to the frontend
let variables = self.list_variables();
let length = variables.len() as i64;
let event = VariablesFrontendEvent::Refresh(RefreshParams {
variables,
length,
version: self.version as i64,
});
self.send_event(event);

// Flag initially set to false, but set to true if the user closes the
// channel (i.e. the frontend is closed)
let mut user_initiated_close = false;
Expand Down Expand Up @@ -249,13 +256,7 @@ impl RVariables {
fn list_variables(&mut self) -> Vec<Variable> {
let mut variables: Vec<Variable> = vec![];
r_task(|| {
Copy link
Contributor

@DavisVaughan DavisVaughan Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the List request used to request a bindings update, but we don't want to do that anymore? We are trusting that it should be update to date because it would have updated after every top level eval?

Copy link
Contributor Author

@lionel- lionel- Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

self.update_bindings(self.bindings());

// If the special .Last.value variable is enabled, add it to the
// list. This is a special R value that doesn't have its own
// binding.
if let Some(last_value) = self.last_value() {
self.showing_last_value = true;
variables.push(last_value.var());
}

Expand Down Expand Up @@ -566,10 +567,47 @@ impl RVariables {

#[tracing::instrument(level = "trace", skip_all)]
fn update(&mut self) {
let mut assigned: Vec<Variable> = vec![];
let mut removed: Vec<String> = vec![];

r_task(|| {
let needs_initial_refresh = self.needs_initial_refresh;
self.needs_initial_refresh = false;

let env_changed = self.update_env();

// Send a full refresh if the environment changed (e.g. entered
// or exited debug mode) or on the first update after startup.
if env_changed || needs_initial_refresh {
self.update_bindings(self.bindings());

let mut variables: Vec<Variable> = vec![];

if let (Some(var), _) = self.track_last_value() {
variables.push(var);
}

for binding in self.current_bindings.get() {
variables.push(PositronVariable::new(binding).var());
}

let length = variables.len() as i64;
self.send_event(VariablesFrontendEvent::Refresh(RefreshParams {
variables,
length,
version: self.version as i64,
}));

return;
}

// Incremental update: diff old vs new bindings
let mut assigned: Vec<Variable> = vec![];
let mut removed: Vec<String> = vec![];

match self.track_last_value() {
(Some(var), _) => assigned.push(var),
(None, true) => removed.push(".Last.value".to_string()),
(None, false) => {},
}

let new_bindings = self.bindings();

let mut old_iter = self.current_bindings.get().iter();
Expand All @@ -578,18 +616,6 @@ impl RVariables {
let mut new_iter = new_bindings.get().iter();
let mut new_next = new_iter.next();

// Track the last value if the user has requested it. Treat this
// value as assigned every time we update the Variables list.
if let Some(last_value) = self.last_value() {
self.showing_last_value = true;
assigned.push(last_value.var());
} else if self.showing_last_value {
// If we are no longer showing the last value, remove it from
// the list of assigned variables
self.showing_last_value = false;
removed.push(".Last.value".to_string());
}

loop {
match (old_next, new_next) {
// nothing more to do
Expand Down Expand Up @@ -644,24 +670,50 @@ impl RVariables {
}
}

// Only update the bindings (and the version) if anything changed
if assigned.len() > 0 || removed.len() > 0 {
self.update_bindings(new_bindings);
self.send_event(VariablesFrontendEvent::Update(UpdateParams {
assigned,
removed,
unevaluated: vec![],
version: self.version as i64,
}));
}
});
}

// SAFETY: The following methods must be called in an `r_task()`

if assigned.len() > 0 || removed.len() > 0 {
let event = VariablesFrontendEvent::Update(UpdateParams {
assigned,
removed,
unevaluated: vec![],
version: self.version as i64,
});
self.send_event(event);
/// Updates `self.showing_last_value` and returns the last value variable
/// if it should be shown, along with whether it was previously shown.
fn track_last_value(&mut self) -> (Option<Variable>, bool) {
let was_showing = self.showing_last_value;

if let Some(last_value) = self.last_value() {
self.showing_last_value = true;
(Some(last_value.var()), was_showing)
} else {
self.showing_last_value = false;
(None, was_showing)
}
}

// SAFETY: The following methods must be called in an `r_task()`
/// Updates `self.env` to the current effective environment if it changed
/// (e.g. entering or exiting debug mode). Returns `true` if switched.
fn update_env(&mut self) -> bool {
if !Console::is_initialized() {
return false;
}

let env = console::selected_env();

if env.sexp == self.env.get().sexp {
return false;
}

self.env = RThreadSafe::new(env);
true
}

fn bindings(&self) -> RThreadSafe<Vec<Binding>> {
let env = self.env.get().clone();
Expand Down
16 changes: 16 additions & 0 deletions crates/ark/tests/variables.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,22 @@ fn test_variables_list() {
Rf_defineVar(sym, Rf_ScalarInteger(42), *test_env);
Copy link
Contributor

@DavisVaughan DavisVaughan Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was confused for a minute about why . prefixed names werent showing up

I wonder if the Variables Pane should instead list all . prefixed names except for .Random.seed, which it should always filter out, rather than not listing any . prefixed names.

Image

});

// Simulate a prompt signal so `update()` picks up the new variable
EVENTS.console_prompt.emit(());
let msg = iopub_rx.recv_comm_msg();
let data = match msg {
CommMsg::Data(data) => data,
_ => panic!("Expected data message"),
};
let evt: VariablesFrontendEvent = serde_json::from_value(data).unwrap();
match evt {
VariablesFrontendEvent::Update(params) => {
assert_eq!(params.assigned.len(), 1);
assert_eq!(params.assigned[0].display_name, "everything");
},
_ => panic!("Expected update event"),
}

// Request a list of variables
let request = VariablesBackendRequest::List;
let data = serde_json::to_value(request).unwrap();
Expand Down
135 changes: 135 additions & 0 deletions crates/ark/tests/variables_debug.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
//
// variables_debug.rs
//
// Copyright (C) 2026 Posit Software, PBC. All rights reserved.
//
//

use amalthea::fixtures::dummy_frontend::ExecuteRequestOptions;
use ark_test::DummyArkFrontend;

/// When entering debug mode (browser), the variables pane should switch to
/// showing the debug environment's bindings. When exiting, it should switch
/// back to the global environment.
#[test]
fn test_variables_pane_shows_debug_env() {
let frontend = DummyArkFrontend::lock();

// Set up a global variable before opening the variables comm
frontend.execute_request_invisibly("test_gv <- 'hello'");

// Open the variables comm and receive the initial Refresh
let initial = frontend.open_variables_comm();
let names: Vec<&str> = initial
.variables
.iter()
.map(|v| v.display_name.as_str())
.collect();
assert!(names.contains(&"test_gv"));

// Enter browser with a local variable
frontend.send_execute_request(
"local({ debug_var <- 42; browser() })",
ExecuteRequestOptions::default(),
);
frontend.recv_iopub_busy();
frontend.recv_iopub_execute_input();
frontend.assert_stream_stdout_contains("Called from:");
frontend.recv_iopub_idle();
frontend.recv_shell_execute_reply();

// The variables pane should have sent a Refresh with the debug env
let refresh = frontend.recv_variables_refresh();
let names: Vec<&str> = refresh
.variables
.iter()
.map(|v| v.display_name.as_str())
.collect();
assert_eq!(names, vec!["debug_var"]);

// Quit the debugger
frontend.send_execute_request("Q", ExecuteRequestOptions::default());
frontend.recv_iopub_busy();
frontend.recv_iopub_execute_input();
frontend.recv_iopub_idle();
frontend.recv_shell_execute_reply();

// Should be back to showing the global environment
let refresh = frontend.recv_variables_refresh();
let names: Vec<&str> = refresh
.variables
.iter()
.map(|v| v.display_name.as_str())
.collect();
assert!(names.contains(&"test_gv"));
assert!(!names.contains(&"debug_var"));

// Clean up
frontend.send_execute_request("rm(test_gv)", ExecuteRequestOptions::default());
frontend.recv_iopub_busy();
frontend.recv_iopub_execute_input();
frontend.recv_iopub_idle();
frontend.recv_shell_execute_reply();

// Consume the Update triggered by the rm()
let _update = frontend.recv_variables_update();
}

/// When entering debug via a function call, the variables pane should show
/// the function's environment with its arguments.
#[test]
fn test_variables_pane_shows_function_debug_env() {
let frontend = DummyArkFrontend::lock();

// Set up a global variable before opening the variables comm
frontend.execute_request_invisibly("test_gv2 <- 'world'");

let _initial = frontend.open_variables_comm();

// Define a function and call it so browser() triggers inside
frontend.send_execute_request(
"f <- function(my_arg) { browser(); my_arg }; f(99)",
ExecuteRequestOptions::default(),
);
frontend.recv_iopub_busy();
frontend.recv_iopub_execute_input();
frontend.assert_stream_stdout_contains("Called from: f(99)");
frontend.recv_iopub_idle();
frontend.recv_shell_execute_reply();

// The debug env should show `my_arg`
let refresh = frontend.recv_variables_refresh();
let names: Vec<&str> = refresh
.variables
.iter()
.map(|v| v.display_name.as_str())
.collect();
assert_eq!(names, vec!["my_arg"]);

// Quit the debugger
frontend.send_execute_request("Q", ExecuteRequestOptions::default());
frontend.recv_iopub_busy();
frontend.recv_iopub_execute_input();
frontend.recv_iopub_idle();
frontend.recv_shell_execute_reply();

// Should be back to showing the global environment
let refresh = frontend.recv_variables_refresh();
let names: Vec<&str> = refresh
.variables
.iter()
.map(|v| v.display_name.as_str())
.collect();
assert!(names.contains(&"test_gv2"));
assert!(!names.contains(&"my_arg"));

// Clean up
frontend.send_execute_request("rm(test_gv2, f)", ExecuteRequestOptions::default());
frontend.recv_iopub_busy();
frontend.recv_iopub_execute_input();
frontend.recv_iopub_idle();
frontend.recv_shell_execute_reply();

// Consume the Update triggered by the rm()
let _update = frontend.recv_variables_update();
}
Loading