fix(chat): closing the tab tears the conversation down, not just seals it - #83
Conversation
…s it Both review points on #82. They landed on that PR's stale diff — I branched the wordmark off feat/chat-editor-only instead of develop, so #82 carried #81's commits until #81 merged. Both are real, and both are about code that is now on develop, so they are fixed here rather than in the wordmark PR (whose diff is now the two files it should always have been). 1. SEALING WAS ONLY HALF THE TEARDOWN sealLiveSession ends the SESSION — liveId() goes null, so the next chat opens visually empty — while `conversation` and `agentMessages` still held every previous turn. The next message therefore shipped the old history to the model. An empty-looking chat that secretly remembers is worse than either honest option. The out-of-process half was worse: background commands and MCP servers are DETACHED children, so they outlived the surface that was reporting on them, and an in-flight agent run kept editing files with nothing left to show for it. newChat's teardown moves into resetConversationState() and the close path calls it. One implementation, or the close path drifts — and it is the path nobody watches. It deliberately does NOT post: New Chat re-renders afterwards because it has a surface to re-render; the close path is tearing one down. 2. A LEGACY `secondarySidebar` SETTING PRODUCED A LYING LOG The value was valid until the chat became editor-only, so it is still sitting in real settings.json files. chatStartLocation still accepted it, so revealChatAtStartup logged `where: secondarySidebar` and then opened the editor tab. Right behaviour, wrong story — and the accepted set no longer matched the enum the package ships. Now mapped explicitly. Guards, each bypass-verified by reverting the fix: - the close sealing but leaving history loaded (the reported bug) - conversation, checkpoints, abort, reapCommands and reapMcp each removed individually - newChat growing its own copy of the teardown - the shared teardown starting to post, which is wrong on the close path - the legacy value no longer mapped; the removed surface accepted again Two bypasses initially looked like misses: commenting out `reapMcp()` and neutering the abort with `if (false)` both left the strings present, and these are presence checks. Redone as deletions — which is how they would actually regress — they fail correctly. Worth stating plainly: these guards catch removal, not disabling. 21 tests in chatSurface, 34 suites green.
There was a problem hiding this comment.
Pull request overview
This PR fixes LevelCode AI chat lifecycle correctness by ensuring that closing the chat editor tab fully tears down both in-memory and out-of-process conversation state (not just sealing the session), and by correcting legacy chat.startLocation handling so startup logging reflects the actual open location.
Changes:
- Extracts a shared
resetConversationState()teardown and calls it from both New Chat and the editor-tab dispose path. - Updates
chatStartLocation()to explicitly map legacysecondarySidebartoeditorand restrict the accepted set to the shipped enum. - Adds/updates
chatSurfaceguards to verify teardown on close and correct legacy setting mapping behavior using shipped source evaluation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| extensions/levelcode-ai/extension.js | Adds shared teardown on close/new chat; maps legacy start location value to avoid misleading logs. |
| extensions/levelcode-ai/test/chatSurface.test.js | Adds regression tests ensuring close tears down conversation state and legacy settings map correctly. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| checkpoints.length = 0; currentCheckpoint = null; // drop the per-turn restore stack | ||
| pendingContext = null; | ||
| contextFiles = []; | ||
| if (abort) { abort.abort(); } | ||
| if (abort) { abort.abort(); } // stop an in-flight run — its surface is going away | ||
| reapCommands(); // kill any background servers/watchers from the old session |
| // `secondarySidebar` was a valid value until the chat became editor-only, so it is still sitting in | ||
| // real settings.json files. Mapped explicitly rather than left to fall through the unknown-value | ||
| // path: the result is the same, but this way the debug log names the location we actually opened | ||
| // instead of reporting a surface that no longer exists. | ||
| if (raw === 'secondarySidebar') { return 'editor'; } |
Review on #83, and the first point is sharp: the teardown was losing a race it did not know it was in. Closing the tab mid-stream calls abort(), and the abort lands in handleSend's catch AFTER resetConversationState has already cleared `conversation` — where the abort branch pushed the partial reply straight back in. The result was a dangling assistant turn with no user turn in front of it, shipped to the model on the next send: exactly the leak the teardown exists to prevent, reintroduced by the teardown's own abort(). agentFlow's finally was worse than reported. runAgent holds `agentMessages` BY REFERENCE, so a teardown that rebinds the global leaves the run pushing into an orphaned array — and then `agentMessages.slice(sessTurnStart)` slices the NEW empty one with an index into the old, recording an empty turn against the wrong session. `abort = null` and `currentCheckpoint = null` are worse still: those clobber whatever turn came next, leaving a fresh run unstoppable and its checkpoint unclosed. Fixed with an epoch. resetConversationState bumps `conversationEpoch` FIRST — before clearing anything, so an abort landing mid-teardown already reads as stale — and each turn captures the epoch before installing its AbortController, then checks it before writing anything back. It also now clears pending approvals/questions, which the guarded finally no longer does on a torn-down turn. Second point: the chatStartLocation docstring still called `secondarySidebar` a supported surface ("kept because the sidebar is the right answer when…") while the code below mapped it away as legacy. In-code documentation sitting directly on top of the change is the worst place to leave a contradiction. Guards, each bypass-verified by reverting the fix: - the teardown no longer invalidating in-flight work - the epoch bumped AFTER clearing, which leaves the race window open - handleSend pushing back without checking; the finally nulling a new turn's controller - agentFlow's finally clobbering the next turn - the epoch captured after the controller is installed - the docstring dropping the legacy marker Caught while wiring it: handleSend referenced `epoch` without capturing it — a ReferenceError at runtime that `node --check` cannot see, because it only checks syntax. 23 tests in chatSurface, 34 suites green.
|
Both valid, and the first one is sharp. Fixed in 1ad4f14. The teardown was losing a race it didn't know it was inConfirmed by reading the path rather than taking it on faith. Closing mid-stream calls if (abort && abort.signal.aborted) {
if (assistant) { conversation.push({ role: 'assistant', content: assistant }); } // ← into the array just clearedThat leaves a dangling assistant turn with no user turn in front of it, shipped to the model on the next send — exactly the leak the teardown exists to prevent, reintroduced by the teardown's own abort().
|
line in the finally |
what it does after a teardown |
|---|---|
abort = null |
clobbers the next turn's controller — that run becomes unstoppable |
currentCheckpoint = null |
clobbers the next turn's checkpoint, and posts checkpointClosed for a stale turnId |
The fix: an epoch, bumped first
resetConversationState increments conversationEpoch before clearing anything, so an abort landing mid-teardown already reads as stale. Each turn captures the epoch before installing its AbortController and checks it before writing back.
Ordering is the whole property, so it's what the guards assert: the bump before the clear, the capture before the controller, the check before the first write in each block.
resetConversationState also now clears pending approvals/questions, since the guarded finally no longer does that for a torn-down turn.
Docstring
Right — it still called secondarySidebar a supported surface ("kept because the sidebar is the right answer when…") while the code below mapped it away as legacy. In-code documentation sitting directly on top of the change is the worst place to leave a contradiction.
Guards
Each bypass-verified by reverting the fix:
| bypass | caught |
|---|---|
| the teardown no longer invalidating in-flight work | ✓ |
| the epoch bumped after clearing — race window survives | ✓ |
handleSend pushing back without checking |
✓ |
the finally nulling a new turn's controller |
✓ |
agentFlow's finally clobbering the next turn |
✓ |
| the epoch captured after the controller is installed | ✓ |
| the docstring dropping the legacy marker | ✓ |
One thing worth naming: while wiring this I left handleSend referencing epoch without capturing it — a ReferenceError at runtime that node --check can't see, because it only checks syntax. Caught by grepping every reference for scope before running anything.
23 tests in chatSurface, 34 suites green.
Both review points from #82. They landed on that PR's stale diff — I branched the wordmark off
feat/chat-editor-onlyinstead ofdevelop, so #82 carried #81's commits until #81 merged. Both comments are real and both are about code now ondevelop, so they're fixed here. #82 has been rebased and its diff is back to the two files it should always have been.1. Sealing was only half the teardown
sealLiveSessionends the session —liveId()goes null, so the next chat opens visually empty — whileconversationandagentMessagesstill held every previous turn. The next message therefore shipped the old history to the model.An empty-looking chat that secretly remembers is worse than either honest option.
The out-of-process half was worse: background commands and MCP servers are detached children, so they outlived the surface reporting on them, and an in-flight agent run kept editing files with nothing left to show for it.
newChat's teardown moves intoresetConversationState()and the close path calls it:conversation,agentMessagescheckpointscontextFiles,pendingContextabort.abort()reapCommands(),reapMcp()One implementation, or the close path drifts — and it's the path nobody watches. It deliberately does not post: New Chat re-renders afterwards because it has a surface to re-render; the close path is tearing one down.
2. A legacy
secondarySidebarsetting produced a lying logThe value was valid until the chat became editor-only, so it's still sitting in real
settings.jsonfiles.chatStartLocationstill accepted it, sorevealChatAtStartuploggedwhere: secondarySidebarand then opened the editor tab — right behaviour, wrong story. The accepted set also no longer matched the enum the package ships.Now mapped explicitly, and the behaviour is checked by evaluating the shipped function with a stubbed config rather than a copy of it:
secondarySidebar→editor,none→none, unknown →editor.Guards
Each bypass-verified by reverting the fix:
conversation/checkpoints/abort/reapCommands/reapMcpremoved individuallynewChatgrows its own copy of the teardownTwo bypasses initially looked like misses. Commenting out
reapMcp()and neutering the abort withif (false)both left the strings present, and these are presence checks. Redone as deletions — how they'd actually regress — they fail correctly. Stating it plainly: these guards catch removal, not disabling.21 tests in
chatSurface, 34 suites green.