Repository navigation
Show R session info in data and list viewer tooltips - #1848
Conversation
eitsupi
left a comment
There was a problem hiding this comment.
Review by ChatGPT.
I reviewed this with particular attention to the design problems and review findings from #1828. I think this is a substantially better design.
The important improvement is that the two concepts are now kept separate: the status bar continues to represent the active R session, while each Data/List Viewer exposes the session that originally produced that viewer. Because viewer focus no longer participates in status-bar/session selection, the races found in #1828 around workspace refreshes, viewer blur, terminal focus/selection, Interactive activation, and delayed terminal PID resolution are avoided structurally rather than being handled by an increasingly complex focus/state machine.
I also checked the new viewer-session lifecycle handling and tests. The source version/PID is captured independently of later session selection, disconnect alone does not incorrectly imply process exit, confirmed Interactive exits are propagated, and the viewer update does not alter activeSession or the existing status-bar behavior. The shared viewerSession implementation for Data Viewer and List Viewer also looks appropriately scoped.
One minor design concern is that attachViewerSessionBridge() creates a per-viewer timer and uses process.kill(pid, 0) for local process liveness. An event-driven/shared lifecycle mechanism would be cleaner in the longer term and would avoid PID-reuse edge cases. However, given the requirement to distinguish transport detachment from actual R process exit, I don't consider this a blocker for this PR.
Overall, I found no actionable correctness issue in the current implementation. This addresses the original goal with considerably less coupling than #1828, and I think this approach is preferable.
|
Since @renkun-ken reviewed #1828, I'd like to wait for @renkun-ken's opinion. |
If this is accepted, I am thinking where to put that info icon on Plot Viewer and HTML Widget Viewer if they are made session-aware later. |
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed 855b274. I found no actionable correctness issues and support this approach. Keeping the source session in each viewer's tooltip avoids the focus and status-bar reconciliation problems from #1828 while leaving active-session selection unchanged.
The shared process monitor also addresses the earlier per-viewer polling concern: viewers of the same source share one probe, reconnects retain the process identity, transport detachment does not imply exit, and the last viewer releases its monitoring. Confirmed Interactive exits update the tooltip directly, including when the host cannot be probed locally.
Validation: local extension bundling, TypeScript checking, and TypeScript lint passed. All 27 focused viewer tests and 7 process-monitor tests passed, as did the changed Interactive nested-table integration test with two live R sessions. Two additional checks using real VS Code 1.140.0 List Viewer and Data Viewer panels confirmed that terminating their live R source while they are hidden updates the actual tooltip to R: (not attached) when revealed, without changing the active session. CI build, lint, and macOS/Linux/Windows tests are green for this revision.
The full Node suite was not validated locally: sandbox socket restrictions and a missing renv installation prevented several unrelated runtime tests from running successfully.
Replaces #1828 .
Closes #1827
Summary
PR #1828 aims to identify the source R session of each Data Viewer and List Viewer when multiple sessions are running. However, using one shared status bar for both the focused viewer’s source session and the active R session introduces complex focus and state handling.
This PR replaces #1828 by displaying source session information in a tooltip within each viewer, leaving the status bar’s active-session behavior unchanged.
Adds an info icon to Data Viewer and List Viewer showing the source session’s R version and PID, using the status bar format:
R version: PID.Validation: build, TypeScript checks, lint, and 26 focused viewer tests passed. Hover and keyboard dismissal were also verified in a browser.