diff --git a/extensions/levelcode-ai/extension.js b/extensions/levelcode-ai/extension.js index 4da3933..d2e023c 100644 --- a/extensions/levelcode-ai/extension.js +++ b/extensions/levelcode-ai/extension.js @@ -58,6 +58,9 @@ let sidebarChatView; // the contributed view, so the panel can hand the slot /** @type {vscode.WebviewPanel | undefined} */ let chatEditorPanel; // set only while the chat is open as an editor tab let chatProvider; // the single provider instance; both surfaces wire through it +// Closing the tab and MOVING the chat both end in panel.dispose(), and they must not mean the same +// thing. Set only by moveChatToSidebar, read once by onDidDispose, cleared immediately. +let movingChatToSidebar = false; // The visible transcript lives in the webview's DOM, so swapping surfaces would blank it. Set before // handing over; the freshly-loaded surface replays on its `ready`, which is the first moment it can // receive anything at all. @@ -2311,19 +2314,37 @@ async function openChatInEditor(opts) { dbg('chat.openInEditor', {}); panel.onDidDispose(() => { + // A MOVE and a CLOSE both land here. Until the chat opened centred by default they were the same + // thing — the sidebar was home, so surrendering the tab meant going home — and this handler + // revealed the sidebar unconditionally. With `chat.startLocation: editor` that turns ⌘W into + // "reopen on the right", and there is no way to put the chat away at all: close the tab, the + // panel appears; close the panel, it is still bound to come back next time. + // + // So the reveal now happens ONLY for a deliberate move. A plain close closes. + const moving = movingChatToSidebar; + movingChatToSidebar = false; chatEditorPanel = undefined; + if (sidebarChatView) { + // The view exists, so hand the conversation back to it either way — otherwise the chat would + // be live nowhere while a resolved view sits there showing a stale hand-over card. Only the + // REVEAL is conditional: making a hidden view live costs nothing and is correct the moment + // the user opens it. pendingTranscriptReplay = 'Back in the sidebar'; chatProvider.makeLive(sidebarChatView.webview); - sidebarChatView.show?.(true); - } else { - // The view was never resolved (the container has not been opened this session). Reveal it — - // resolveWebviewView then makes it live, and without this the chat would have no surface at all. + if (moving) { sidebarChatView.show?.(true); } + } else if (moving) { + // Moving with no resolved view: reveal it, and resolveWebviewView makes it live on arrival. activeWebview = undefined; pendingTranscriptReplay = 'Back in the sidebar'; - focusChatView('editorClosed'); + focusChatView('movedToSidebar'); + } else { + // Plain close, nothing resolved: the chat has no surface, which is exactly what was asked + // for. Drop the reference so nothing posts into a disposed webview; reopening from the + // command, the sidebar, or the next window restores it. + activeWebview = undefined; } - dbg('chat.closedEditor', {}); + dbg('chat.closedEditor', { moving }); }); } @@ -2336,7 +2357,13 @@ async function openChatInEditor(opts) { * hand-over changed. */ function moveChatToSidebar() { - if (chatEditorPanel) { chatEditorPanel.dispose(); return undefined; } + if (chatEditorPanel) { + // Tell onDidDispose this is a MOVE. Disposing is how the move is performed, so without this flag + // the hand-over cannot tell it apart from the user simply closing the tab. + movingChatToSidebar = true; + chatEditorPanel.dispose(); + return undefined; + } // Already there (or never moved) — just reveal it, so the command is never a silent no-op. // // RETURNED, not fired and forgotten. `registerCommand` awaits whatever the handler returns, so a diff --git a/extensions/levelcode-ai/test/chatSurface.test.js b/extensions/levelcode-ai/test/chatSurface.test.js index 90e9af6..d455520 100644 --- a/extensions/levelcode-ai/test/chatSurface.test.js +++ b/extensions/levelcode-ai/test/chatSurface.test.js @@ -61,9 +61,14 @@ function fnBody(src, name) { test('SURFACE: only makeLive() ever moves the conversation, so two surfaces cannot both be live', () => { // `post()` writes to activeWebview. If anything else assigned it, a hand-over could leave the // pointer on a webview the user is no longer looking at — messages vanish into a hidden DOM. + // Stated as an invariant rather than an exact list: the number of RESETS is allowed to grow (the + // dispose path now has one per close route), but the number of places that point it at a live + // surface is not. Pinning the whole list meant adding a reset looked like a regression. const writes = [...ext.matchAll(/activeWebview\s*=\s*([^;]+);/g)].map((m) => m[1].trim()); - assert.deepStrictEqual(writes.sort(), ['undefined', 'webview'], - 'activeWebview is assigned somewhere other than makeLive()/the dispose reset: ' + writes.join(' | ')); + const live = writes.filter((w) => w !== 'undefined'); + assert.deepStrictEqual(live, ['webview'], + 'activeWebview is pointed at a surface somewhere other than makeLive(): ' + writes.join(' | ')); + assert.ok(writes.length > live.length, 'nothing ever releases activeWebview — a disposed webview stays addressable'); assert.match(fnBody(ext, 'openChatInEditor'), /chatProvider\.makeLive\(panel\.webview\)/, 'the panel never becomes the live surface'); }); @@ -140,14 +145,18 @@ test('RESTORE: closing the tab and "Bring it back" are the SAME path', () => { assert.match(open, /chatEditorPanel = undefined/, 'the panel ref outlives the panel'); }); -test('RESTORE: a sidebar that was never resolved is revealed rather than assumed', () => { +test('RESTORE: a MOVE to a sidebar that was never resolved reveals it rather than assuming it', () => { // If the container has not been opened this session, sidebarChatView is undefined — restoring by - // writing to it would throw, and doing nothing would leave the chat with no surface at all. + // writing to it would throw, and for a MOVE, doing nothing would leave the chat with no surface at + // all after the user explicitly asked for it on the right. + // + // This used to assert the reveal happened on EVERY close, which was right while the sidebar was + // home and became a bug the moment the editor became the default: it turned closing the tab into + // reopening the panel. The reveal is now scoped to the move, and the plain-close half is pinned by + // the CLOSE tests below. const open = fnBody(ext, 'openChatInEditor'); - // The reveal now goes through focusChatView() so its rejection cannot go unhandled; what this test - // cares about is unchanged — the else-branch must still reveal the view rather than assume it. - assert.match(open, /if \(sidebarChatView\) \{[\s\S]*\} else \{[\s\S]*focusChatView\(/, - 'the never-resolved sidebar case is unhandled'); + assert.match(open, /if \(sidebarChatView\) \{[\s\S]*\} else if \(moving\) \{[\s\S]*focusChatView\(/, + 'a move with no resolved sidebar view no longer reveals it — the chat would land nowhere'); }); test('RESTORE: while detached, the sidebar shows the hand-off card, not a second chat', () => { @@ -354,4 +363,44 @@ test('FOCUS: the shared helper logs the failure and names who caused it', () => assert.ok(callers >= 6, 'expected the background reveals to route through the helper, found ' + callers); }); +test('CLOSE: closing the tab closes the chat — it does not reopen on the right', () => { + // The regression this pins, reported from a real build: with chat.startLocation defaulting to the + // editor, onDidDispose revealed the sidebar unconditionally. Closing the tab therefore POPPED THE + // PANEL OPEN on the right, and there was no way to put the chat away at all — close the tab, the + // panel appears; close the panel, it comes back with the next window. + // + // A move and a close both end in panel.dispose(), so they are told apart by an explicit flag rather + // than by anything dispose itself can see. + const dispose = ext.slice(ext.indexOf('panel.onDidDispose'), ext.indexOf('panel.onDidDispose') + 1800); + assert.match(dispose, /const moving = movingChatToSidebar;/, 'dispose cannot tell a move from a close'); + assert.match(dispose, /movingChatToSidebar = false;/, 'the flag must be cleared, or the NEXT close reveals too'); + + // The two reveals are the whole bug. Both must now be conditional. + assert.match(dispose, /if \(moving\) \{ sidebarChatView\.show\?\.\(true\); \}/, + 'a plain close still forces the resolved sidebar view open'); + // The no-view path is the one that actually bit: with nothing resolved, dispose used to call + // focusChatView() unconditionally, which is what POPPED THE PANEL OPEN. It must now sit inside the + // `moving` branch — checked positionally, because that is the property, and a regex trying to + // describe the surrounding block shape is how the first version of this assertion broke. + const revealIdx = dispose.indexOf('focusChatView('); + const movingBranchIdx = dispose.indexOf('else if (moving)'); + assert.ok(movingBranchIdx > 0, 'the no-view close path is no longer split on `moving`'); + assert.ok(revealIdx > movingBranchIdx, + 'focusChatView is reachable on a plain close — closing the tab reopens the chat on the right'); + + // …and a close must still release the surface, or messages post into a disposed webview. + assert.match(dispose, /activeWebview = undefined;/, 'the disposed webview is still referenced'); +}); + +test('CLOSE: a deliberate move still hands the conversation over', () => { + // The other half — it would be easy to fix the close by making the move stop working. + assert.match(fnBody(ext, 'moveChatToSidebar'), /movingChatToSidebar = true;[\s\S]{0,120}chatEditorPanel\.dispose\(\)/, + 'the move must announce itself BEFORE disposing, or dispose reads a stale flag'); + const dispose = ext.slice(ext.indexOf('panel.onDidDispose'), ext.indexOf('panel.onDidDispose') + 1800); + assert.match(dispose, /chatProvider\.makeLive\(sidebarChatView\.webview\)/, + 'a resolved sidebar view must still become live, or the chat is live nowhere'); + assert.match(dispose, /pendingTranscriptReplay = 'Back in the sidebar'/, + 'the transcript must still replay into whichever surface takes over'); +}); + console.log('\nchatSurface: ' + n + ' tests passed.');