Fix blank Data Explorer when viewing objects piped with %>%

[juliasilge]

• Closes Magrittr pipe dataframe to View() does not work positron#7385
• Closes data explorer not rendering any data with magrittr pipe and renv positron#8167

This PR fixes a blank Data Explorer when viewing a data frame through the magrittr pipe, e.g. ggplot2::diamonds %>% View(). Currently the tab opens, shows a loading spinner, and then goes blank because the kernel closes the comm before any data is sent. We originally thought this was something to do with renv, but that seems like it was incorrect. The base pipe (ggplot2::diamonds |> View()) was always unaffected.

Why did this happen?

View() registers the Data Explorer against a variable binding (a name plus an environment) so the grid can live-update when the underlying object changes. On each top-level execution, RDataExplorer::update() re-resolves that binding with Rf_findVarInFrame() and compares the result against the object it is currently showing.

The magrittr pipe binds the piped object to . inside its private pipe environment, and it stores . as a promise rather than a plain value. So the re-resolution returned the promise object (a PROMSXP), not the data frame. That promise pointer never equals the forced data frame we are viewing, so update() treated it as a replacement value, failed to compute a table shape for it, and closed the comm. The base pipe works because diamonds |> View() is pure syntax that rewrites to View(diamonds), and diamonds in the global environment is a plain value.

What changes in this PR

When re-resolving the binding, force the value if it is a promise before comparing and computing its shape. For the magrittr case the promise is already forced, so this is a cached read with no side effects, and the resolved value now equals the data frame we are viewing, so the comm stays open. The change preserves existing behavior for genuine top-level variables: removing a watched variable (rm(x)) still resolves to an unbound value and correctly closes the comm.

This seems like a pretty innocuous fix to me, but if you all think we should not take it just for magrittr pipe users, I will not be super offended.

Positron Release Notes

New Features

• N/A

Bug Fixes

• Fixed the Data Explorer being blank when viewing objects piped with %>% (Magrittr pipe dataframe to View() does not work positron#7385 )

[lionel-]

I think this should work out in principle, but the fix and testing approach need a different shape.

[lionel-]

Should at least use r_promise_force_with_rollback() to avoid "restarting interrupted promise evaluation" warnings when the promise fails.

But a comm is not the right place to be forcing a promise as this can have all sorts of side effects. The promise might force other promises which won't be rolled back, might cause warnings, might emit output, change bindings in arbitrary environments, modify options, etc. Note that completions only force lazyload promises, and leaves all other promises intact. These lazyload promises have well known behaviour.

Looking at the use case (piping into View()), it seems fine to force the magrittr input, it's just that this should not happen here.

[lionel-]

IIUC, the promise should already be forced by View() right? Then I'd prefer to do something like:

if prom_is_forced(x)
then prom_force(x)

[lionel-]

Would be nice to test for the edge case of an object watched by a data explorer going from forced to delayed, and check that the promise is not forced by the explorer even though it could watch forced values just before.

[juliasilge]

Thank you, I understand! I updated so the re-resolution now reads the value of an already-forced promise via r_promise_value() instead of forcing. If the binding holds an unforced promise, we return Ok(true) early and leave the current view in place rather than forcing it. That also means no r_promise_force_with_rollback() is needed, since we never force from here.

[juliasilge]

I dropped the legacy helper and test and moved the tests to the protocol-level backend using the dummy frontend:

• test_magrittr_pipe_data_explorer_stays_open: drives the real df %>% View(), triggers an environment change, and asserts the comm stays open and still serves the data frame.
• test_data_explorer_does_not_force_delayed_binding: I added your suggested edge case. The watched binding transitions from a plain value to an unforced delayedAssign promise that flips a flag when evaluated. We assert the flag stays FALSE (never forced) and the explorer keeps showing the last resolved value.

I confirmed both tests fail with the exact comm_close on the unfixed code.

[lionel-]

Perfect!

[lionel-]

Here are the release notes template from #1318 in case you'd like to try posit-dev/positron#14676 for bumping Ark:

### Positron Release Notes

<!--
  These notes are typically filled by the Positron team. If you are an external
  contributor, you may leave the `N/A` bullets in place.
-->

#### New Features

- N/A

#### Bug Fixes

- N/A

[juliasilge]

I updated the PR body with the release notes! ✅