{{ message }}
Repository navigation
fix(desktop): Edit▸Replace action + saved-indicator false-clean (Phase G) - #4431
Merged
Merged
Conversation
`editorReplace` emitted `edit(win, 'undo')`, so the Edit▸Replace menu item and its accelerator (Cmd+Opt+F / Ctrl+R) performed an UNDO instead of opening the replace bar. The renderer re-emits the IPC action verbatim on the bus, so 'undo' invoked `handleUndo` and `listenReplace` (bound to the 'replace' bus event) was never reached from the menu/shortcut path. Send 'replace' so the search component opens in replace mode, matching the command-palette `edit.replace` entry (Phase G — G3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes two desktop-only regressions in packages/desktop: (1) Edit ▸ Replace incorrectly triggering Undo, and (2) the saved/dirty indicator becoming falsely “clean” in specific undo + divergent re-edit scenarios (data-loss risk), by switching desktop save-tracking to a content-keyed synthetic history id.
Changes:
- Fix
editorReplaceto emit the correct'replace'edit action instead of'undo'. - Introduce
SyntheticHistoryto allocate monotonic save-tracking ids keyed on document content (with newline normalization) and wire it into the WYSIWYG editor/store bridge. - Add/adjust unit + e2e coverage for the G6 false-clean scenario and for PG15 saved-indicator behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+42
to
+56
| // A fast, stable 32-bit string hash (FNV-1a) over the trailing-newline-normalized | ||
| // content. Used so the content -> id map stores short keys instead of whole | ||
| // documents; collisions only risk a false-clean for two genuinely different | ||
| // documents that hash identically, which is astronomically unlikely for FNV-1a | ||
| // over editor content. | ||
| const hashContent = (content: string): number => { | ||
| const normalized = stripTrailingNewlines(content) | ||
| let hash = 0x811c9dc5 | ||
| for (let i = 0; i < normalized.length; i++) { | ||
| hash ^= normalized.charCodeAt(i) | ||
| hash = Math.imul(hash, 0x01000193) | ||
| } | ||
| // Coerce to an unsigned 32-bit integer for a stable numeric key. | ||
| return hash >>> 0 | ||
| } |
Member
Author
There was a problem hiding this comment.
Good catch — switched the content key to a 64-bit FNV-1a (BigInt key). At 32 bits the birthday bound puts ~50% collision odds near ~77k snapshots, realistic over a long session, and a collision would reintroduce the false-clean. 64 bits makes it negligible. Added a unit test allocating 5000 distinct snapshots and asserting 5000 distinct ids.
Comment on lines
+286
to
294
| const syntheticHistoryByTab = new Map<string, SyntheticHistory>() | ||
| const getSyntheticHistory = (id: string, baselineContent: string): SyntheticHistory => { | ||
| let tracker = syntheticHistoryByTab.get(id) | ||
| if (!tracker) { | ||
| tracker = new SyntheticHistory(baselineContent) | ||
| syntheticHistoryByTab.set(id, tracker) | ||
| } | ||
| return tracker | ||
| } |
Member
Author
There was a problem hiding this comment.
Fixed — added a watcher on the store tab-id set that prunes BOTH syntheticHistoryByTab and the pre-existing engineHistoryByTab when tabs close, so neither map (nor the per-tab content→id map) grows unbounded over a session.
…tory id The saved/clean indicator (PG15) could falsely show "clean" for a dirty document, risking data loss on close-without-save. The renderer fed the store a SYNTHETIC, desktop-shaped history whose entry id was the engine undo-stack DEPTH. Depth is reused across distinct documents at the same stack height, so the sequence type A, type B, save, undo B, type C (a NEW divergent edit) returned the depth — and therefore the id — to the saved value while the document was actually A+C, not the saved A+B. The store compares `editEntry.id !== lastSavedHistoryId`, so the tab read as clean and could be closed without a save prompt. Replace the depth-as-id scheme with a MONOTONIC, never-reused id keyed on the live document content (new `SyntheticHistory` allocator): each distinct content snapshot gets a fresh id the first time it is seen and reuses that id on every revisit. Undoing back to the saved content restores the saved id (clean); a divergent re-edit produces brand-new content and hence a brand-new id (dirty) — never the saved one. This matches legacy muyajs' monotonic, never-reused history ids while preserving the undo-to-saved clean case. The allocator is re-baselined to the loaded content (id 0) on `file-loaded`, matching the store's seeded `lastSavedHistoryId: 0`. The content signature ignores trailing newlines: the engine's markdown serialization is unstable across a `setContent` -> edit -> undo round-trip purely in trailing newlines (loading `'x\n'` may serialize to `'x\n\n\n'` while undoing an edit lands on `'x\n'`), and the store itself normalizes trailing newlines via `adjustTrailingNewlines`. Without this, undo-to-saved would never re-match the baseline. The content key uses a 64-bit FNV-1a hash (a 32-bit hash hits ~50% collision odds near ~77k snapshots via the birthday bound — realistic over a long session — and a collision would reintroduce the false-clean it guards against). The per-tab engine/synthetic history maps are pruned when tabs close (watcher on the store's tab id set) so they don't grow unbounded over a long session. Add unit coverage for the id/dirty logic (the exact G6 scenario, undo-to-saved, redo-to-saved, unsaved-baseline, trailing-newline insensitivity, and distinctness across many snapshots) and an e2e regression test that drives the full undo + divergent-re-edit-at-saved-depth flow and asserts the tab stays dirty (verified to fail against the old depth-as-id scheme) (Phase G — G6). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jocs
force-pushed
the
fix/desktop-replace-and-saved-indicator
branch
from
June 9, 2026 23:39
89faea0 to
1b21492
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| let hash = FNV64_OFFSET | ||
| for (let i = 0; i < normalized.length; i++) { | ||
| hash ^= BigInt(normalized.charCodeAt(i)) | ||
| hash = (hash * FNV64_PRIME) & MASK64 |
| let hash = FNV64_OFFSET | ||
| for (let i = 0; i < normalized.length; i++) { | ||
| hash ^= BigInt(normalized.charCodeAt(i)) | ||
| hash = (hash * FNV64_PRIME) & MASK64 |
Contributor
|
Build artifacts for PR #4431: Run: https://github.com/marktext/marktext/actions/runs/27242945602 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two desktop-only bugs found by the Phase G audit (
packages/desktop). Two single-responsibility commits.G3 (major) — Edit▸Replace performed an UNDO
editorReplace(src/main/menu/actions/edit.ts) emittededit(win, 'undo'), so the Edit▸Replace menu item and its accelerator (Cmd+Opt+F / Ctrl+R) performed an UNDO instead of opening the replace bar. The renderer re-emits the IPC action verbatim on the bus, so'undo'invokedhandleUndoandlistenReplace(bound to the'replace'bus event) was never reached from the menu/shortcut path.Fix: send
'replace'so the search component opens in replace mode (showSearch=true,type='replace'), matching the command-paletteedit.replaceentry. Verified end-to-end: IPC'replace'→EDITOR_EDIT_ACTION→bus.emit('replace','replace')→listenReplace(). Covered by the existingfind-replace.spec.ts"Replace action shows the search bar in replace mode".G6 (major, data-loss risk) — saved indicator false-clean
The renderer fed the store a synthetic, desktop-shaped history whose entry id was the engine undo-stack DEPTH. Depth is reused across distinct documents at the same stack height, so:
returned the depth — and therefore the id — to the saved value while the document was actually A+C, not the saved A+B. The store's PG15 check is
editEntry.id !== lastSavedHistoryId, so the tab read as clean and could be closed without a save prompt → silent data loss.Fix: replace the depth-as-id scheme with a monotonic, never-reused id keyed on the live document content (new
SyntheticHistoryallocator). Each distinct content snapshot gets a fresh id the first time it is seen and reuses that id on every revisit. Undoing back to the saved content restores the saved id (clean); a divergent re-edit produces brand-new content and hence a brand-new id (dirty) — never the saved one. This matches legacy muyajs' monotonic, never-reused history ids while preserving the undo-to-saved clean case. The allocator is re-baselined to the loaded content (id 0) onfile-loaded, matching the store's seededlastSavedHistoryId: 0.The content signature ignores trailing newlines, because the engine's markdown serialization is unstable across a
setContent → edit → undoround-trip purely in trailing newlines (loading'x\n'may serialize to'x\n\n\n'while undoing an edit lands on'x\n'), and the store already normalizes trailing newlines viaadjustTrailingNewlines.Tests
test/unit/specs/synthetic-history.spec.ts(9 cases): the exact G6 scenario, undo-to-saved, redo-to-saved, unsaved-baseline, trailing-newline insensitivity.G6: a divergent re-edit at the saved undo depth stays dirtyinparity-source-undo-saved.spec.ts, driving the full undo + divergent-re-edit-at-saved-depth flow and asserting the tab stays dirty. Verified to fail against the old depth-as-id scheme and pass with the fix.expect.poll.Verification
pnpm run typecheck,pnpm run lint(0 errors),pnpm run test(464 pass),pnpm run build:unpack, and the relevant e2e (find-replace,parity-source-undo-saved) all green.Refs: Phase G audit G3, G6.
🤖 Generated with Claude Code