Skip to content

Feature/update status bar PID for session-aware data/list viewers from different sessions - #1828

Closed
Fred-Wu wants to merge 23 commits into
REditorSupport:mainfrom
Fred-Wu:feature/viewer-session-pid
Closed

Fred-Wu wants to merge 23 commits into
REditorSupport:mainfrom
Fred-Wu:feature/viewer-session-pid

Conversation

@Fred-Wu

@Fred-Wu Fred-Wu commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1827

Summary

This PR makes status bar PID updated for session-aware Data Viewer and List Viewer when switching between viewers.

It does not activate or switch the underlying R session; it only updates the displayed session information based on the viewer’s existing session ownership.

CI coverage was added to verify the PID changes correctly while activeSession remains unchanged.

This also paves the way for switching HTML widgets and the plot viewers between R sessions once remaining session-aware viewers are implemented.

@Fred-Wu
Fred-Wu marked this pull request as ready for review October 5, 2026 07:32

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two reproducible gaps remain in the displayed PID's lifecycle: workspace refreshes overwrite the focused viewer's PID, and leaving a viewer does not restore the active session's PID. Both can be addressed while preserving this PR's intended behavior of leaving activeSession unchanged.

Validation: the 12 existing viewer ownership/panel tests pass in VS Code 1.140.0. Two additional focused regression checks fail as described in the inline comments, including a real WebviewPanel-to-R-editor focus transition. CI build, lint, and all three platform test jobs are green. Local bundling succeeded; the full local typecheck reports unrelated errors at src/helpViewer/packages.ts:402–403 with the workspace's existing dependencies.

Comment thread src/session.ts Outdated
sessionStatusBarItem.tooltip = `${info.version || rVer}\nProcess ID: ${pid}\nCommand: ${info.command}\nStart time: ${info.start_time}\nClick to attach to active terminal.`;
sessionStatusBarItem.show();
}
updateSessionStatusBar(session);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Preserve the focused viewer's PID during workspace refreshes

With session B active, focus a viewer owned by A and then call updateWorkspace(): the status initially shows A, but updateSessionWorkspace(B, ...) calls refreshActiveSession(B), and this unconditional update changes the displayed PID back to B while A's viewer remains focused. The same path runs for scheduled workspace_updated refreshes and Interactive workspace events, so the new display can become incorrect without any focus change. I reproduced this with the existing session-ownership harness (aList.activate(); await session.updateWorkspace()), with activeSession correctly remaining B. Please retain/resolve the focused viewer's owner when refreshing the status bar, and add coverage for a workspace refresh after viewer focus.

Comment thread src/session.ts
Comment on lines +293 to +295
panel.onDidChangeViewState(({ webviewPanel }) => {
if (!webviewPanel.active || !sessionId) {
return;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Restore the active session's PID when leaving a viewer

This listener writes A's PID when A's viewer gains focus, but returns immediately when it loses focus. With B still the active session, switching from A's viewer back to an ordinary, unbound .R editor therefore leaves A's PID displayed even though session-based completions and workspace actions still use B. The regular text-editor handler only tracks the editor; it does not restore this status item. I reproduced the transition using a real VS Code WebviewPanel followed by showTextDocument(): the panel becomes inactive, activeSession remains B, and the status still shows A. Please restore the appropriate session display when no session-owned viewer is active, and cover the viewer-to-editor transition as well as switching between viewers.

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The two findings from my previous review are addressed: workspace refresh preserves the viewer PID, and returning to a regular R editor restores the active PID. Local bundling/typecheck and all 13 updated viewer ownership/panel tests pass in VS Code 1.140.0.

One remaining regression is described inline: focusing/selecting an attached R terminal while a viewer is still the active editor leaves the viewer's PID displayed, even after switchSessionByTerminal() selects the terminal's session. I reproduced this with a real WebviewPanel and terminal, an IPC attach handshake, and the terminal switching handler.

Windows CI also needs attention: the current Windows job has 378 passing tests and one failure in the new restores the active session PID when leaving a real viewer for an R editor test. It times out at out/test/suite/listViewerPanels.test.js:137, which maps to the initial await waitFor(() => statusBar.text === 'R 4.6.0: 111') assertion, before the editor transition. Build and lint passed; macOS/Linux CI were still running when checked.

Comment thread src/session.ts Outdated
sessionStatusBarItem.tooltip = `${info.version || rVer}\nProcess ID: ${pid}\nCommand: ${info.command}\nStart time: ${info.start_time}\nClick to attach to active terminal.`;
sessionStatusBarItem.show();
}
updateSessionStatusBar(sessions.get(focusedDataViewSessionId ?? '') ?? session);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Allow terminal selection to override the viewer PID

WebviewPanel.active stays true when keyboard focus moves from the editor to the bottom terminal panel, so the view-state listener does not clear focusedDataViewSessionId in that transition. Consequently, with viewer A open, selecting attached terminal B calls switchSessionByTerminal(B) and activates B, but this expression still writes A's PID. I reproduced this in VS Code 1.140.0 with a real panel and terminal: after terminal.show(false), workbench.action.terminal.focus, and switchSessionByTerminal(terminal), activeSession is B while the status still shows A and panel.active remains true. This regresses the existing PID update when switching R terminals. Please distinguish workspace-only refreshes from explicit terminal/session selection so the latter can display B, and add coverage for a viewer-to-terminal transition without opening another editor.

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The previous findings are addressed. Workspace refreshes preserve the displayed viewer PID, leaving a viewer restores the active session PID, and terminal selection correctly overrides the viewer display. The latest focus/blur messages also handle returning to an already selected terminal, where VS Code emits no new terminal-selection event. Session cleanup reconciles the display with the remaining viewer/active session. I found no remaining actionable issues in the latest changes.

Validation: local bundling and TypeScript checks pass, as do all 15 focused viewer ownership/panel tests. Two additional checks using real VS Code 1.140.0 panels and an attached terminal passed for both List Viewer and Data Viewer: the actual webview focus/blur messages restore the terminal PID while activeSession remains unchanged. CI build, lint, and macOS/Linux/Windows test jobs are all green for this revision.

@Fred-Wu

Fred-Wu commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

A few new issues identified, and fixing them now.

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed the changes since 80a04e8. The guards for inactive viewer focus messages, detached viewer status, and closing the last terminal have focused coverage, and local bundling/typecheck plus all 17 existing viewer ownership/panel tests pass. CI build, lint, and all three platform test jobs are green.

One regression remains in the deferred blur fallback, described inline: selecting an unattached terminal correctly displays R: (not attached), but the pending blur callback restores the previous R session's PID. I reproduced it with the session-ownership harness and with real VS Code 1.140.0 List Viewer and Data Viewer panels receiving their actual webview blur messages. The session itself remains unchanged; the incorrect behavior is in the displayed status.

Comment thread src/session.ts Outdated
focusedDataViewPanel = undefined;
focusedDataViewSessionId = null;
if (deferStatus) {
setTimeout(updateSessionStatusFromFocus, 0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Preserve an unattached terminal's status when deferred blur runs

This deferred fallback can overwrite a newer terminal-selection result. With session B active and viewer A focused, deliver A's dataview/blur, then select a terminal with no attached R session. switchSessionByTerminal() correctly sets R: (not attached), but intentionally leaves activeSession as B; when this timer runs, updateSessionStatusFromFocus() sees no focused viewer and writes B's PID over the detached status. I reproduced the sequence with await send(aList, { message: 'dataview/blur' }); await session.switchSessionByTerminal(unattachedTerminal); followed by one timer tick, and confirmed it with actual blur messages from both real List Viewer and Data Viewer panels in VS Code 1.140.0. Please make the deferred fallback respect the current terminal selection or invalidate it when a newer status decision has been made, and cover the unattached-terminal case alongside the attached-terminal blur test.

@Fred-Wu
Fred-Wu force-pushed the feature/viewer-session-pid branch 2 times, most recently from 15c9ced to 3003e74 Compare October 6, 2026 08:31
@Fred-Wu
Fred-Wu force-pushed the feature/viewer-session-pid branch from 3003e74 to 1c2850e Compare October 6, 2026 08:38

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The deferred-blur finding from my previous review is fixed: both real List Viewer and Data Viewer panels preserve the detached status after returning to an unattached terminal. The build, TypeScript check, and all 18 existing viewer ownership/panel tests pass locally; CI build, lint, and all three platform test jobs are green.

One regression remains in the new terminal-status cache, described inline. I reproduced it through the real Interactive commands in VS Code 1.140.0 with two live R sessions: after focusing one Interactive session, detaching the other changes the displayed status from that live session's PID to R: (not attached), while the focused notebook and Workspace owner remain unchanged. A separate focused session-cleanup check reproduces the same issue with an attached terminal, restoring that terminal's PID instead.

Comment thread src/session.ts Outdated
} else {
resetStatusBar();
}
} else if (statusTerminal) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Relinquish cached terminal status when selecting an Interactive session

statusTerminal survives InteractiveManager.activate() calling activateSession(view.target), and VS Code keeps window.activeTerminal set while a notebook is focused. This branch therefore prefers the old terminal on the next status reconciliation, even after an explicit Interactive selection. To reproduce, select an unattached terminal, open two Interactive sessions, focus session A, then detach session B through r.interactive.detach: A remains the focused notebook and Workspace owner, but unregisterSessionTransport() invokes this resolver and changes A's displayed PID to R: (not attached). With an attached terminal, it restores that terminal's PID instead. Clear or supersede the cached terminal ownership on explicit session activation so unrelated cleanup cannot overwrite the focused Interactive session's status.

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The cached-terminal cleanup finding from my previous review is fixed. The original regression now passes both in the focused ownership harness and through real Interactive commands with two live R sessions: detaching the other session preserves the focused session's PID and Workspace owner. Local bundling/typecheck and all 18 existing viewer ownership/panel tests pass using the PR's cached vscode-languageclient 9.0.1 dependency.

Two regressions remain in the new terminal fallback, described inline. I reproduced the active-session switch on actual List Viewer and Data Viewer panels when moving focus to Explorer, without focusing the terminal. A separate controlled delayed-processId check reproduces the newer viewer PID being overwritten by an older terminal lookup.

Comment thread src/session.ts Outdated
Comment on lines +323 to +324
if (panel.active && window.activeTerminal) {
void switchSessionByTerminal(window.activeTerminal);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Avoid activating the last terminal on every webview blur

panel.active remains true when keyboard focus moves to Explorer, the Workspace tree, or another sidebar, and window.activeTerminal still identifies the last selected terminal. This condition therefore treats those focus changes as terminal focus. Moreover, switchSessionByTerminal() calls activateSession(target, false), so the fallback changes the active R session and Workspace owner as well as the PID. I reproduced this with both real viewer types: select attached terminal B, explicitly activate session A and focus A's viewer, then run workbench.view.explorer; the actual dataview/blur message changes activeSession from A to B. Subsequent unbound workspace actions or console routing can now use B unexpectedly. Resolve the display without activating a terminal unless terminal focus/selection actually caused the transition.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am not able to reproduce this behaviour.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for checking. The missing precondition in my description was that the explicitly active session is A, while the last selected terminal still belongs to B. If activeSession is already B and only viewer A gains focus, this check does not demonstrate an ownership change. My native test uses activateSession(A) to model selecting an Interactive session without changing the terminal selection; it does not imply that ordinary viewer focus activates A. Switching between two R terminals can mask this prerequisite because selecting terminal A also makes window.activeTerminal A. For a manual attempt, select Interactive session A while terminal B remains the last selected terminal, then focus A's expanded viewer and move to Explorer.

The exact native sequence on 466929c was:

  1. Create a real VS Code terminal B and associate it through an IPC attach handshake using that terminal's process ID.
  2. Focus B, then explicitly activate requester-backed session A (session.activateSession(A)). Leave window.activeTerminal pointing to B.
  3. Open A's List Viewer or Data Viewer and wait for its actual dataview/focus message. At this point the displayed PID is A's and activeSession === A.
  4. Execute workbench.view.explorer (equivalent to moving keyboard focus to Explorer). Do not select or focus the terminal. Wait for the actual dataview/blur message.
  5. Check activeSession after the asynchronous fallback completes.

I reran both real-panel cases before replying. They reach the ownership assertion, rather than timing out during focus setup:

List Viewer: expected review-sidebar-list-a, actual review-sidebar-list-b
Data Viewer: expected review-sidebar-table-a, actual review-sidebar-table-b
panel.active: true
window.activeTerminal.name: PR review sidebar terminal B
messages: dataview/blur, review/blur

The key distinction is that WebviewPanel.active means the selected editor, not keyboard focus. It stays true for Explorer focus, so lines 323–324 invoke switchSessionByTerminal(B), which also changes activeSession. This was verified on macOS arm64 / VS Code 1.140.0. A is a requester-backed test transport and B uses an IPC test client; the test isolates focus and ownership without requiring two real R processes. The real webview focus/blur events and terminal are not mocked.

Here is a patch against 466929c5a37e057439f5d39a76bda5db70bd49cb containing the two short deterministic tests and both native-panel tests. It applies cleanly to that revision, typechecks, and reproduces all four assertion failures. The deterministic ownership and delayed-lookup tests both pass when only src/session.ts is restored to the preceding 1c2850e revision.

Reproduction patch — copy the fenced diff to pr1828-repro.patch
--- a/src/test/suite/dataViewerSessions.test.ts
+++ b/src/test/suite/dataViewerSessions.test.ts
@@ -317,6 +317,45 @@
         assert.strictEqual(statusBar.text, 'R: (not attached)');
     });
 
+    test('pr1828 repro: viewer blur does not switch the active session to the last selected terminal', async () => {
+        const terminalPid = 47202;
+        const terminal = { processId: Promise.resolve(terminalPid) } as unknown as vscode.Terminal;
+        sandbox.stub(vscode.window, 'terminals').value([terminal]);
+        sandbox.stub(vscode.window, 'activeTerminal').value(terminal);
+        const b = await attach('review-blur-terminal-b', os.hostname(), terminalPid);
+        await session.switchSessionByTerminal(terminal);
+        const a = await attach('review-blur-viewer-a');
+        const aList = await open(a, 'list');
+        assert.strictEqual(session.activeSession?.sessionId, a.id);
+        aList.activate();
+        await send(aList, { message: 'dataview/focus' });
+        await send(aList, { message: 'dataview/blur' });
+        await new Promise(resolve => setTimeout(resolve, 50));
+        console.info('Review blur ownership:', { expected: a.id, actual: session.activeSession?.sessionId });
+        assert.strictEqual(session.activeSession?.sessionId, a.id);
+        assert.notStrictEqual(session.activeSession?.sessionId, b.id);
+    });
+
+    test('pr1828 repro: delayed terminal lookup preserves a viewer focused after terminal selection', async () => {
+        let resolvePid: (pid: number) => void = () => undefined;
+        const terminalPid = 47203;
+        const terminal = { processId: Promise.resolve(terminalPid) } as unknown as vscode.Terminal;
+        sandbox.stub(vscode.window, 'terminals').value([terminal]);
+        sandbox.stub(vscode.window, 'activeTerminal').value(terminal);
+        const a = await attach('review-pending-viewer-a');
+        await attach('review-pending-terminal-b', os.hostname(), terminalPid);
+        const aList = await open(a, 'list');
+        Object.defineProperty(terminal, 'processId', { value: new Promise<number>(resolve => { resolvePid = resolve; }) });
+        const pending = session.switchSessionByTerminal(terminal);
+        aList.activate();
+        await send(aList, { message: 'dataview/focus' });
+        assert.strictEqual(statusBar.text, 'R 4.6.0: review-pending-viewer-a');
+        resolvePid(terminalPid);
+        await pending;
+        console.info('Review delayed terminal lookup status:', statusBar.text);
+        assert.strictEqual(statusBar.text, 'R 4.6.0: review-pending-viewer-a');
+    });
+
     test('active session disconnect preserves a focused viewer PID', async () => {
         const a = await attach('viewer-session-a');
         const b = await attach('viewer-session-b');
--- a/src/test/suite/listViewerPanels.test.ts
+++ b/src/test/suite/listViewerPanels.test.ts
@@ -1,11 +1,22 @@
 import * as assert from 'assert';
 import * as path from 'path';
+import * as net from 'net';
+import * as os from 'os';
+import * as extension from '../../extension';
 import * as sinon from 'sinon';
 import * as vscode from 'vscode';
 import { mockExtensionContext } from '../common/mockvscode';
 import * as session from '../../session';
 import { GlobalEnvItem } from '../../workspaceViewer';
 
+async function reviewWait(condition: () => boolean): Promise<void> {
+    const deadline = Date.now() + 5000;
+    while (!condition()) {
+        if (Date.now() > deadline) { throw new Error('Timed out waiting for review state'); }
+        await new Promise(resolve => setTimeout(resolve, 10));
+    }
+}
+
 suite('List viewer panels', () => {
     let sandbox: sinon.SinonSandbox;
     const panels: vscode.WebviewPanel[] = [];
@@ -21,6 +32,87 @@
         panels.splice(0).forEach(panel => { panel.dispose(); });
         sandbox.restore();
     });
+
+    for (const source of ['list', 'table']) {
+        test(`pr1828 repro: ${source} viewer to sidebar keeps the explicitly active session`, async () => {
+            const statusBar = { text: '', tooltip: '', show: sandbox.stub() } as unknown as vscode.StatusBarItem;
+            sandbox.stub(extension, 'sessionStatusBarItem').value(statusBar);
+            const request = sandbox.stub().callsFake((message: {method?: string}) => Promise.resolve(
+                message.method === 'dataview_init' ? { columns: [], totalRows: 0 }
+                    : message.method === 'dataview_page' ? { rows: [], totalRows: 0, totalUnfiltered: 0, lastRow: 0 }
+                        : { children: [], next_start: null, globalenv: {}, search: [], loaded_namespaces: [] }
+            ));
+            const a = session.registerSessionTransport(`review-sidebar-${source}-a`, 'host', '/tmp', request);
+            a.pid = '111'; a.rVer = '4.6.0';
+            const terminal = vscode.window.createTerminal({ name: 'PR review sidebar terminal B', ...(process.platform === 'win32' ? {} : { shellPath: '/bin/sh', shellArgs: [] }) });
+            const socket = net.createConnection(await session.getGlobalPipePath());
+            const bId = `review-sidebar-${source}-b`;
+            let buffer = '';
+            socket.on('data', (data: Buffer) => {
+                buffer += data.toString();
+                let newline: number;
+                while ((newline = buffer.indexOf('\n')) >= 0) {
+                    const message = JSON.parse(buffer.slice(0, newline)) as { id: number };
+                    buffer = buffer.slice(newline + 1);
+                    socket.write(JSON.stringify({ jsonrpc: '2.0', id: message.id,
+                        result: { globalenv: {}, search: [], loaded_namespaces: [] } }) + '\n');
+                }
+            });
+            const selected = vscode.window.onDidChangeActiveTerminal(value => { void session.switchSessionByTerminal(value); });
+            try {
+                await new Promise<void>((resolve, reject) => {
+                    socket.once('connect', resolve); socket.once('error', reject);
+                });
+                const terminalPid = await terminal.processId;
+                socket.write(JSON.stringify({ jsonrpc: '2.0', method: 'attach', params: {
+                    protocol_version: 2, session_id: bId, host: os.hostname(),
+                    pid: terminalPid, version: '4.6.0', tempdir: '/tmp', wd: '/tmp',
+                } }) + '\n');
+                assert.strictEqual(await session.waitForTerminalReady(terminal, 5000), true);
+                terminal.show(false);
+                await vscode.commands.executeCommand('workbench.action.terminal.focus');
+                await session.switchSessionByTerminal(terminal);
+                await session.activateSession(a);
+                const create = sandbox.spy(vscode.window, 'createWebviewPanel');
+                await session.showDataView(source, 'json', 'Review sidebar viewer A', '', 'Two', `review-sidebar-${source}`, undefined, a.sessionId, 1);
+                const panel = create.lastCall.returnValue;
+                panels.push(panel);
+                const messages: Array<{message?: string; focused?: boolean}> = [];
+                panel.webview.onDidReceiveMessage(message => { messages.push(message); });
+                panel.webview.html = panel.webview.html.replace('</body>', `<script>
+                    window.addEventListener('focus', () => vscode.postMessage({ message: 'review/focus' }));
+                    window.addEventListener('blur', () => vscode.postMessage({ message: 'review/blur' }));
+                    vscode.postMessage({ message: 'review/ready', focused: document.hasFocus() });
+                </script></body>`);
+                await reviewWait(() => messages.some(message => message.message === 'review/ready'));
+                terminal.show(false);
+                await vscode.commands.executeCommand('workbench.action.terminal.focus');
+                await new Promise(resolve => setTimeout(resolve, 100));
+                await session.activateSession(a);
+                for (let attempt = 0; attempt < 3 && !messages.some(message => message.message === 'review/focus'); attempt++) {
+                    panel.reveal(vscode.ViewColumn.Two, false);
+                    await vscode.commands.executeCommand('workbench.action.focusActiveEditorGroup');
+                    await new Promise(resolve => setTimeout(resolve, 250));
+                }
+                await reviewWait(() => messages.some(message => message.message === 'review/focus'));
+                await reviewWait(() => statusBar.text === 'R 4.6.0: 111');
+                assert.strictEqual(session.activeSession?.sessionId, a.sessionId);
+                messages.splice(0);
+                await vscode.commands.executeCommand('workbench.view.explorer');
+                await reviewWait(() => messages.some(message => message.message === 'dataview/blur'));
+                await new Promise(resolve => setTimeout(resolve, 100));
+                console.info('Review native sidebar ownership:', {
+                    expected: a.sessionId, actual: session.activeSession?.sessionId,
+                    panelActive: panel.active, activeTerminal: vscode.window.activeTerminal?.name, status: statusBar.text, messages,
+                });
+                assert.strictEqual(session.activeSession?.sessionId, a.sessionId);
+                assert.strictEqual(statusBar.text, 'R 4.6.0: 111');
+            } finally {
+                selected.dispose(); socket.destroy(); terminal.dispose();
+                await session.cleanupSession(a.sessionId); await session.cleanupSession(bId);
+            }
+        });
+    }
 
     for (const source of ['list', 'table']) {
         test(`reopens a disposed ${source} viewer using real VS Code panels`, async () => {

Run from a separate checkout of the reviewed revision, using dependencies from its lockfile. Save this as pr1828-repro.mjs so only the two relevant test files run:

import { defineConfig } from '@vscode/test-cli';
export default defineConfig({
    files: [
        'out/test/suite/dataViewerSessions.test.js',
        'out/test/suite/listViewerPanels.test.js',
    ],
    mocha: { ui: 'tdd', timeout: 20000, color: false },
    desktop: { installExtensions: ['REditorSupport.r-syntax'] },
    launchArgs: ['--disable-workspace-trust'],
});
git apply pr1828-repro.patch
pnpm run compile
pnpm exec tsc -p ./
pnpm exec vscode-test --config pr1828-repro.mjs --code-version 1.140.0 \
  --grep 'pr1828 repro:'

The first two tests control state directly inside the existing ownership harness. The native tests need a foreground VS Code test window and wait for real webview focus before moving to Explorer. Please keep those setup timeouts distinct from the final ownership assertion; a timeout before dataview/focus does not reproduce the finding.

Comment thread src/session.ts Outdated
if (revision !== terminalSelectionRevision) {
return;
}
updateSessionStatusBar(target);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Preserve viewer focus acquired during the terminal lookup

The revision only changes for another terminal selection, so focusing a viewer while terminal.processId is pending leaves this terminal update valid. After the promise resolves, this unconditional status update overwrites the newer viewer PID even though focusedDataViewPanel now points to that viewer. The previous resolver checked viewer focus at completion; the new direct update loses that protection. A controlled delayed-processId regression reproduces it: start switchSessionByTerminal(B), focus viewer A and confirm A's PID, then resolve B's PID; the viewer remains focused but displays B's PID. Recheck the current focused surface, or invalidate pending terminal display updates when a newer viewer/Interactive focus occurs.

@Fred-Wu Fred-Wu Oct 6, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am not able to reproduce this. When session B is active, and focus on View A, and change to session A, will show A's PID, but with a tiny flash that first shows B's PID then resolves to A's PID. The previous changes tried to address this flash, but the behaviour still exists. However, the PID appears in the end is correct.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks — the sequence you describe is consistent with the final PID being A. My second finding requires a different event order: B's terminal lookup starts first, A's viewer gains focus while that lookup is pending, and B's lookup completes last. Changing to session A after B completes restores A and does not exercise this race.

This one is a controlled asynchronous regression test, not a claim that I can reliably reproduce the race by clicking already-running terminals. An existing terminal's processId is usually already resolved, so a manual test can be too fast to hit the ordering. The mocked promise makes the contract explicit:

start switchSessionByTerminal(B), hold B.processId unresolved
focus viewer A -> status is A
resolve B.processId
await the original switch -> status is B, although viewer A remains focused

Add this inside the existing Viewer session ownership suite in src/test/suite/dataViewerSessions.test.ts; it reuses that file's attach, open, send, sandbox, and statusBar helpers:

test('pr1828 repro: delayed terminal lookup preserves a viewer focused after terminal selection', async () => {
    let resolvePid: (pid: number) => void = () => undefined;
    const terminalPid = 47203;
    const terminal = {
        processId: Promise.resolve(terminalPid),
    } as unknown as vscode.Terminal;
    sandbox.stub(vscode.window, 'terminals').value([terminal]);
    sandbox.stub(vscode.window, 'activeTerminal').value(terminal);

    const a = await attach('review-pending-viewer-a');
    await attach('review-pending-terminal-b', os.hostname(), terminalPid);
    const aList = await open(a, 'list');

    // Attach first so B has a real protocol association; delay only the lookup.
    Object.defineProperty(terminal, 'processId', {
        value: new Promise<number>(resolve => { resolvePid = resolve; }),
    });
    const pending = session.switchSessionByTerminal(terminal);
    aList.activate();
    await send(aList, { message: 'dataview/focus' });
    assert.strictEqual(statusBar.text, 'R 4.6.0: review-pending-viewer-a');

    resolvePid(terminalPid);
    await pending;
    assert.strictEqual(statusBar.text, 'R 4.6.0: review-pending-viewer-a');
});

Rerunning this against 466929c fails the final assertion with actual R 4.6.0: 47203. The same test passes with the preceding 1c2850e src/session.ts. This is a persistent wrong PID at completion of the tested sequence, distinct from the brief flash you observed. It demonstrates a missing stale-result guard under a delayed lookup; the frequency of that delay in normal manual use is not established by this test. The existing revision check rejects newer terminal selections, but viewer focus does not change that revision.

The companion reply on the sidebar finding contains a complete applyable patch and commands, including this test. To run only this case, use --grep 'pr1828 repro: delayed terminal'.

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The two findings from my previous review are addressed. The native List Viewer and Data Viewer sidebar checks now preserve the explicitly active session, and terminal activation completion respects newer viewer focus. Local bundling/typecheck, all 20 updated viewer ownership/panel tests, and all 51 terminal binding/lifecycle tests pass; CI build, lint, and macOS/Linux/Windows tests are green.

One display regression remains in returning to an already selected terminal, described inline. I reproduced it with both real viewer types and separately with a live arf Interactive session evaluating iris and opening its actual table viewer. The terminal was a real VS Code terminal with an IPC test session association. In each case the actual webview blur message arrived, no new active-terminal event fired, and the status retained the Interactive session's PID while keyboard focus was in the terminal. The underlying active session correctly remained unchanged.

Comment thread src/session.ts
Comment on lines +323 to +324
} else if (activeSession) {
updateSessionStatusBar(activeSession);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Restore the selected terminal's display when refocusing it

This fallback always displays activeSession, which can differ from the still-selected terminal after choosing an Interactive session. VS Code does not emit onDidChangeActiveTerminal when keyboard focus returns to the same terminal, so switchSessionByTerminal() cannot correct the display. Reproduction on macOS arm64 / VS Code 1.140.0: (1) select attached terminal B, (2) open/focus Interactive session A without changing the terminal selection, (3) open and focus A's expanded table or list viewer, (4) focus terminal B again, without selecting another terminal. The actual dataview/blur arrives with panel.active === true; the terminal-selection event count stays unchanged; this branch leaves A's PID displayed in B's terminal. The live Interactive table check recorded actual R 4.6.1: 35567 versus terminal B's expected R 4.6.0: 35770, with one active-terminal event before and after refocus. Both native viewer checks reproduce the same result. Please restore B's display for a real terminal-focus transition while preserving the sidebar fix and leaving activeSession unchanged. This needs focus information beyond panel.active, since both sidebar and terminal focus leave that flag true.

To adapt the real-panel reproduction patch I posted earlier, keep the setup that selects B then explicitly activates A, and replace the workbench.view.explorer transition with:

const before = terminalEvents; // increment in onDidChangeActiveTerminal
terminal.show(false);
await vscode.commands.executeCommand('workbench.action.terminal.focus');
await reviewWait(() => messages.some(message => message.message === 'dataview/blur'));
await new Promise(resolve => setTimeout(resolve, 100));
assert.strictEqual(terminalEvents, before);
assert.strictEqual(session.activeSession?.sessionId, a.sessionId);
assert.strictEqual(statusBar.text, `R 4.6.0: ${terminalPid}`);

Do not call switchSessionByTerminal() after refocusing B in this check: doing so supplies the missing event and masks the display regression.

@Fred-Wu

Fred-Wu commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

This has become overly complicated for its intended purpose. I am closing this PR in favor of the simpler approach in #1848.

@Fred-Wu Fred-Wu closed this Oct 7, 2026
@Fred-Wu
Fred-Wu deleted the feature/viewer-session-pid branch October 8, 2026 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(Feat): update status bar PID when switching between Data and List Viewers from different sessions

2 participants