diff --git a/packages/vscode/src/DOCUMENTATION.md b/packages/vscode/src/DOCUMENTATION.md index bc5f2481..7c1a2993 100644 --- a/packages/vscode/src/DOCUMENTATION.md +++ b/packages/vscode/src/DOCUMENTATION.md @@ -87,7 +87,7 @@ The webview build emits each worker as one self-contained file. VS Code webviews - A thread never owns a draft. It mints the draft id, hands the payload to a chat webview (the active or newly opened session panel, else the sidebar), and follows the webview's whole-draft-list `inlineComments:sync` snapshots: present means show, absent after having been seen means dispose. A snapshot is tagged with the surface that produced it (a panel id or `sidebar`) and only decides that surface's own threads, because every webview runs its own draft store. - A comment the composer never confirms holding within 30 s is retracted from every surface's pending hold, its thread disposed, and the user told, so a thread cannot promise a send that will never happen. - `inlineCommentSelection.ts` holds the pure pieces (line ranges, the diff-side and real-path resolution for `git:` documents, the pending hold, removal broadcast, thread fate) without the `vscode` import so they are unit-tested directly. - - Webview side: `webview/inlineCommentRemovals.ts` remembers removals that arrive before a delayed delivery lands, so a comment dropped while its panel was still booting does not appear as a chip later. The extension is not activated on startup for this; the right-click command activates it, and the gutter `+` appears from then on. + - Webview side: `webview/inlineCommentTarget.ts` decides where a delivered comment is filed. A session panel stamps its session on every comment it delivers and the webview waits until it shows that session; the sidebar files on its current session or open draft. Filing on the first snapshot with a directory put the draft under `draft` while a fresh panel was still loading its session list, a key that composer never reads. `webview/inlineCommentRemovals.ts` remembers removals that arrive before a delayed delivery lands, so a comment dropped while its panel was still booting does not appear as a chip later. The extension is not activated on startup for this; the right-click command activates it, and the gutter `+` appears from then on. ## Shared webview message ordering diff --git a/packages/vscode/src/SessionEditorPanelProvider.ts b/packages/vscode/src/SessionEditorPanelProvider.ts index 32bfbced..17b3f82c 100644 --- a/packages/vscode/src/SessionEditorPanelProvider.ts +++ b/packages/vscode/src/SessionEditorPanelProvider.ts @@ -30,6 +30,12 @@ type LineCommentPayload = { type SessionPanelState = { /** This panel's id, which is also its surface identity for comment threads. */ id: string; + /** + * The session this panel was opened for; null for a new-session panel. A + * comment delivered here names it, so the webview files the draft under that + * session's key rather than whatever it shows while still booting. + */ + sessionId: string | null; panel: vscode.WebviewPanel; sseStreams: Map; /** @@ -148,6 +154,7 @@ export class SessionEditorPanelProvider { const state: SessionPanelState = { id: panelId, + sessionId: initialSessionId, panel, sseStreams: new Map(), pendingLineComments: [], @@ -183,7 +190,7 @@ export class SessionEditorPanelProvider { void panel.webview.postMessage({ type: 'command', command: 'addLineComment', - payload: pending, + payload: { ...pending, targetSessionId: state.sessionId ?? undefined }, }); } @@ -363,7 +370,7 @@ export class SessionEditorPanelProvider { void entry.panel.webview.postMessage({ type: 'command', command: 'addLineComment', - payload, + payload: { ...payload, targetSessionId: entry.sessionId ?? undefined }, }); return entry.id; } diff --git a/packages/vscode/webview/inlineCommentTarget.test.ts b/packages/vscode/webview/inlineCommentTarget.test.ts new file mode 100644 index 00000000..1ac02ca4 --- /dev/null +++ b/packages/vscode/webview/inlineCommentTarget.test.ts @@ -0,0 +1,56 @@ +import { describe, test } from 'node:test'; +import assert from 'node:assert/strict'; +import { resolveCommentTarget } from './inlineCommentTarget'; + +const snapshot = (overrides = {}) => ({ + currentSessionId: null, + sessionDirectory: null, + draftOpen: false, + draftDirectory: null, + currentDirectory: '/repo', + ...overrides, +}); + +describe('resolveCommentTarget', () => { + test('a booting panel with a directory but no session keeps waiting', () => { + // This was the bug: the draft went under `draft` here, a key the + // session's composer never reads, and the chip never appeared. + assert.equal(resolveCommentTarget(snapshot(), 'ses_1'), null); + assert.equal(resolveCommentTarget(snapshot()), null); + }); + + test('a session panel files only once it shows the session the comment is for', () => { + assert.equal(resolveCommentTarget(snapshot({ currentSessionId: 'ses_other', sessionDirectory: '/repo' }), 'ses_1'), null); + assert.deepEqual( + resolveCommentTarget(snapshot({ currentSessionId: 'ses_1', sessionDirectory: '/repo/wt' }), 'ses_1'), + { directory: '/repo/wt', sessionKey: 'ses_1' }, + ); + }); + + test('the session directory wins over the webview directory', () => { + assert.deepEqual( + resolveCommentTarget(snapshot({ currentSessionId: 'ses_1', sessionDirectory: '/repo/wt' })), + { directory: '/repo/wt', sessionKey: 'ses_1' }, + ); + assert.deepEqual( + resolveCommentTarget(snapshot({ currentSessionId: 'ses_1' })), + { directory: '/repo', sessionKey: 'ses_1' }, + ); + }); + + test('the sidebar files on an open new-session draft', () => { + assert.deepEqual( + resolveCommentTarget(snapshot({ draftOpen: true, draftDirectory: '/repo/other' })), + { directory: '/repo/other', sessionKey: 'draft' }, + ); + assert.deepEqual( + resolveCommentTarget(snapshot({ draftOpen: true })), + { directory: '/repo', sessionKey: 'draft' }, + ); + }); + + test('nothing is filed without any directory', () => { + assert.equal(resolveCommentTarget(snapshot({ currentSessionId: 'ses_1', currentDirectory: null })), null); + assert.equal(resolveCommentTarget(snapshot({ draftOpen: true, currentDirectory: null })), null); + }); +}); diff --git a/packages/vscode/webview/inlineCommentTarget.ts b/packages/vscode/webview/inlineCommentTarget.ts new file mode 100644 index 00000000..11e73e8f --- /dev/null +++ b/packages/vscode/webview/inlineCommentTarget.ts @@ -0,0 +1,53 @@ +/** + * Where a comment arriving from the editor should be filed. + * + * Inline drafts are keyed by directory and session, and the composer reads + * exactly one key: the current session's, or `draft` while a new-session draft + * is open. A panel that has just been opened knows its directory before it has + * selected its session, so filing on the first snapshot that has a directory + * put the draft under `draft`, a key that panel's composer never reads. The + * chip never appeared while the editor thread reported it attached. + */ + +export interface CommentTargetSnapshot { + currentSessionId: string | null; + /** The current session's directory, when the session has one. */ + sessionDirectory: string | null; + /** Whether a new-session draft is open, and the directory it points at. */ + draftOpen: boolean; + draftDirectory: string | null; + /** The webview's current directory, the last resort. */ + currentDirectory: string | null; +} + +export interface CommentTarget { + directory: string; + sessionKey: string; +} + +/** + * The draft key for this snapshot, or null while the surface is not yet + * showing the session the comment is for. + * + * @param targetSessionId the session the extension delivered the comment to, + * when it knows one (a session panel); undefined for the sidebar and a + * new-session panel, which file wherever their composer currently is + */ +export function resolveCommentTarget(snapshot: CommentTargetSnapshot, targetSessionId?: string): CommentTarget | null { + const { currentSessionId } = snapshot; + if (targetSessionId) { + if (currentSessionId !== targetSessionId) return null; + const directory = snapshot.sessionDirectory ?? snapshot.currentDirectory; + return directory ? { directory, sessionKey: targetSessionId } : null; + } + if (currentSessionId) { + const directory = snapshot.sessionDirectory ?? snapshot.currentDirectory; + return directory ? { directory, sessionKey: currentSessionId } : null; + } + if (snapshot.draftOpen) { + const directory = snapshot.draftDirectory ?? snapshot.currentDirectory; + return directory ? { directory, sessionKey: 'draft' } : null; + } + // No session and no draft yet: the surface is still booting. + return null; +} diff --git a/packages/vscode/webview/main.tsx b/packages/vscode/webview/main.tsx index feb8cd14..2ce556d7 100644 --- a/packages/vscode/webview/main.tsx +++ b/packages/vscode/webview/main.tsx @@ -1,5 +1,6 @@ import { createVSCodeAPIs } from './api'; import { createRemovalTombstones } from './inlineCommentRemovals'; +import { resolveCommentTarget } from './inlineCommentTarget'; import { onCommand, onThemeChange, postBridgeNotification, proxyApiRequest, proxySessionMessageRequest, sendBridgeMessage, startSseProxy, stopSseProxy } from './api/bridge'; import { vscodeStreamPerfCount, vscodeStreamPerfMeasure, vscodeStreamPerfObserve } from './api/streamPerf'; import { extractBodyBase64, extractBodyText, extractJsonBody, hasInitBody } from './requestBodyTransport'; @@ -1348,11 +1349,15 @@ onCommand('addLineComment', (payload) => { code?: unknown; language?: unknown; comment?: unknown; + targetSessionId?: unknown; }; // The editor thread mints the id so it can track its own draft without a // round trip. Absent when the comment came from anywhere else. const draftId = typeof record.draftId === 'string' && record.draftId ? record.draftId : undefined; + // A session panel is told which session the comment is for, so it can wait + // until it actually shows that session. The sidebar files wherever it is. + const targetSessionId = typeof record.targetSessionId === 'string' && record.targetSessionId ? record.targetSessionId : undefined; const relativePath = typeof record.relativePath === 'string' ? record.relativePath : ''; const source = record.source === 'diff' ? 'diff' : 'file'; const side = record.side === 'original' || record.side === 'modified' ? record.side : undefined; @@ -1379,27 +1384,30 @@ onCommand('addLineComment', (payload) => { // reads. Directory precedence matches the composer's own. const resolveTarget = () => { const sessionState = useSessionUIStore.getState(); - const currentSessionId = sessionState.currentSessionId; - const sessionDirectory = currentSessionId ? sessionState.getDirectoryForSession(currentSessionId) : null; - const draftDirectory = sessionState.newSessionDraft?.open - ? sessionState.newSessionDraft.bootstrapPendingDirectory ?? sessionState.newSessionDraft.directoryOverride ?? null - : null; - const directory = sessionDirectory ?? draftDirectory ?? useDirectoryStore.getState().currentDirectory; - return directory ? { directory, sessionKey: currentSessionId ?? 'draft' } : null; + const currentSessionId = sessionState.currentSessionId ?? null; + const draft = sessionState.newSessionDraft; + return resolveCommentTarget({ + currentSessionId, + sessionDirectory: currentSessionId ? sessionState.getDirectoryForSession(currentSessionId) ?? null : null, + draftOpen: Boolean(draft?.open), + draftDirectory: draft?.open ? draft.bootstrapPendingDirectory ?? draft.directoryOverride ?? null : null, + currentDirectory: useDirectoryStore.getState().currentDirectory ?? null, + }, targetSessionId); }; - // A comment can arrive before the chat surface has finished booting: the - // extension opens the sidebar and posts after a fixed delay, which a cold - // webview can outlast. Dropping the draft here loses a comment the user - // already wrote and already saw accepted in the editor, so wait for the - // target to land instead. + // A comment can arrive before the chat surface shows its session: a panel + // opened for the comment knows its directory long before the session list + // has loaded and the session is selected. Filing before that put the draft + // under a key this composer never reads. Wait for the surface to land on + // the session (or an open draft) instead, within the extension's own + // confirmation deadline. let target = resolveTarget(); - for (let attempt = 0; !target && attempt < 40; attempt += 1) { + for (let attempt = 0; !target && attempt < 80; attempt += 1) { await new Promise((resolve) => setTimeout(resolve, 250)); target = resolveTarget(); } if (!target) { - console.warn('[openchamber] no directory resolved; dropping inline comment', { relativePath, startLine }); + console.warn('[openchamber] chat surface never showed the session; dropping inline comment', { relativePath, startLine, targetSessionId }); return; }