Repository navigation
feat(desktop): merge external changes into unsaved edits like a pull #1464
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,8 @@ import { | |
| errorMessage, | ||
| isAppError, | ||
| upsertFrontmatter, | ||
| resolveConflictMarkers, | ||
| type MergeTextOutcome, | ||
| } from '@reflect/core' | ||
| import { splitDoc } from './note-session-doc.ts' | ||
| import { frontmatterPatchToYaml, type FrontmatterPatch } from './note-session-frontmatter.ts' | ||
|
|
@@ -32,6 +34,7 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { | |
| let dirty = false | ||
| let missing = false | ||
| let conflict: string | null = null | ||
| let mergedPreview: string | null = null | ||
| let error: string | null = null | ||
|
|
||
| // Pipeline state (never surfaces). | ||
|
|
@@ -79,6 +82,7 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { | |
| dirty, | ||
| missing, | ||
| conflict, | ||
| mergedPreview, | ||
| error, | ||
| } | ||
| if ( | ||
|
|
@@ -89,6 +93,7 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { | |
| lastEmitted.dirty === next.dirty && | ||
| lastEmitted.missing === next.missing && | ||
| lastEmitted.conflict === next.conflict && | ||
| lastEmitted.mergedPreview === next.mergedPreview && | ||
| lastEmitted.error === next.error | ||
| ) { | ||
| return | ||
|
|
@@ -182,6 +187,9 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { | |
| } | ||
| buffer = markdown | ||
| dirty = header + markdown !== disk | ||
| // A parked preview was merged from the buffer as it was; later typing | ||
| // would be lost under Keep both or Review, so those options go away. | ||
| mergedPreview = null | ||
| if (missing && markdown.trim() === '') { | ||
| // A still-unwritten note cleared back to nothing (e.g. the seeded | ||
| // empty-title template deleted wholesale) stays unwritten: creating an | ||
|
|
@@ -261,17 +269,66 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { | |
| return | ||
| } | ||
| if (dirty) { | ||
| await mergeExternal(content) | ||
| return | ||
| } | ||
| adoptCleanContent(content) | ||
| } | ||
|
|
||
| /** | ||
| * External content arrived while the buffer has unsaved edits. Merge the | ||
| * two three-way over the last content read from disk: disjoint edits (a | ||
| * script appending to the daily note while the user types elsewhere, a | ||
| * folded bullet) apply silently and keep saving; overlapping edits park | ||
| * with the marked merge as a preview. Without a merge capability, or when | ||
| * a side already carries markers, the change parks as before. Never | ||
| * clobber unsaved edits: nothing is written here, and the save pipeline | ||
| * pauses while a conflict is parked. | ||
| */ | ||
| async function mergeExternal(content: string): Promise<void> { | ||
| const ours = header + buffer | ||
|
Collaborator
Author
There was a problem hiding this comment. Done in f15f848: |
||
| let merged: MergeTextOutcome | null = null | ||
| if (io.mergeText !== undefined) { | ||
| try { | ||
| merged = await io.mergeText(path, disk, ours, content) | ||
| } catch (cause) { | ||
| console.error('three-way merge failed; parking the conflict:', cause) | ||
| } | ||
|
Comment on lines
+339
to
+343
Collaborator
Author
There was a problem hiding this comment. Done in f15f848: "a merge that throws keeps the edits beside the note before adopting the external version" scripts |
||
| } | ||
| if (disposed) { | ||
| return | ||
| } | ||
| // Typing continued while the merge ran: its result no longer covers the | ||
| // buffer. Park without it instead of applying a stale merge over newer | ||
| // keystrokes. | ||
| const current = header + buffer === ours | ||
| if (merged?.kind === 'clean' && current) { | ||
| adoptMerged(merged.content, content) | ||
| return | ||
| } | ||
| cancelScheduledSave() | ||
| conflict = content | ||
| mergedPreview = merged?.kind === 'conflicted' && current ? merged.content : null | ||
| emit() | ||
| } | ||
|
|
||
| /** Put `merged` in the editor as the dirty buffer over `onDisk`, and keep saving. */ | ||
| function adoptMerged(merged: string, onDisk: string): void { | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| const doc = splitDoc(merged) | ||
| header = doc.header | ||
| buffer = doc.body | ||
| disk = onDisk | ||
| conflict = null | ||
| mergedPreview = null | ||
| dirty = merged !== onDisk | ||
| missing = false | ||
| emit() | ||
| applyToEditor(doc.body) | ||
| if (dirty) { | ||
| scheduleSave() | ||
|
Collaborator
Author
There was a problem hiding this comment. Reviewed by GPT-6. [P1] Make flush await the clean merge's write, not its debounce timer If another device changes the file just before Cmd-Q, the exit-facing Please make the persistence boundary drain reconciliation and its resulting checked write before resolving, including teardown, without waiting recursively on the same save chain. Add a regression with stale disk plus a clean merge: awaiting
Collaborator
Author
There was a problem hiding this comment. Done in b9e1164. Two changes: |
||
| } | ||
| } | ||
|
|
||
| /** The initial read; with `createIfMissing`, a missing file is an empty note. */ | ||
| async function readInitial(): Promise<{ content: string; fileMissing: boolean }> { | ||
| try { | ||
|
|
@@ -356,6 +413,7 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { | |
| missing = false | ||
| } | ||
| conflict = null | ||
| mergedPreview = null | ||
| dirty = true // force the rewrite even if content drifted equal | ||
| emit() | ||
| save() | ||
|
|
@@ -367,11 +425,54 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { | |
| } | ||
| const content = conflict | ||
| conflict = null | ||
| mergedPreview = null | ||
| // Same re-gating as the clean-reload path: never load lossy content into a | ||
| // live editor whose next save would drop what it can't model. | ||
| adoptCleanContent(content) | ||
| } | ||
|
|
||
| function keepBoth(): void { | ||
| if (conflict === null || mergedPreview === null) { | ||
| return | ||
| } | ||
| adoptMerged(resolveConflictMarkers(mergedPreview, 'both'), conflict) | ||
| } | ||
|
|
||
| function review(): void { | ||
| if (conflict === null || mergedPreview === null || io.write === null) { | ||
| return | ||
| } | ||
| // The marked merge becomes the file, exactly as a Git pull leaves a | ||
| // conflicted note, and opens protected: the notice resolves it block by | ||
| // block, and every side stays recoverable on disk meanwhile. The write | ||
| // expects the external version this merge was made from; a newer one on | ||
| // disk means the preview is stale, so the conflict stays parked and is | ||
| // reconciled afresh. The parked state is only cleared once the write | ||
| // lands, and the chain settles either way. | ||
| const marked = mergedPreview | ||
| const onDisk = conflict | ||
| const write = io.write | ||
| saveChain = saveChain.then(async () => { | ||
|
Collaborator
Author
There was a problem hiding this comment. [P2] Recover the save chain when the review write fails GPT-6, reviewed SHA 152c16b. When the Review write rejects (for example disk full), conflict and mergedPreview have already been cleared, and this promise has no catch. The session gets neither its normal error state nor a retryable conflict, and saveChain remains rejected. On the next edit, save() appends a then to that rejected chain, so its write is skipped; the catch only reconciles and does not retry the queued write. Keep the conflict until the write succeeds, surface failures, and restore a settled save chain so an ordinary retry can save. Add a failed-review-write regression followed by a successful retry.
Collaborator
Author
There was a problem hiding this comment. Done in 8f5c9f0: the parked state is cleared only after the write lands. On failure the session records |
||
| try { | ||
| await write(path, marked, onDisk) | ||
| } catch (cause) { | ||
| if (disposed) { | ||
| return | ||
| } | ||
| error = errorMessage(cause) | ||
| emit() | ||
| await reconcileFromDisk() | ||
| return | ||
| } | ||
| if (!disposed) { | ||
| conflict = null | ||
| mergedPreview = null | ||
| error = null | ||
| adoptCleanContent(marked) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| function updateFrontmatter(patch: FrontmatterPatch): boolean { | ||
| if (disposed || isProtected || status !== 'ready') { | ||
| return false | ||
|
|
@@ -548,6 +649,8 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { | |
| flush, | ||
| keepMine, | ||
| loadTheirs, | ||
| keepBoth, | ||
| review, | ||
| content: () => header + buffer, | ||
| liveContent: () => (status === 'ready' ? header + buffer : null), | ||
| isDirty: () => dirty, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] (anchored here; the early return is at line 253 in
reconcileFromDisk)** A final flush refused by CAS drops the buffer: no write, no copy, no error.**
document-binding.ts:84-93callstarget.flush()and thentarget.dispose()synchronously, sodisposed === truebefore the write runs. Sequence: the user edits note A, another device or a pull writes A before the 800 ms debounce fires, the user navigates away.save()fails CAS → catch at 132 →reconcileFromDisk→ reads the new content → returns here. No merge, nokeepAside, no emit, andflush()resolves. Reproduced with a probe test (edit, mutate disk,dispose(), advance timers):writes: [],copies: [], disk unchanged.Master had the same hole but parked the buffer. This PR and
docs/git-backup-safety.mdS8 claim “the buffer is never left in a state it cannot save from, so no exit path has to rescue it”, and #1451 was closed on that claim, so this needs to hold.Fix: let a reconciliation that a dispose-flush started run to completion. Make
disposedsuppress onlyemit()/applyToEditor, notreconcileFromDisk/mergeExternal/materialize/keepAside(for example afinalizingflag set bydispose()that these guards accept). Regression: “a dispose flush refused by an external change still merges or copies the buffer”.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed, and it had to hold. Fixed in b1e6613 without a new flag: the
disposedearly returns inreconcileFromDisk,mergeExternal, andmaterializeare gone, so a reconciliation that the final flush's refused save started runs to completion (merge and save, exact write, or a copy beside the note). Whatdisposedstill suppresses is the UI side only:emit()already ignored it, andapplyToEditornow does too, since the editor belongs to the next session by then. Two guards replace the old ones: a discarded session (its file is being deleted) still stops at every await, and a disposed session with nothing dirty returns after the read, because it has nothing to rescue.Tests:
a dispose flush refused by an external change still merges the buffer(flush, dispose in the same tick, disk mutated before; the merged content is written) anda dispose flush refused with no merge available keeps the buffer beside the note.docs/git-backup-safety.mdS8 names the case and the test.