Repository navigation
fix(runtime): keep a client-dirty mirrored file dirty across a host republish - #21393
Conversation
…epublish The host publishes only its own store's isDirty and never learns about client edits, so rebuilding a mirrored OpenFile from the snapshot cleared the client's flag while editorDrafts still held the draft. The tab strip then closed the tab with no unsaved-changes prompt and closeFile deleted the draft; the external-change reload guards would reload over it too. Keep the client's flag when the client's file is dirty and it holds a draft; with no draft the host's flag still wins so a host-side save does not strand the tab as dirty. A host-side save never clears a client draft. Fixes #21392
|
This run was cancelled 🛑 The workflow was cancelled before completion. Please check the link below for details. |
📝 WalkthroughWalkthroughThe change adds Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to The new regression test may not run, leaving the draft-preserving close behavior unprotected. Hoist the mock initialization before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: f94c438e-c1bc-4880-9709-8bb2e9a53ed9
📒 Files selected for processing (5)
src/renderer/src/runtime/web-session-tabs-sync-editor-tabs.test.tssrc/renderer/src/runtime/web-session-tabs-sync-mirrored-draft-close.test.tssrc/renderer/src/runtime/web-session-tabs-sync/apply-preparation-browser.tssrc/renderer/src/runtime/web-session-tabs-sync/state.tssrc/renderer/src/runtime/web-session-tabs-sync/tab-builders.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| const closeWebRuntimeSessionTabMock = vi.fn(async (_args: unknown) => 'applied' as const) | ||
|
|
||
| vi.mock('./web-runtime-session', () => ({ | ||
| closeWebRuntimeSessionTab: (args: unknown) => closeWebRuntimeSessionTabMock(args) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,155p' src/renderer/src/runtime/web-session-tabs-sync-mirrored-draft-close.test.ts
rg -n '"test:.*renderer|vitest|web-session-tabs-sync-mirrored-draft-close"' package.json src/renderer/package.jsonRepository: stablyai/orca
Length of output: 5237
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- package and vitest config ---'
sed -n '1,70p' package.json
fd -i 'vitest*.config.*' config . 2>/dev/null | head -20
printf '%s\n' '--- candidate local Vitest installation ---'
if [ -d node_modules/vitest ]; then
printf 'node_modules/vitest present\n'
find node_modules/vitest -maxdepth 2 -type f | head -30
else
printf 'node_modules/vitest absent\n'
fi
printf '%s\n' '--- focused test imports and bound module references ---'
rg -n --context 4 "from ['\\\"]\\./web-runtime-session['\\\"]|web-runtime-session|closeWebRuntimeSessionTab|from ['\\\"]\\.\\./store['\\\"]|from ['\\\"]\\./web-session-tabs-sync['\\\"]" src/renderer/src/runtime/web-session-tabs-sync-mirrored-draft-close.test.ts src/renderer/src/runtime/web-session-tabs-sync.ts src/renderer/src/runtime/web-runtime-session.ts 2>/dev/null
printf '%s\n' '--- test file metadata ---'
wc -l src/renderer/src/runtime/web-session-tabs-sync-mirrored-draft-close.test.tsRepository: stablyai/orca
Length of output: 13282
🌐 Web query:
site:vitest.dev/api/vi vi.mock hoisted factory vi.hoisted top-level variables evaluation order
💡 Result:
<search_synthesis>
In Vitest, the execution and evaluation order of vi.mock and vi.hoisted is designed to handle the fact that standard ES module import statements are hoisted to the top of the file and evaluated before any other code [1][2]. 1. vi.mock Hoisting and Evaluation vi.mock calls are automatically moved (hoisted) by Vitest to the top of the file [1][3]. This ensures that vi.mock is executed before all module imports [1][4]. Consequently, any factory function passed to vi.mock is also executed during this early phase, before the actual modules are imported or the rest of your file code runs [1][5]. 2. vi.hoisted Purpose and Evaluation Because vi.mock is hoisted, it cannot reference variables defined in the standard scope of your file, as those variables have not been initialized yet [1][2]. vi.hoisted is specifically designed to solve this limitation [1][3]. Code passed to vi.hoisted is also executed before module imports, allowing you to define variables that need to be accessible within a vi.mock factory [1][4]. 3. Correct Usage Pattern To use local variables within a vi.mock factory, you must define them inside vi.hoisted before the vi.mock call [1][5]. Vitest executes these in the order they appear in the file (while still remaining hoisted above imports): // Variables here are accessible in the mock factory const { myMockValue } = vi.hoisted( => ({ myMockValue: 'some-value' })); vi.mock('./my-module', => ({ getValue: => myMockValue, })); By following this pattern, the vi.hoisted factory runs, providing the necessary values, followed by the vi.mock factory, which can then successfully reference those values before the import statements are evaluated [1][3][2].
</search_synthesis>
<source_evidence>
Citations:
- 1: https://vitest.dev/api/vi
- 2: https://v0.vitest.dev/api/vi
- 3: https://main.vitest.dev/api/vi
- 4: https://v3.vitest.dev/api/vi
- 5: https://v2.vitest.dev/api/vi
Create the mock with vi.hoisted.
Vitest hoists vi.mock and evaluates its factory before normal top-level initialization. The factory can therefore access closeWebRuntimeSessionTabMock while it is still in the temporal dead zone and fail during module loading.
Proposed fix
-const closeWebRuntimeSessionTabMock = vi.fn(async (_args: unknown) => 'applied' as const)
+const { closeWebRuntimeSessionTabMock } = vi.hoisted(() => ({
+ closeWebRuntimeSessionTabMock: vi.fn(async (_args: unknown) => 'applied' as const)
+}))
vi.mock('./web-runtime-session', () => ({
closeWebRuntimeSessionTab: (args: unknown) => closeWebRuntimeSessionTabMock(args)
}))
ELI5
If you edit a file that Orca mirrors from a remote host and the host then republishes its tab list, the client used to forget that your tab had unsaved edits. Closing it afterwards skipped the "save or discard?" prompt and threw your edits away. Now a client draft keeps its tab marked dirty until you save or discard it yourself.
What Changed
buildMirroredEditorTabs(web-session-tabs-sync/tab-builders.ts): a rebuilt mirrored file stays dirty when the client's existing file is dirty and the client holds a draft for it (isDirty: tab.isDirty || (existingFile.isDirty && hasLocalDraft(fileId))). With no client draft the host's flag still wins, so a host-side save clears the tab as before.WebSessionTabsSyncStategains optionaleditorDrafts;apply-preparation-browser.tspasses the draft predicate. Production callers already pass the full store state, so drafts are always available there.isDirty: falserepublish; no-draft follows a host save; a lingering draft on a clean tab does not invent dirtiness) and one end-to-end case that applies a host republish to the real store and then runs the tab strip's close command: the close must be routed to the unsaved-changes prompt with the file and draft intact.Why
The host publishes only its own store's
isDirtyand never learns about client edits (client→host editor RPCs are onlymarkdown.readTab/markdown.saveTab).buildMirroredEditorTabsspread the existing local file and then overwroteisDirtyfrom the snapshot;sameOpenFilescounts that as a real diff,apply-preparation-browserapplies it unconditionally, andeditorDraftsis never touched. So a client draft could outlive its own dirty flag, and every consumer of the flag then misbehaved: the tab strip's close gates (getNextQueuedEditorClose,closeEditorIfUnreferenced) closed with no prompt andcloseFiledeleted the draft, and the external-change reload guards (useEditorPanelExternalContentEvents,useEditorPanelContentReloadTriggers) would reload over it.Two design questions, settled:
editorDraftswould protect one consumer and leave the reload guards destroying the draft; it would also mis-prompt on "typed then undid", where a draft lingers on a clean tab. Restoring the flag's truth at the single point where it is clobbered protects all consumers at once. A draft is used as evidence of why the local flag is authoritative rather than as the gate itself.tab.isDirtyto false but the client tab stays dirty while its draft exists. Dirtiness is not made monotonic: a dirty flag with no draft (it came from an earlier host snapshot) keeps following the host, so a host save does not strand the tab.Linked Issue
Fixes #21392 (found while reviewing #21375)
Visual Proof
N/A — state reconciliation; no UI change.
Testing
pnpm test src/renderer/src/runtime/web-session-tabs-sync-editor-tabs.test.ts src/renderer/src/runtime/web-session-tabs-sync-mirrored-draft-close.test.ts— 9 passedpnpm tc:web— clean;oxlint,oxfmt --check,audit:anti-slop— clean; changed-code quality gate vsorigin/main— passedisDirty: tab.isDirty): exactly the two positive tests fail — the sync-layer "keeps a client-dirty mirrored file dirty" case and the end-to-end close case (close request never dispatched; file and draft gone). The no-draft and reverted-draft cases still pass, as they should.AI Disclosure
Review
Self-reviewed for correctness (both directions of the flag), data safety (no path here closes a tab or touches
editorDrafts), SSH/remote behavior (host-owned flag still authoritative absent client evidence), and cross-platform impact (renderer only). No wire change.Agent skill upstream boundary
Notes
The pre-existing debounce window between
setEditorDraftandmarkFileDirty(noted inclose-file-action.ts) is unchanged and not mirrored-specific.Checklist