fix(desktop): flush pending edits before saving (dropped last keystroke)#4859
Conversation
… isn't lost (#3803) The engine commits edits to the store's currentFile.markdown on a deferred requestAnimationFrame (json-change). FILE_SAVE / FILE_SAVE_AS snapshotted currentFile.markdown and shipped it to the main process without flushing first, so a keystroke typed in the same frame as Cmd+S (or Save As) — a window that widens under main-thread load on large docs — was dropped from the written file, with no dirty indicator. Tab switching already guards against this by emitting 'flush-active-editor' (the #2938 fix), but the save paths did not. Emit 'flush-active-editor' at the start of FILE_SAVE and FILE_SAVE_AS; the editor flushes the queued op into currentFile synchronously before its markdown is read. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Build artifacts for PR #4859: Run: https://github.com/marktext/marktext/actions/runs/28730589380
|
…paths (#3803) Follow-up cleanup: the flush-before-read was a copy-pasted bus.emit at two call sites (and a third pre-existing copy in UPDATE_CURRENT_FILE). Extract a single flushActiveEditor() action and route all of them through it. Also apply it to MOVE_FILE_TO and RESPONSE_FOR_RENAME, which read currentFile.markdown and persist it via the exact same pattern as FILE_SAVE — so "move to" / renaming an untitled file no longer drops a same-frame keystroke either. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jocs
left a comment
Jocs
left a comment
There was a problem hiding this comment.
Reviewed the mechanism end-to-end — the fix is correct. flush() cancels the rAF and synchronously applies the queued op → synchronous json-change (mitt) → LISTEN_FOR_CONTENT_CHANGE sets tab.markdown, and since currentFile is the same object reference as the tab, the destructure that follows reads the freshly-committed markdown. No ordering / re-entrancy / double-save issue, and source-code mode is safe for an independent reason (it commits synchronously per keystroke via cursorActivity — its commitTimer is declared but never armed, so there is no debounce).
No correctness bug. A few test-strength / cleanup notes inline.
One cross-cutting note on the spec: with no editor.vue / listener mounted, bus.emit('flush-active-editor') fires into the void, so in the test the flush is a no-op and the seeded markdown: 'hello' is sent regardless of the fix. The test therefore locks the store's emit-ordering contract but does not exercise the actual "last keystroke captured before save" behavior — that end-to-end guarantee rests entirely on #2938's flushPendingOps.spec.ts. Acceptable given the PR says as much, just flagging that the headline behavior has no direct coverage here.
| const saveCall = sendSpy.mock.calls.find((c) => c[0] === 'mt::response-file-save') | ||
| expect(saveCall).toBeTruthy() | ||
| // The flush must happen before the save payload is sent. | ||
| expect(emitSpy.mock.invocationCallOrder[0]).toBeLessThan(sendSpy.mock.invocationCallOrder[0]) |
There was a problem hiding this comment.
This ordering assertion guards the wrong boundary (same at :86 and :103). It asserts flush emit < IPC send, but the #3803 bug is captured at the const { …, markdown } = this.currentFile read, which sits between the flush and the send. A regression that moved the flush to after the markdown snapshot but before the send would still pass this test while re-introducing the dropped-keystroke bug. Consider anchoring the assertion to the read instead — or, stronger, wire a flush-active-editor listener that mutates currentFile.markdown and assert the sent payload reflects it.
There was a problem hiding this comment.
Fixed in 664449d. The spec now wires a real flush-active-editor listener that commits the pending keystroke into currentFile.markdown (mirroring editor.vue), and asserts the sent IPC payload carries it (markdown arg === the flushed value) for FILE_SAVE / FILE_SAVE_AS / MOVE_FILE_TO / RESPONSE_FOR_RENAME. Verified it is anchored at the read: temporarily moving flushActiveEditor() below the const { …, markdown } = this.currentFile destructure turns the FILE_SAVE payload test RED (it then sends the stale snapshot).
| expect(emitSpy).toHaveBeenCalledWith('flush-active-editor') | ||
| const saveCall = sendSpy.mock.calls.find((c) => c[0] === 'mt::response-file-save') | ||
| expect(saveCall).toBeTruthy() | ||
| // The flush must happen before the save payload is sent. | ||
| expect(emitSpy.mock.invocationCallOrder[0]).toBeLessThan(sendSpy.mock.invocationCallOrder[0]) |
There was a problem hiding this comment.
Minor: invocationCallOrder[0] assumes the flush is the first emit of any kind. It holds today only because flushActiveEditor() is the first line of each action; if an earlier bus.emit is ever added, [0] silently points at an unrelated emit and the assertion becomes meaningless while staying green. Locating the flush call's own index via emitSpy.mock.calls would pin it robustly. (Also: the MOVE_FILE_TO case asserts only sendSpy).toHaveBeenCalled() rather than filtering the exact channel like the FILE_SAVE cases.)
There was a problem hiding this comment.
Fixed. Dropped the invocationCallOrder[0] ordering assertions in favor of payload-content checks. The one remaining order check (RESPONSE_FOR_RENAME rename branch) locates each emit by event name via emitSpy.mock.calls.findIndex(c => c[0] === event), not a positional [0]. The MOVE_FILE_TO test now filters the exact mt::response-file-save channel instead of a bare toHaveBeenCalled().
|
|
||
| RESPONSE_FOR_RENAME(): void { | ||
| if (!this.currentFile) return | ||
| this.flushActiveEditor() |
There was a problem hiding this comment.
RESPONSE_FOR_RENAME gets the flush but has no test — the spec covers FILE_SAVE / FILE_SAVE_AS / MOVE_FILE_TO only. Since this path takes the else branch (bus.emit('rename')) when pathname is set, it needs a flush emit < 'rename' emit assertion rather than the sendSpy shape. Removing this flush later would go undetected.
There was a problem hiding this comment.
Fixed. Added two RESPONSE_FOR_RENAME tests: the untitled branch (pathname: "") asserts the flushed markdown reaches the mt::response-file-save payload, and the existing-file branch asserts the flush is emitted before "rename" — so silently removing the flush would fail the suite.
|
|
||
| MOVE_FILE_TO(): void { | ||
| if (!this.currentFile) return | ||
| this.flushActiveEditor() |
There was a problem hiding this comment.
Cleanup nit (also applies to RESPONSE_FOR_RENAME at :762): in the pathname branch this action sends { id, pathname } (and RESPONSE_FOR_RENAME emits 'rename') and never transmits markdown, so flushing there commits nothing useful — the flush only matters in each action's untitled/no-pathname branch, which does send markdown. Harmless and arguably simpler than branching the flush, so fine to leave as-is; noting it so it's a conscious choice.
There was a problem hiding this comment.
Left as-is per your note — the flush stays at the top of MOVE_FILE_TO / RESPONSE_FOR_RENAME (a harmless no-op in the pathname branch, simpler than duplicating it into each untitled branch). Both are now covered: the payload assertion exercises the untitled branch where markdown is actually sent, and the flush-before-rename ordering guards the kept flush in the pathname branch.
…3803 review) The prior spec only asserted flush-emit < IPC-send ordering, but the #3803 bug lives at the `const { …, markdown } = this.currentFile` read that sits between them — a flush moved after that read would still pass. Wire a real `flush-active-editor` listener that commits the pending keystroke (mirroring editor.vue) and assert the SENT PAYLOAD carries it, so a regression past the read boundary is caught. Also: cover RESPONSE_FOR_RENAME (both the untitled payload path and the existing-file rename-emit path), filter the exact IPC channel in the MOVE_FILE_TO case, and anchor ordering by event name instead of the fragile invocationCallOrder[0]. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Problem
Fixes #3803.
Reports of "lost content despite saving" — most dramatically the tab-switch corruption already fixed in #2938 (PR #4658), but a narrower path survives that fix: the save itself can drop the last keystroke.
The engine commits edits into the renderer store's
currentFile.markdownon a deferredrequestAnimationFrame(json-change).FILE_SAVE/FILE_SAVE_AS(packages/desktop/src/renderer/src/store/editor.ts) readcurrentFile.markdownand send it to the main process without flushing that pending op first. So a keystroke typed in the same frame asCmd+S(or Save As) is written to disk as the previous content — with no dirty indicator, matching the "a trailing character went missing" reports. The window is ~16 ms normally, but widens under main-thread congestion (large docs, spellcheck, diagram re-render).Tab switching already handles this:
UPDATE_CURRENT_FILEemitsbus.emit('flush-active-editor')before reading, which the editor flushes intocurrentFilesynchronously (the #2938 mechanism). The save paths just never did.Fix
Emit
flush-active-editorat the start ofFILE_SAVEandFILE_SAVE_AS, before snapshottingcurrentFile.markdown. This reuses the existing, synchronous flush wiring (editor.vueflushActiveEditor→editor.flush()→json-change→LISTEN_FOR_CONTENT_CHANGE), so the latest keystroke is captured in the saved file.Tests
test/unit/specs/flush-before-save.spec.ts(Pinia store, modeled oneditor-store-anchor.spec.ts): assertsFILE_SAVEandFILE_SAVE_ASeach emitflush-active-editorbefore sending their save IPC (mt::response-file-save/-save-as). Verified RED ondevelop(no flush emitted) → GREEN with the fix.typecheckandeslintpass.The end-to-end no-lost-keystroke behavior depends on the already-tested flush mechanism (#2938's
flushPendingOps.spec.ts+ theflush-active-editorwiring); this PR adds the missing emit on the save paths and guards it.