Repository navigation
Conversation
renkun-ken
left a comment
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
[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.
| panel.onDidChangeViewState(({ webviewPanel }) => { | ||
| if (!webviewPanel.active || !sessionId) { | ||
| return; |
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
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.
|
A few new issues identified, and fixing them now. |
renkun-ken
left a comment
There was a problem hiding this comment.
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.
| focusedDataViewPanel = undefined; | ||
| focusedDataViewSessionId = null; | ||
| if (deferStatus) { | ||
| setTimeout(updateSessionStatusFromFocus, 0); |
There was a problem hiding this comment.
[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.
15c9ced to
3003e74
Compare
3003e74 to
1c2850e
Compare
renkun-ken
left a comment
There was a problem hiding this comment.
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.
| } else { | ||
| resetStatusBar(); | ||
| } | ||
| } else if (statusTerminal) { |
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
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.
| if (panel.active && window.activeTerminal) { | ||
| void switchSessionByTerminal(window.activeTerminal); |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
I am not able to reproduce this behaviour.
There was a problem hiding this comment.
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:
- Create a real VS Code terminal B and associate it through an IPC
attachhandshake using that terminal's process ID. - Focus B, then explicitly activate requester-backed session A (
session.activateSession(A)). Leavewindow.activeTerminalpointing to B. - Open A's List Viewer or Data Viewer and wait for its actual
dataview/focusmessage. At this point the displayed PID is A's andactiveSession === A. - Execute
workbench.view.explorer(equivalent to moving keyboard focus to Explorer). Do not select or focus the terminal. Wait for the actualdataview/blurmessage. - Check
activeSessionafter 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.
| if (revision !== terminalSelectionRevision) { | ||
| return; | ||
| } | ||
| updateSessionStatusBar(target); |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
| } else if (activeSession) { | ||
| updateSessionStatusBar(activeSession); |
There was a problem hiding this comment.
[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.
|
This has become overly complicated for its intended purpose. I am closing this PR in favor of the simpler approach in #1848. |
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.