diff --git a/apps/desktop/src/components/note-conflict-banner.test.tsx b/apps/desktop/src/components/note-conflict-banner.test.tsx deleted file mode 100644 index 601b02977..000000000 --- a/apps/desktop/src/components/note-conflict-banner.test.tsx +++ /dev/null @@ -1,41 +0,0 @@ -import { render } from 'vitest-browser-react' -import { userEvent } from 'vitest/browser' -import { describe, expect, it, vi } from 'vitest' -import { NoteConflictBanner } from './note-conflict-banner.tsx' - -async function renderBanner() { - const onKeepMine = vi.fn() - const onLoadTheirs = vi.fn() - const view = await render( - , - ) - return { view, onKeepMine, onLoadTheirs } -} - -describe('NoteConflictBanner', () => { - it('explains the conflict and offers both resolutions', async () => { - const { view } = await renderBanner() - expect(view.getByRole('alert').element().textContent).toContain( - 'This note changed on disk while you had unsaved edits.', - ) - await expect.element(view.getByRole('button', { name: 'Keep mine' })).toBeInTheDocument() - await expect.element(view.getByRole('button', { name: 'Load theirs' })).toBeInTheDocument() - await view.unmount() - }) - - it('fires onKeepMine when keeping the editor buffer', async () => { - const { view, onKeepMine, onLoadTheirs } = await renderBanner() - await userEvent.click(view.getByRole('button', { name: 'Keep mine' })) - expect(onKeepMine).toHaveBeenCalledOnce() - expect(onLoadTheirs).not.toHaveBeenCalled() - await view.unmount() - }) - - it('fires onLoadTheirs when loading the external content', async () => { - const { view, onKeepMine, onLoadTheirs } = await renderBanner() - await userEvent.click(view.getByRole('button', { name: 'Load theirs' })) - expect(onLoadTheirs).toHaveBeenCalledOnce() - expect(onKeepMine).not.toHaveBeenCalled() - await view.unmount() - }) -}) diff --git a/apps/desktop/src/components/note-conflict-banner.tsx b/apps/desktop/src/components/note-conflict-banner.tsx deleted file mode 100644 index 2efbea38f..000000000 --- a/apps/desktop/src/components/note-conflict-banner.tsx +++ /dev/null @@ -1,35 +0,0 @@ -import type { ReactElement } from 'react' -import { InlineAlert } from '@/components/inline-alert.tsx' -import { Button } from '@/components/ui/button.tsx' - -interface NoteConflictBannerProps { - /** Resolve by keeping the editor buffer (rewrites the file). */ - onKeepMine: () => void - /** Resolve by loading the external content (discards the buffer). */ - onLoadTheirs: () => void -} - -/** - * The non-destructive conflict prompt (Plan 05): an external change raced - * unsaved edits, saves are paused, and nothing is written until the user - * picks a side. The two actions map 1:1 onto the note session's - * `keepMine`/`loadTheirs`. - */ -export function NoteConflictBanner({ - onKeepMine, - onLoadTheirs, -}: NoteConflictBannerProps): ReactElement { - return ( - - This note changed on disk while you had unsaved edits. -
- - -
-
- ) -} diff --git a/apps/desktop/src/components/note-save-alerts.tsx b/apps/desktop/src/components/note-save-alerts.tsx index d0aad827d..532faeda7 100644 --- a/apps/desktop/src/components/note-save-alerts.tsx +++ b/apps/desktop/src/components/note-save-alerts.tsx @@ -1,6 +1,5 @@ import type { ReactElement } from 'react' import { InlineAlert } from '@/components/inline-alert.tsx' -import { NoteConflictBanner } from '@/components/note-conflict-banner.tsx' import type { AssetSaveError } from '@/editor/use-asset-persistence.ts' import type { NoteDocument } from '@/editor/use-note-document.ts' @@ -10,7 +9,7 @@ interface NoteSaveAlertsProps { assetSaveError?: AssetSaveError | null } -/** What went wrong saving a ready document, and the external-change conflict prompt. */ +/** What went wrong saving a ready document. */ export function NoteSaveAlerts({ document, assetSaveError = null, @@ -29,9 +28,6 @@ export function NoteSaveAlerts({ {assetSaveError.message}. It was not added to the note. ) : null} - {document.conflict !== null ? ( - - ) : null} ) } diff --git a/apps/desktop/src/editor/alias-placement.ts b/apps/desktop/src/editor/alias-placement.ts index 1b15685b1..377de2095 100644 --- a/apps/desktop/src/editor/alias-placement.ts +++ b/apps/desktop/src/editor/alias-placement.ts @@ -16,8 +16,8 @@ import { openSession } from './open-documents.ts' * Placement routes through the live session whenever the note is open — in * the renaming pane or a *reopened* one (the open-documents service is the * one liveness signal). A direct disk write under a reopened dirty buffer - * would park a conflict caused by our own background work, and "keep mine" - * would silently drop the alias. Only when no session can take the patch + * would force a merge against our own background work. Only when no + * session can take the patch * does the alias go straight to disk; a loading/clean session reconciles it * like any external change, and a header-only patch is body-safe even for * protected notes. diff --git a/apps/desktop/src/editor/document-binding.test.ts b/apps/desktop/src/editor/document-binding.test.ts index 59cee584d..9a89e45db 100644 --- a/apps/desktop/src/editor/document-binding.test.ts +++ b/apps/desktop/src/editor/document-binding.test.ts @@ -27,12 +27,10 @@ function fakeSession(path: string) { editorChanged: () => {}, externalChanged: () => {}, flush, - keepMine: () => {}, isDirty: () => false, isUnpersisted: () => false, prepareDelete: async () => false, cancelDelete: () => {}, - loadTheirs: () => {}, commitFrontmatter: async () => true, content: () => '', liveContent: () => '', diff --git a/apps/desktop/src/editor/move-note.test.ts b/apps/desktop/src/editor/move-note.test.ts index 0bebdb210..e12dce84d 100644 --- a/apps/desktop/src/editor/move-note.test.ts +++ b/apps/desktop/src/editor/move-note.test.ts @@ -31,12 +31,10 @@ function fakeSession(path: string) { editorChanged: () => {}, externalChanged: () => {}, flush, - keepMine: () => {}, isDirty: () => false, isUnpersisted: () => false, prepareDelete: async () => false, cancelDelete: () => {}, - loadTheirs: () => {}, commitFrontmatter: async () => true, content: () => '', liveContent: () => '', diff --git a/apps/desktop/src/editor/note-session-state.ts b/apps/desktop/src/editor/note-session-state.ts index b1774e81a..f12f400e9 100644 --- a/apps/desktop/src/editor/note-session-state.ts +++ b/apps/desktop/src/editor/note-session-state.ts @@ -4,17 +4,22 @@ import { errorMessage, isAppError, upsertFrontmatter, + type MergeTextOutcome, } from '@reflect/core' import { splitDoc } from './note-session-doc.ts' import { frontmatterPatchToYaml, type FrontmatterPatch } from './note-session-frontmatter.ts' import type { + ConflictCopy, NoteSession, + NoteSessionIo, NoteSessionOptions, NoteSessionSnapshot, NoteSessionStatus, } from './note-session-types.ts' const DEFAULT_SAVE_DEBOUNCE_MS = 800 +/** The commit's edit went into a conflict copy, not the note. */ +const KEPT_ASIDE = 'The note changed on disk; the edit was kept beside it' /** Create the document session for one note. See note-session.ts for semantics. */ export function createNoteSession(options: NoteSessionOptions): NoteSession { @@ -31,7 +36,6 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { let isProtected = false let dirty = false let missing = false - let conflict: string | null = null let error: string | null = null // Pipeline state (never surfaces). @@ -60,6 +64,11 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { /** A watcher event arrived during the load; replay reconciliation after it. */ let missedChange = false let disposed = false + /** The one reconciliation in flight, and whether another was asked for during it. */ + let reconciling: Promise | null = null + let reconcileAgain = false + /** Buffers kept beside the note so far: a commit whose edit went there did not land. */ + let keptAside = 0 /** True while deletion has paused this session's persistence pipeline. */ let deleting = false // Set by `discard` — tells `dispose` to skip its flush (the file is being @@ -78,7 +87,6 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { protected: isProtected, dirty, missing, - conflict, error, } if ( @@ -88,7 +96,6 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { lastEmitted.protected === next.protected && lastEmitted.dirty === next.dirty && lastEmitted.missing === next.missing && - lastEmitted.conflict === next.conflict && lastEmitted.error === next.error ) { return @@ -100,11 +107,8 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { function save(): void { // A discarded session never writes: its file is being deleted, so any // save — including a teardown `flush()` (the pane unmounts via flush → - // dispose) or an already-queued step — would recreate it. A parked - // conflict likewise pauses all saves: writing the buffer before the user - // chooses Keep mine / Load theirs would clobber the external change and - // defeat the non-destructive flow. - if (discarded || deleting || io.write === null || !dirty || isProtected || conflict !== null) { + // dispose) or an already-queued step — would recreate it. + if (discarded || deleting || io.write === null || !dirty || isProtected) { return } const write = io.write @@ -115,7 +119,7 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { // have reverted or kept typing, or the session may have been discarded // for a delete. (After dispose the buffer is frozen, so this same step // doubles as the final flush.) - if (discarded || deleting || !dirty || isProtected || conflict !== null) { + if (discarded || deleting || !dirty || isProtected) { return } const content = header + buffer @@ -160,13 +164,23 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { } } - function flush(): Promise { + async function flush(): Promise { reconcilePendingEditorInput?.() cancelScheduledSave() save() // save() extended the chain synchronously (or left it settled when there - // was nothing to do) — the chain as of now is exactly this flush's write. - return saveChain + // was nothing to do). Settle the whole chain, not just that step: a + // refused write reconciles, and a clean merge appends its own write, + // which the quit flush must see land before it reports done. + // Bounded: a writer that changes the file again before every retry must + // not keep a closing window waiting forever. + for (let round = 0; round < 5; round += 1) { + const tail = saveChain + await tail + if (tail === saveChain) { + return + } + } } function editorChanged(markdown: string): void { @@ -195,8 +209,15 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { } } - /** Apply external content to the live editor without entering the save path. */ + /** + * Apply external content to the live editor without entering the save path. + * Nothing after dispose: the editor belongs to the next session by then, + * while the buffer here stays frozen for the final flush's reconciliation. + */ function applyToEditor(content: string): void { + if (disposed) { + return + } applyingContent = true try { // The editor dispatches synchronously, so its change handler runs (and is @@ -214,6 +235,7 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { buffer = doc.body disk = content dirty = false + error = null // a reconciliation that lands clears the save failure that led here missing = false // external content means the file exists on disk now // Re-gate: the content may have introduced (or removed) syntax the editor // can't round-trip. When protection flips the pane remounts via @@ -233,9 +255,30 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { /** * Re-read the note and reconcile the buffer with what's on disk (the - * external-change path). + * external-change path). One reconciliation runs at a time: the watcher + * and a refused save both lead here for the same change, and two + * interleaved merges would merge the first one's result again. A call + * during one schedules a rerun, which finds nothing left to do when the + * first one landed. Disposal does not stop it: a save the final flush + * started may be the one refused, and its buffer must still reach the + * note or a copy beside it. */ - async function reconcileFromDisk(): Promise { + function reconcileFromDisk(): Promise { + reconcileAgain = true + reconciling ??= (async () => { + try { + while (reconcileAgain) { + reconcileAgain = false + await reconcileOnce() + } + } finally { + reconciling = null + } + })() + return reconciling + } + + async function reconcileOnce(): Promise { let content: string try { content = await io.read(path) @@ -246,8 +289,8 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { } return // preserve the buffer if the file disappeared or cannot be read } - if (disposed) { - return + if (discarded || (disposed && !dirty)) { + return // being deleted, or closed with nothing left to save } if (content === disk || content === inFlightWrite) { // Nothing to reconcile (stale, or an echo of our own possibly @@ -258,20 +301,161 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { missing = false emit() } + if (!dirty && error?.includes('changed on disk') === true) { + // A save refused by a change that an earlier reconciliation has + // already taken in: nothing is pending, so nothing is failing. + error = null + emit() + } return } if (dirty) { - // Never clobber unsaved edits — park the external content and pause the - // save pipeline (cancel any pending debounce) until the user chooses; a - // save landing now would overwrite "theirs" first. - cancelScheduledSave() - conflict = content - emit() + 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, the way a Git pull + * or an iCloud sweep would: disjoint edits (a script appending to the + * daily note while the user types elsewhere, a folded bullet) apply + * silently and keep saving; overlapping edits are written into the file as + * labeled markers, which opens the note protected for block-by-block + * resolution. When no merge is possible (a side already carries markers, + * no merge is available, or the buffer would not hold still) the edits are + * kept as a copy beside the note and the external version is adopted. + * Either way nothing typed is lost and the buffer is never left unsavable. + */ + async function mergeExternal(content: string, attempt = 0): Promise { + const ours = header + buffer + if (content === ours) { + adoptCleanContent(content) // the same edit landed from both sides + return + } + 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:', cause) + } + } + if (discarded) { + return + } + if (header + buffer !== ours) { + // Keystrokes landed while merging: the result no longer covers the + // buffer. Merge again from the new buffer; a buffer that will not hold + // still for three rounds is kept aside instead. + if (attempt < 3) { + return await mergeExternal(content, attempt + 1) + } + merged = null + } + if (merged?.kind === 'clean' && classify(splitDoc(merged.content).body) !== 'lossy') { + adoptMerged(merged.content, content) + return + } + if (merged !== null && merged.kind !== 'unmergeable' && io.write !== null) { + // Markers, or syntax the editor cannot round-trip: the exact merge goes + // to disk and the note opens protected, never into the live editor. + await materialize(merged.content, ours, content, io.write) + return + } + if (await keepAside()) { + adoptCleanContent(content) + } + } + + /** + * Write a merge the live editor must not hold (markers, or syntax it + * cannot round-trip) over the external version it was made from (the + * write expects that version, so a file that moved again is reconciled + * afresh instead of overwritten) and adopt it: it opens protected. + * Keystrokes that land during the write are kept beside the note. + */ + async function materialize( + unsafe: string, + ours: string, + onDisk: string, + write: NonNullable, + ): Promise { + try { + await write(path, unsafe, onDisk) + } catch (cause) { + error = errorMessage(cause) + emit() + if (error.includes('changed on disk')) { + reconcileAgain = true // the file moved again: the rerun merges afresh + } + // Any other failure keeps the dirty buffer; its next save expects the + // old disk content, fails the same way, and reconciles again. + return + } + if (discarded) { + return + } + error = null + if (header + buffer === ours || (await keepAside())) { + adoptCleanContent(unsafe) + } + } + + /** + * Keep the buffer beside the note before external content replaces it. + * Keystrokes that land during the copy are copied again, so the copy holds + * what the user last saw. A copy that cannot be made (or a session with + * nowhere to make one) keeps the dirty buffer: its next save fails against + * the changed file and comes back here to retry. + */ + async function keepAside(): Promise { + if (io.copyAside === undefined) { + return false // nowhere to keep them: the buffer stays, the external version waits + } + let copy: ConflictCopy | null = null // this reconciliation's copy only + for (let round = 0; round < 3; round += 1) { + const contents = header + buffer + try { + copy = { path: await io.copyAside(path, contents, copy), contents } + } catch (cause) { + error = errorMessage(cause) + emit() + return false + } + if (disposed || header + buffer === contents) { + keptAside += 1 + return true + } + } + error = 'The note kept changing while its edits were being kept aside' + emit() + return false + } + + /** + * Put `merged` in the editor as the dirty buffer over `onDisk` and write + * it now: a merge is already the reconciled state of two writers, and a + * flush that ran into the external change is waiting on exactly this. + */ + function adoptMerged(merged: string, onDisk: string): void { + const doc = splitDoc(merged) + header = doc.header + buffer = doc.body + initialContent = doc.body // an editor that mounts later starts from the merge + disk = onDisk + dirty = merged !== onDisk + missing = false + error = null + emit() + applyToEditor(doc.body) + // The other writer's version is the new ground truth first: a title it + // changed must not read as this user's rename when the merge is saved. + onContent?.(onDisk, 'external') + save() + } + /** The initial read; with `createIfMissing`, a missing file is an empty note. */ async function readInitial(): Promise<{ content: string; fileMissing: boolean }> { try { @@ -288,7 +472,6 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { loading = true missedChange = false status = 'loading' - conflict = null error = null emit() loadPromise = (async () => { @@ -350,28 +533,6 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { void reconcileFromDisk() } - function keepMine(): void { - if (conflict !== null) { - disk = conflict - missing = false - } - conflict = null - dirty = true // force the rewrite even if content drifted equal - emit() - save() - } - - function loadTheirs(): void { - if (conflict === null) { - return - } - const content = conflict - conflict = 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 updateFrontmatter(patch: FrontmatterPatch): boolean { if (disposed || isProtected || status !== 'ready') { return false @@ -399,21 +560,11 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { } const attemptedHeader = header try { - if (conflict === null) { - const shouldPersist = dirty - await flush() - if (shouldPersist && error !== null) { - throw new Error(error) - } - } else { - // Keep both conflict resolutions consistent with the persisted flag. - const patched = upsertFrontmatter(conflict, frontmatterPatchToYaml(patch)) - if (patched !== conflict) { - await io.write(path, patched) - conflict = patched - disk = patched - emit() - } + const shouldPersist = dirty + const keptAsideBefore = keptAside + await flush() + if (shouldPersist && (error !== null || keptAside !== keptAsideBefore)) { + throw new Error(error ?? KEPT_ASIDE) } return true } catch (cause) { @@ -436,8 +587,8 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { * navigating to a note (⌘D to today) opens its session in the same tick the * Tasks view's unmount flush writes to it, and the pending read is a moment, * not a reason to refuse. Returns false when the session can't safely take a - * body edit (no write channel, disposed, protected/read-only, failed to load, - * or a parked conflict) so the caller refuses rather than clobber the buffer + * body edit (no write channel, disposed, protected/read-only, or failed to + * load) so the caller refuses rather than clobber the buffer * via disk. `transform` runs before any mutation, so a `TaskStaleError` (the * marker can't be located) propagates with nothing changed. And the write is * all-or-nothing: a failed flush reverts the in-memory edit so the editor and @@ -455,7 +606,7 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { await loadPromise } // Protection is decided by the load, so this gate runs after the wait. - if (disposed || isProtected || status !== 'ready' || conflict !== null) { + if (disposed || isProtected || status !== 'ready') { return false } reconcilePendingEditorInput?.() @@ -473,12 +624,15 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { // A no-op edit (transform changed nothing) writes nothing, so a *prior* // surfaced save error must not be mistaken for this edit's failure. const shouldPersist = dirty + const keptAsideBefore = keptAside emit() await flush() // `flush()` resolves even when the write failed (captured in `error`, not - // thrown). Revert and surface the failure: it persists, or nothing changes. - if (shouldPersist && error !== null) { - const message = error + // thrown), and a write refused by an external change may have left the + // edit in a copy beside the note. Revert and surface the failure: it + // persists, or nothing changes. + if (shouldPersist && (error !== null || keptAside !== keptAsideBefore)) { + const message = error ?? KEPT_ASIDE if (header === doc.header) header = previousHeader if (buffer === doc.body) { buffer = previousBuffer @@ -546,8 +700,6 @@ export function createNoteSession(options: NoteSessionOptions): NoteSession { editorChanged, externalChanged, flush, - keepMine, - loadTheirs, content: () => header + buffer, liveContent: () => (status === 'ready' ? header + buffer : null), isDirty: () => dirty, diff --git a/apps/desktop/src/editor/note-session-types.ts b/apps/desktop/src/editor/note-session-types.ts index e137e774b..3218e109e 100644 --- a/apps/desktop/src/editor/note-session-types.ts +++ b/apps/desktop/src/editor/note-session-types.ts @@ -1,3 +1,4 @@ +import type { MergeTextOutcome } from '@reflect/core' import type { FrontmatterPatch } from './note-session-frontmatter.ts' import type { RoundTripFidelity } from './roundtrip.ts' @@ -26,8 +27,6 @@ export interface NoteSessionSnapshot { * note exists only as this buffer until the first save lands. */ missing: boolean - /** External content waiting on the user's choice (set only when dirty). */ - conflict: string | null error: string | null } @@ -38,10 +37,15 @@ export const INITIAL_NOTE_SNAPSHOT: NoteSessionSnapshot = { protected: false, dirty: false, missing: false, - conflict: null, error: null, } +/** A conflict copy as last written: its path and the contents it holds. */ +export interface ConflictCopy { + path: string + contents: string +} + /** File access injected by the host (the hook binds `@reflect/core` commands). */ export interface NoteSessionIo { read: (path: string) => Promise @@ -53,6 +57,25 @@ export interface NoteSessionIo { write: | ((path: string, contents: string, expectedContents?: string | null) => Promise) | null + /** + * Three-way merge of the buffer (`ours`) and external content (`theirs`) + * over the last content read from disk (`base`). + */ + mergeText?: + | ((path: string, base: string, ours: string, theirs: string) => Promise) + | undefined + /** + * Keep `contents` as a sibling file of `path` (` (conflict).md`) and + * return the copy's path: the fallback when edits cannot be merged into an + * external change (the file already carries markers, or no merge is + * available), so nothing typed is ever lost. `previous` is the copy this + * reconciliation already made (newer keystrokes arrived during it): it is + * overwritten only while it still holds what was written, else a fresh + * sibling is made. A later conflict passes `null` and gets its own copy. + */ + copyAside?: + | ((path: string, contents: string, previous: ConflictCopy | null) => Promise) + | undefined } /** Why {@link NoteSessionOptions.onContent} fired. */ @@ -130,10 +153,6 @@ export interface NoteSession { * can't die before the bytes land. */ flush: () => Promise - /** Resolve a conflict by keeping the buffer (rewrites the file). */ - keepMine: () => void - /** Resolve a conflict by loading the external content (discards the buffer). */ - loadTheirs: () => void /** The full current document (frontmatter + buffer), as a save would write it. */ content: () => string /** @@ -180,13 +199,9 @@ export interface NoteSession { updateFrontmatter: (patch: FrontmatterPatch) => boolean /** * {@link NoteSession.updateFrontmatter}, but the patch **lands on disk now** - * regardless of session state. Normally that's a flush; under a parked - * conflict — where saves are paused and a flush is a deliberate no-op — the - * contested content is patched and written through too, so the index sees - * the change immediately and *both* resolutions keep it ("keep mine" writes - * the patched header, "load theirs" adopts the patched park). Same gating - * and false-return as `updateFrontmatter`. For patches that should ride the - * resolution instead (the rename alias), use `updateFrontmatter`. + * (a flush) so the index sees the change immediately. Same gating and + * false-return as `updateFrontmatter`. For patches that can ride the next + * save instead (the rename alias), use `updateFrontmatter`. */ commitFrontmatter: (patch: FrontmatterPatch) => Promise /** @@ -205,9 +220,9 @@ export interface NoteSession { * synchronously, so there is no read/write race with the editor. A session * still loading waits for the load, then applies the edit to the loaded * buffer. Returns false when the session can't take it (failed to load, - * protected, disposed, or a parked conflict) so the caller refuses rather - * than clobber the buffer; an - * error thrown by `transform` (a stale task locator) propagates untouched, + * protected, or disposed) so the caller refuses rather than clobber the + * buffer; an error thrown by `transform` (a stale task locator) propagates + * untouched, * and a failed flush reverts the in-memory edit before rethrowing. */ commitSourceEdit: (transform: (source: string) => string) => Promise diff --git a/apps/desktop/src/editor/note-session.test.ts b/apps/desktop/src/editor/note-session.test.ts index e242dc56b..c1fad7540 100644 --- a/apps/desktop/src/editor/note-session.test.ts +++ b/apps/desktop/src/editor/note-session.test.ts @@ -1,5 +1,6 @@ import { applyTaskEdits, projectTasks, TaskStaleError, type TaskLocator } from '@reflect/core' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { MergeTextOutcome } from '@reflect/core' import { createNoteSession, type NoteSessionSnapshot } from './note-session.ts' import type { RoundTripFidelity } from './roundtrip.ts' @@ -16,7 +17,7 @@ function toggleTransform(task: TaskLocator): (source: string) => string { /** * Direct tests of the document state machine, no React. The full pipeline - * (load, debounce, echo detection, conflict parking, protection) is covered + * (load, debounce, echo detection, external merges, protection) is covered * end-to-end through the hook in `use-note-document.test.tsx`; these pin the * session-level contracts the hook can't observe directly. */ @@ -31,6 +32,12 @@ interface Harness { setDisk: (contents: string | null) => void /** While set, writes reject with this message (the save-failure seam). */ failWrites: (message: string | null) => void + /** Script the next three-way merge outcome (`null` = the merge throws). */ + setMerge: (outcome: MergeTextOutcome | null) => void + /** Buffers kept beside the note when they could not be merged (`previous` = the copy being retaken). */ + copies: Array<{ path: string; contents: string; previous: string | null }> + /** While set, conflict copies reject with this message. */ + failCopies: (message: string | null) => void session: ReturnType } @@ -38,21 +45,30 @@ function harness(options?: { /** Runs before every read resolves (the slow-load seam). */ beforeRead?: () => Promise beforeWrite?: () => Promise + /** Runs before every conflict copy resolves (the slow-copy seam). */ + beforeCopy?: () => void write?: false + /** No conflict-copy capability (a session that can write but not copy aside). */ + copyAside?: false classify?: (markdown: string) => RoundTripFidelity /** `null` simulates a missing file: reads throw the notFound AppError. */ disk?: string | null createIfMissing?: boolean missingSeed?: string reconcilePendingEditorInput?: () => void + /** Initial scripted merge outcome; `undefined` = no merge capability. */ + merge?: MergeTextOutcome }): Harness { const snapshots: NoteSessionSnapshot[] = [] const expectedContents: (string | null | undefined)[] = [] const writes: Array<{ path: string; contents: string }> = [] const applied: string[] = [] const contents: Array<{ content: string; origin: string }> = [] + const copies: Array<{ path: string; contents: string; previous: string | null }> = [] let disk = options?.disk === undefined ? '# Hello\n' : options.disk let writeFailure: string | null = null + let copyFailure: string | null = null + let mergeOutcome: MergeTextOutcome | null = options?.merge ?? null const session = createNoteSession({ path: 'notes/a.md', io: { @@ -77,6 +93,26 @@ function harness(options?: { writes.push({ path, contents }) disk = contents }, + mergeText: + options?.merge === undefined + ? undefined + : async () => { + if (mergeOutcome === null) { + throw new Error('no merge') + } + return mergeOutcome + }, + copyAside: + options?.copyAside === false + ? undefined + : async (path, contents, previous) => { + options?.beforeCopy?.() + if (copyFailure !== null) { + throw new Error(copyFailure) + } + copies.push({ path, contents, previous: previous?.path ?? null }) + return previous?.path ?? `${path.slice(0, -3)} (conflict${copies.length}).md` + }, }, classify: options?.classify ?? (() => 'exact'), onSnapshot: (snapshot) => { @@ -107,6 +143,13 @@ function harness(options?: { failWrites: (message) => { writeFailure = message }, + failCopies: (message) => { + copyFailure = message + }, + setMerge: (outcome) => { + mergeOutcome = outcome + }, + copies, session, } } @@ -117,6 +160,7 @@ beforeEach(() => { afterEach(() => { vi.useRealTimers() + vi.restoreAllMocks() // a `console.error` spy must not outlive a failed test }) async function settled(): Promise { @@ -213,26 +257,441 @@ describe('createNoteSession', () => { expect(snapshots.length).toBe(afterLoad + 1) // one dirty transition, not two }) - it('keepMine rewrites the file even when the conflict content equals the buffer', async () => { - const { session, writes, snapshots, setDisk } = harness() + it('a clean three-way merge applies silently and keeps saving', async () => { + // The field report: a script appended to the daily note while the user + // folded a bullet. Nothing overlaps, so nothing to ask. + const merged = '# Hello\n\n+ mine\n- from the script\n' + const { session, writes, applied, snapshots, setDisk, expectedContents } = harness({ + merge: { kind: 'clean', content: merged }, + }) + session.load() + await settled() + session.editorChanged('# Hello\n\n+ mine\n') + setDisk('# Hello\n\n- mine\n- from the script\n') + session.externalChanged() + await settled() + + expect(snapshots.at(-1)).toMatchObject({ dirty: false, protected: false }) + expect(applied).toEqual([merged]) + expect(writes).toEqual([{ path: 'notes/a.md', contents: merged }]) + // The write expects the external content: that is what is on disk now. + expect(expectedContents.at(-1)).toBe('# Hello\n\n- mine\n- from the script\n') + }) + + it('a clean merge reports the other version as external before its own save', async () => { + // The other device retitled the note; the user edited the body. The + // rename tracker must take the new title as ground truth, not as this + // user's rename, so it hears `external` before `saved`. + const theirs = '# Renamed elsewhere\n\nbody\n' + const merged = '# Renamed elsewhere\n\nbody, edited here\n' + const { session, contents, setDisk } = harness({ + disk: '# Hello\n\nbody\n', + merge: { kind: 'clean', content: merged }, + }) + session.load() + await settled() + session.editorChanged('# Hello\n\nbody, edited here\n') + setDisk(theirs) + session.externalChanged() + await settled() + + expect(contents.slice(-2)).toEqual([ + { content: theirs, origin: 'external' }, + { content: merged, origin: 'saved' }, + ]) + }) + + it('a clean merge the editor cannot round-trip is written exactly and opens protected', async () => { + const merged = '+ [ ] mine\n+ [ ] theirs\n' + const { session, writes, applied, snapshots, setDisk, expectedContents } = harness({ + merge: { kind: 'clean', content: merged }, + classify: (markdown) => (markdown.includes('+ [ ]') ? 'lossy' : 'exact'), + }) + session.load() + await settled() + session.editorChanged('mine\n') + const appliedBefore = applied.length + setDisk('theirs\n') + session.externalChanged() + await settled() + + // The exact merge lands on disk; the live editor never sees it, so its + // normalized text can never be saved over the merge. + expect(writes).toEqual([{ path: 'notes/a.md', contents: merged }]) + expect(expectedContents.at(-1)).toBe('theirs\n') + expect(applied).toHaveLength(appliedBefore) + expect(snapshots.at(-1)).toMatchObject({ + protected: true, + dirty: false, + error: null, + initialContent: merged, + }) + }) + + it('overlapping edits are written into the file as markers and open protected', async () => { + const marked = '<<<<<<< this device\nmine\n=======\ntheirs\n>>>>>>> other device\n' + const { session, writes, snapshots, copies, setDisk, expectedContents } = harness({ + merge: { kind: 'conflicted', content: marked }, + }) + session.load() + await settled() + session.editorChanged('mine\n') + setDisk('theirs\n') + session.externalChanged() + await settled() + + // Written over the external version it was merged from, like a pull. + expect(writes).toEqual([{ path: 'notes/a.md', contents: marked }]) + expect(expectedContents.at(-1)).toBe('theirs\n') + expect(copies).toEqual([]) + expect(snapshots.at(-1)).toMatchObject({ + protected: true, + dirty: false, + error: null, + initialContent: marked, + }) + }) + + it('a file that moved again before the markers landed is merged afresh, not overwritten', async () => { + const marked = '<<<<<<< this device\nmine\n=======\ntheirs\n>>>>>>> other device\n' + let h: Harness | null = null + h = harness({ + merge: { kind: 'conflicted', content: marked }, + beforeWrite: async () => { + h?.setDisk('theirs, newer\n') // another device wrote again under the merge + }, + }) + h.session.load() + await settled() + h.session.editorChanged('mine\n') + h.setDisk('theirs\n') + h.session.externalChanged() + await settled() + + // The first write expected the version the merge was made from and was + // refused as stale; the session re-read the file and merged over it. + expect(h.expectedContents).toEqual(['theirs\n', 'theirs, newer\n']) + expect(h.writes).toEqual([{ path: 'notes/a.md', contents: marked }]) + expect(h.snapshots.at(-1)).toMatchObject({ protected: true, error: null }) + }) + + it('typing while the markers are written is kept beside the note', async () => { + const marked = '<<<<<<< this device\nmine\n=======\ntheirs\n>>>>>>> other device\n' + let target: ReturnType | null = null + const { session, writes, copies, snapshots, setDisk } = harness({ + merge: { kind: 'conflicted', content: marked }, + beforeWrite: async () => { + target?.editorChanged('mine, typed during the merge\n') + }, + }) + target = session + session.load() + await settled() + session.editorChanged('mine\n') + setDisk('theirs\n') + session.externalChanged() + await settled() + + expect(writes).toEqual([{ path: 'notes/a.md', contents: marked }]) + expect(copies).toMatchObject([ + { path: 'notes/a.md', contents: 'mine, typed during the merge\n' }, + ]) + expect(snapshots.at(-1)).toMatchObject({ protected: true, initialContent: marked }) + }) + + it('a failed marker write keeps the buffer dirty and the next save merges again', async () => { + const marked = '<<<<<<< this device\nmine\n=======\ntheirs\n>>>>>>> other device\n' + const { session, writes, snapshots, setDisk, failWrites } = harness({ + merge: { kind: 'conflicted', content: marked }, + }) + session.load() + await settled() + failWrites('disk full') + session.editorChanged('mine\n') + setDisk('theirs\n') + session.externalChanged() + await settled() + expect(writes).toEqual([]) + expect(snapshots.at(-1)).toMatchObject({ error: 'disk full', dirty: true, protected: false }) + + // The save chain is still live: the next save finds the file changed, + // reconciles, and lands the merge. + failWrites(null) + session.editorChanged('mine, more\n') + await settled() + expect(writes).toEqual([{ path: 'notes/a.md', contents: marked }]) + expect(snapshots.at(-1)).toMatchObject({ error: null, protected: true }) + }) + + it('edits that cannot be merged are kept beside the note and the external version loads', async () => { + const { session, writes, applied, copies, snapshots, setDisk } = harness({ + merge: { kind: 'unmergeable', content: 'theirs\n' }, + }) + session.load() + await settled() + session.editorChanged('mine\n') + setDisk('theirs\n') + session.externalChanged() + await settled() + + expect(writes).toEqual([]) + expect(copies).toMatchObject([{ path: 'notes/a.md', contents: 'mine\n' }]) + expect(applied).toEqual(['theirs\n']) + expect(snapshots.at(-1)).toMatchObject({ dirty: false, protected: false, error: null }) + }) + + it('a dispose flush refused by an external change still merges the buffer', async () => { + const merged = '# Hello\n\n+ mine\n- from the script\n' + const { session, writes, copies, setDisk } = harness({ + merge: { kind: 'clean', content: merged }, + }) + session.load() + await settled() + session.editorChanged('# Hello\n\n+ mine\n') + setDisk('# Hello\n\n- from the script\n') + // The pane unmounts: flush, then dispose in the same tick. + const flushed = session.flush() + session.dispose() + await flushed + + expect(writes).toEqual([{ path: 'notes/a.md', contents: merged }]) + expect(copies).toEqual([]) + }) + + it('a dispose flush refused with no merge available keeps the buffer beside the note', async () => { + const { session, writes, copies, setDisk } = harness() + session.load() + await settled() + session.editorChanged('mine\n') + setDisk('theirs\n') + const flushed = session.flush() + session.dispose() + await flushed + + expect(writes).toEqual([]) + expect(copies).toMatchObject([{ path: 'notes/a.md', contents: 'mine\n' }]) + }) + + it('two signals for one external change reconcile once', async () => { + const marked = '<<<<<<< this device\nmine\n=======\ntheirs\n>>>>>>> other device\n' + const { session, writes, copies, snapshots, setDisk } = harness({ + merge: { kind: 'conflicted', content: marked }, + }) + session.load() + await settled() + session.editorChanged('mine\n') + setDisk('theirs\n') + session.externalChanged() // the watcher + session.externalChanged() // a reload of the open documents + await settled() + + // Merged and written once; the second signal found nothing left to do. + expect(writes).toEqual([{ path: 'notes/a.md', contents: marked }]) + expect(copies).toEqual([]) + expect(snapshots.at(-1)).toMatchObject({ + protected: true, + dirty: false, + error: null, + initialContent: marked, + }) + }) + + it('a lossy clean merge stays out of the live editor even without a writer', async () => { + const merged = '+ [ ] mine\n+ [ ] theirs\n' + const { session, applied, copies, setDisk } = harness({ + write: false, + merge: { kind: 'clean', content: merged }, + classify: (markdown) => (markdown.includes('+ [ ]') ? 'lossy' : 'exact'), + }) + session.load() + await settled() + session.editorChanged('mine\n') + setDisk('theirs\n') + session.externalChanged() + await settled() + + expect(applied).not.toContain(merged) + expect(copies).toMatchObject([{ path: 'notes/a.md', contents: 'mine\n' }]) + }) + + it('a failed conflict copy keeps the dirty buffer, and the next save retries it', async () => { + const { session, writes, applied, copies, snapshots, setDisk, failCopies } = harness({ + merge: { kind: 'unmergeable', content: 'theirs\n' }, + }) + session.load() + await settled() + failCopies('disk full') + session.editorChanged('mine\n') + setDisk('theirs\n') + session.externalChanged() + await settled() + + // Nothing durable holds the edits yet, so nothing replaces them. + expect(copies).toEqual([]) + expect(applied).toEqual([]) + expect(session.content()).toBe('mine\n') + expect(snapshots.at(-1)).toMatchObject({ dirty: true, error: 'disk full' }) + + // The next save is refused as stale, reconciles, and copies again. + failCopies(null) + session.editorChanged('mine, more\n') + await settled() + expect(writes).toEqual([]) + expect(copies).toMatchObject([{ path: 'notes/a.md', contents: 'mine, more\n' }]) + expect(applied).toEqual(['theirs\n']) + expect(snapshots.at(-1)).toMatchObject({ dirty: false, error: null }) + }) + + it('keystrokes typed while the conflict copy is made are copied too', async () => { + let target: ReturnType | null = null + let typed = false + const { session, copies, applied, setDisk } = harness({ + merge: { kind: 'unmergeable', content: 'theirs\n' }, + beforeCopy: () => { + if (!typed) { + typed = true + target?.editorChanged('mine, typed during the copy\n') + } + }, + }) + target = session + session.load() + await settled() + session.editorChanged('mine\n') + setDisk('theirs\n') + session.externalChanged() + await settled() + + // The second round overwrites the copy the first made, not a new file. + expect(copies).toMatchObject([ + { contents: 'mine\n', previous: null }, + { contents: 'mine, typed during the copy\n', previous: 'notes/a (conflict1).md' }, + ]) + expect(applied).toEqual(['theirs\n']) + }) + + it('a later conflict in the same session gets its own copy', async () => { + const { session, copies, setDisk } = harness({ + merge: { kind: 'unmergeable', content: 'theirs\n' }, + }) + session.load() + await settled() + for (const [mine, theirs] of [ + ['mine A\n', 'theirs\n'], + ['mine B\n', 'theirs, again\n'], + ] as const) { + session.editorChanged(mine) + setDisk(theirs) + session.externalChanged() + await settled() + } + // Neither reconciliation knows about the other's copy: version A survives. + expect(copies).toMatchObject([ + { contents: 'mine A\n', previous: null }, + { contents: 'mine B\n', previous: null }, + ]) + }) + + it('a merge that throws keeps the edits beside the note before adopting the external version', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + const { session, copies, applied, writes, snapshots, setDisk, setMerge } = harness({ + merge: { kind: 'clean', content: 'never used\n' }, + }) session.load() await settled() + setMerge(null) // the merge command rejects (a dev harness without it, an IPC failure) + session.editorChanged('mine\n') + setDisk('theirs\n') + session.externalChanged() + await settled() - // The user types X while the same X lands on disk externally (e.g. another - // device synced the identical edit). The external content parks as a - // conflict; "keep mine" must still persist deterministically. + expect(consoleError).toHaveBeenCalledWith('three-way merge failed:', expect.any(Error)) + expect(copies).toMatchObject([{ path: 'notes/a.md', contents: 'mine\n' }]) + expect(applied).toEqual(['theirs\n']) + expect(writes).toEqual([]) + expect(snapshots.at(-1)).toMatchObject({ dirty: false, error: null }) + consoleError.mockRestore() + }) + + it('a stale autosave whose content is already on disk reconciles clean and clears the error', async () => { + // Another writer put exactly the buffer on disk before the checked save + // ran: the save is refused, the reconciliation adopts the matching + // content, and the failure it reported must not linger with nothing + // left to save. + const { session, snapshots, setDisk } = harness() + session.load() + await settled() session.editorChanged('# Same on both\n') setDisk('# Same on both\n') + await session.flush() + await settled() + expect(snapshots.at(-1)).toMatchObject({ dirty: false, error: null }) + }) + + it('with nowhere to keep the edits, the buffer stays and the external version waits', async () => { + const { session, applied, snapshots, setDisk } = harness({ + merge: { kind: 'unmergeable', content: 'theirs\n' }, + copyAside: false, + }) + session.load() + await settled() + session.editorChanged('mine\n') + setDisk('theirs\n') session.externalChanged() await settled() - expect(snapshots.at(-1)?.conflict).toBe('# Same on both\n') - expect(writes).toEqual([]) // parked conflict paused the debounced save - session.keepMine() + expect(applied).toEqual([]) + expect(session.content()).toBe('mine\n') + expect(snapshots.at(-1)?.dirty).toBe(true) + }) + + it('a flush that runs into an external change lands the clean merge before it resolves', async () => { + // Cmd-Q right after another device wrote: the checked save is refused, + // the merge is clean, and the flush must not report done until the + // merged content is on disk. + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + const merged = '# Hello\n\n- mine\n- theirs\n' + const { session, writes, snapshots, setDisk } = harness({ + merge: { kind: 'clean', content: merged }, + }) + session.load() + await settled() + session.editorChanged('# Hello\n\n- mine\n') + setDisk('# Hello\n\n- theirs\n') + await session.flush() + + expect(writes.at(-1)).toEqual({ path: 'notes/a.md', contents: merged }) + expect(snapshots.at(-1)).toMatchObject({ dirty: false, error: null }) + consoleError.mockRestore() + }) + + it('without a merge capability the edits are kept beside the note too', async () => { + const { session, copies, applied, setDisk } = harness() + session.load() + await settled() + session.editorChanged('mine\n') + setDisk('theirs\n') + session.externalChanged() await settled() - expect(writes).toEqual([{ path: 'notes/a.md', contents: '# Same on both\n' }]) - expect(snapshots.at(-1)?.conflict).toBeNull() - expect(snapshots.at(-1)?.dirty).toBe(false) + + expect(copies).toMatchObject([{ path: 'notes/a.md', contents: 'mine\n' }]) + expect(applied).toEqual(['theirs\n']) + }) + + it('the same edit landing from both sides adopts cleanly', async () => { + // The user types X while the same X lands on disk (another device synced + // the identical edit): nothing to merge, nothing to keep aside. + const { session, writes, copies, snapshots, setDisk } = harness() + session.load() + await settled() + session.editorChanged('# Same on both\n') + setDisk('# Same on both\n') + session.externalChanged() + await settled() + + expect(copies).toEqual([]) + expect(writes).toEqual([]) + expect(snapshots.at(-1)).toMatchObject({ dirty: false, initialContent: '# Same on both\n' }) }) it('re-gates protection when external content stops being representable', async () => { @@ -343,58 +802,13 @@ describe('frontmatter ownership (Plan 07b)', () => { h.setDisk(`---\naliases:\n - Newer\n---\n# Hello\n`) h.session.externalChanged() await vi.runAllTimersAsync() - expect(h.snapshots.at(-1)?.conflict).toBeNull() + expect(h.snapshots.at(-1)?.dirty).toBe(false) // Next save preserves the adopted header. h.session.editorChanged('# Hello!\n') await vi.runAllTimersAsync() expect(h.writes.at(-1)?.contents).toBe('---\naliases:\n - Newer\n---\n\n# Hello!\n') }) - it('a frontmatter patch under a parked conflict lands with "keep mine"', async () => { - // The rename coordinator's alias can arrive while a conflict is parked: - // it rides the in-memory header (saves are paused, not dropped) and - // persists when the user keeps their version. "Load theirs" discarding - // it is the user explicitly choosing external content over the rename's - // consequences — a disk write here would clobber the protected "theirs". - const h = harness() - h.session.load() - await vi.runAllTimersAsync() - h.session.editorChanged('# Mine\n') // dirty - h.setDisk('# Theirs\n') - h.session.externalChanged() - await vi.runAllTimersAsync() - expect(h.snapshots.at(-1)?.conflict).toBe('# Theirs\n') - - expect(h.session.updateFrontmatter({ aliases: ['Old Title'] })).toBe(true) - await vi.runAllTimersAsync() - expect(h.writes).toEqual([]) // paused, not written under the conflict - - h.session.keepMine() - await vi.runAllTimersAsync() - const written = h.writes.at(-1)?.contents ?? '' - expect(written).toContain('Old Title') // the alias survived the conflict - expect(written).toContain('# Mine') - }) - - it('a later external change refreshes a parked conflict snapshot', async () => { - const h = harness() - h.session.load() - await vi.runAllTimersAsync() - h.session.editorChanged('# Mine\n') - h.setDisk('# Theirs\n') - h.session.externalChanged() - await vi.runAllTimersAsync() - expect(h.snapshots.at(-1)?.conflict).toBe('# Theirs\n') - - h.setDisk('---\npinned: true\n---\n# Theirs\n') - h.session.externalChanged() - await vi.runAllTimersAsync() - expect(h.snapshots.at(-1)?.conflict).toBe('---\npinned: true\n---\n# Theirs\n') - - h.session.loadTheirs() - expect(h.session.content()).toBe('---\npinned: true\n---\n\n# Theirs\n') - }) - it('commitFrontmatter lands the patch immediately on a clean session', async () => { const h = harness({ disk: '# Hello\n' }) h.session.load() @@ -438,6 +852,23 @@ describe('frontmatter ownership (Plan 07b)', () => { } }) + it('a frontmatter commit whose edit was kept beside the note reports the failure', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + const h = harness({ merge: { kind: 'unmergeable', content: 'theirs\n' } }) + try { + h.session.load() + await settled() + h.setDisk('theirs\n') // changed on disk, not yet noticed + await expect(h.session.commitFrontmatter({ pinned: true })).rejects.toThrow('kept beside') + expect(h.copies).toHaveLength(1) + expect(h.session.content()).toBe('theirs\n') + expect(h.snapshots.at(-1)).toMatchObject({ dirty: false, error: null }) + } finally { + h.session.discard() + consoleError.mockRestore() + } + }) + it('a no-op frontmatter commit does not report an earlier save error', async () => { const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) const h = harness() @@ -456,23 +887,6 @@ describe('frontmatter ownership (Plan 07b)', () => { } }) - it('a failed conflict patch restores the header and leaves the conflict intact', async () => { - const h = harness() - h.session.load() - await settled() - h.session.editorChanged('# Mine\n') - h.setDisk('# Theirs\n') - h.session.externalChanged() - await settled() - h.failWrites('disk full') - - await expect(h.session.commitFrontmatter({ pinned: true })).rejects.toThrow('disk full') - expect(h.session.content()).toBe('# Mine\n') - expect(h.snapshots.at(-1)?.conflict).toBe('# Theirs\n') - expect(h.snapshots.at(-1)?.dirty).toBe(true) - h.session.discard() - }) - it('commitFrontmatter declines when the session has no write channel', async () => { // No graph generation → no `io.write`. The patch can't land, so report // false rather than the in-memory success that would let publish/pin/private @@ -485,40 +899,6 @@ describe('frontmatter ownership (Plan 07b)', () => { expect(h.writes).toEqual([]) }) - it('commitFrontmatter under a parked conflict writes through and refreshes the park', async () => { - const h = harness() - h.session.load() - await vi.runAllTimersAsync() - h.session.editorChanged('# Mine\n') - h.setDisk('# Theirs\n') - h.session.externalChanged() - await vi.runAllTimersAsync() - expect(h.snapshots.at(-1)?.conflict).toBe('# Theirs\n') - - await expect(h.session.commitFrontmatter({ pinned: true })).resolves.toBe(true) - // The contested content was patched and written — the index sees it now… - expect(h.writes.at(-1)?.contents).toBe('---\npinned: true\n---\n\n# Theirs\n') - // …the park holds the patched bytes, so "load theirs" adopts the pin… - expect(h.snapshots.at(-1)?.conflict).toBe('---\npinned: true\n---\n\n# Theirs\n') - h.session.loadTheirs() - expect(h.session.content()).toBe('---\npinned: true\n---\n\n# Theirs\n') - }) - - it('commitFrontmatter under a conflict keeps the patch through "keep mine" too', async () => { - const h = harness() - h.session.load() - await vi.runAllTimersAsync() - h.session.editorChanged('# Mine\n') - h.setDisk('# Theirs\n') - h.session.externalChanged() - await vi.runAllTimersAsync() - - await h.session.commitFrontmatter({ pinned: true }) - h.session.keepMine() - await vi.runAllTimersAsync() - expect(h.writes.at(-1)?.contents).toBe('---\npinned: true\n---\n\n# Mine\n') - }) - it('onContent reports full joined content with the right origins', async () => { const h = harness({ disk: `${FM}# Hello\n` }) h.session.load() @@ -644,7 +1024,7 @@ describe('missing-note seed (new ordinary notes)', () => { it('an external write while the seed is showing adopts cleanly and clears missing', async () => { // Another device/process creates the file while the seeded buffer is open - // and untouched: not a conflict — the buffer was never dirty. + // and untouched: nothing to merge — the buffer was never dirty. const h = harness({ disk: null, createIfMissing: true, missingSeed: SEED }) h.session.load() await settled() @@ -654,7 +1034,6 @@ describe('missing-note seed (new ordinary notes)', () => { await settled() const ready = h.snapshots.at(-1) - expect(ready?.conflict).toBeNull() expect(ready?.missing).toBe(false) expect(h.applied).toEqual(['# Created elsewhere\n']) expect(h.writes).toEqual([]) @@ -674,7 +1053,6 @@ describe('missing-note seed (new ordinary notes)', () => { const ready = h.snapshots.at(-1) expect(ready?.missing).toBe(false) - expect(ready?.conflict).toBeNull() expect(h.applied).toEqual([]) // content unchanged: no editor reload expect(h.writes).toEqual([]) }) @@ -708,11 +1086,10 @@ describe('missing-note seed (new ordinary notes)', () => { h.session.editorChanged('# Unsaved\n') h.setDisk(null) - h.session.externalChanged() // read fails: nothing to park a conflict against + h.session.externalChanged() // read fails: nothing to merge against await settled() const after = h.snapshots.at(-1) - expect(after?.conflict).toBeNull() expect(after?.dirty).toBe(false) // the debounced save already landed… expect(h.writes.at(-1)).toEqual({ path: 'notes/a.md', contents: '# Unsaved\n' }) // …recreating the file }) @@ -951,15 +1328,17 @@ describe('commitSourceEdit', () => { expect(h.writes).toEqual([]) }) - it('refuses (returns false) while a conflict is parked', async () => { + it('refuses (returns false) once an external merge opened the note protected', async () => { const source = '+ [ ] x\n' - const h = harness({ disk: source }) + const marked = + '<<<<<<< this device\n+ [ ] x edited\n=======\n+ [ ] x external\n>>>>>>> other device\n' + const h = harness({ disk: source, merge: { kind: 'conflicted', content: marked } }) h.session.load() await settled() h.session.editorChanged('+ [ ] x edited\n') // dirty h.setDisk('+ [ ] x external\n') - h.session.externalChanged() // parks a conflict (dirty + divergent disk) + h.session.externalChanged() // overlapping edits: markers land, the note goes protected await settled() expect(await h.session.commitSourceEdit(toggleTransform(firstTask(source)))).toBe(false) @@ -1028,24 +1407,21 @@ it.each([null, '# Hello\n'])( }, ) -it('parks an external revision when a checked autosave fails', async () => { +it('a checked autosave that finds the file changed merges the external revision in', async () => { const log = vi.spyOn(console, 'error').mockImplementation(() => {}) - const h = harness() + const merged = '# My unsaved text\n# Capture wrote here\n' + const h = harness({ merge: { kind: 'clean', content: merged } }) try { h.session.load() await settled() h.session.editorChanged('# My unsaved text\n') h.setDisk('# Capture wrote here\n') - await h.session.flush() - expect(h.snapshots.at(-1)?.conflict).toBe('# Capture wrote here\n') - expect(h.session.content()).toBe('# My unsaved text\n') - expect(h.writes).toEqual([]) - await h.session.flush() - expect(h.expectedContents).toHaveLength(1) - h.session.keepMine() - await h.session.flush() - expect(h.expectedContents.at(-1)).toBe('# Capture wrote here\n') - expect(h.writes.at(-1)?.contents).toBe('# My unsaved text\n') + await h.session.flush() // refused as stale, then reconciled and merged + await settled() + expect(h.expectedContents).toEqual(['# Hello\n', '# Capture wrote here\n']) + expect(h.writes).toEqual([{ path: 'notes/a.md', contents: merged }]) + expect(h.session.content()).toBe(merged) + expect(h.snapshots.at(-1)).toMatchObject({ dirty: false, error: null }) } finally { h.session.discard() log.mockRestore() diff --git a/apps/desktop/src/editor/note-session.ts b/apps/desktop/src/editor/note-session.ts index b02958c0c..dc11e0c48 100644 --- a/apps/desktop/src/editor/note-session.ts +++ b/apps/desktop/src/editor/note-session.ts @@ -7,6 +7,7 @@ export { frontmatterPatchToYaml } from './note-session-frontmatter.ts' export { INITIAL_NOTE_SNAPSHOT } from './note-session-types.ts' export type { FrontmatterPatch } from './note-session-frontmatter.ts' export type { + ConflictCopy, NoteContentOrigin, NoteSession, NoteSessionIo, diff --git a/apps/desktop/src/editor/open-documents.test.ts b/apps/desktop/src/editor/open-documents.test.ts index bfe0a5444..beb05c028 100644 --- a/apps/desktop/src/editor/open-documents.test.ts +++ b/apps/desktop/src/editor/open-documents.test.ts @@ -19,12 +19,10 @@ function fakeSession(path: string, log: string[]): NoteSession { flush: async () => { log.push(`flush:${path}`) }, - keepMine: () => {}, isDirty: () => false, isUnpersisted: () => false, prepareDelete: async () => false, cancelDelete: () => {}, - loadTheirs: () => {}, commitFrontmatter: async () => true, content: () => '', liveContent: () => '', @@ -153,12 +151,20 @@ describe('reloadOpenDocuments with live sessions', () => { read: () => string, applied: string[], snapshots: NoteSessionSnapshot[], + copies: string[] = [], ): NoteSession { return createNoteSession({ path: 'notes/stale.md', // No writer: the session tracks dirtiness but never writes, so a dirty // buffer can't race a debounced save into the assertions. - io: { read: async () => read(), write: null }, + io: { + read: async () => read(), + write: null, + copyAside: async (path, contents) => { + copies.push(contents) + return `${path} (conflict).md` + }, + }, classify: () => 'exact', onSnapshot: (snapshot) => { snapshots.push(snapshot) @@ -183,17 +189,19 @@ describe('reloadOpenDocuments with live sessions', () => { reloadOpenDocuments() await vi.waitFor(() => expect(applied).toContain('# New from the Mac\n')) - expect(snapshots.at(-1)?.conflict).toBeNull() + expect(snapshots.at(-1)?.dirty).toBe(false) } finally { unregister() session.dispose() } }) - it('a dirty open note parks the conflict banner instead of losing edits', async () => { + it('a dirty open note keeps its edits beside the note instead of losing them', async () => { let disk = '# Old\n' + const applied: string[] = [] + const copies: string[] = [] const snapshots: NoteSessionSnapshot[] = [] - const session = liveSession(() => disk, [], snapshots) + const session = liveSession(() => disk, applied, snapshots, copies) const unregister = registerOpenDocument({ session }) try { session.load() @@ -203,15 +211,16 @@ describe('reloadOpenDocuments with live sessions', () => { disk = '# New from the Mac\n' reloadOpenDocuments() - await vi.waitFor(() => expect(snapshots.at(-1)?.conflict).toBe('# New from the Mac\n')) - expect(snapshots.at(-1)?.dirty).toBe(true) + await vi.waitFor(() => expect(applied).toContain('# New from the Mac\n')) + expect(copies).toEqual(['# Old\nmy unsaved line\n']) + expect(snapshots.at(-1)?.dirty).toBe(false) } finally { unregister() - session.discard() // never flush the deliberately-dirty buffer + session.dispose() } }) - it('an unchanged file is a no-op read: no editor push, no conflict', async () => { + it('an unchanged file is a no-op read: no editor push, no merge', async () => { const applied: string[] = [] const snapshots: NoteSessionSnapshot[] = [] const session = liveSession(() => '# Old\n', applied, snapshots) diff --git a/apps/desktop/src/editor/open-documents.ts b/apps/desktop/src/editor/open-documents.ts index 223920fce..107eb6d50 100644 --- a/apps/desktop/src/editor/open-documents.ts +++ b/apps/desktop/src/editor/open-documents.ts @@ -52,7 +52,7 @@ export function openSession(path: string): NoteSession | null { /** * Paths of open documents holding unsaved edits right now. The iCloud - * conflict sweep (Plan 21) defers these — the session's own conflict parking + * conflict sweep (Plan 21) defers these — the session's own three-way merge * protects the buffer regardless, but skipping avoids churning a note the * user is mid-thought in. */ @@ -65,7 +65,7 @@ export function dirtyOpenPaths(): string[] { /** * Ask every open document to reconcile against disk, exactly as if a watcher * upsert had arrived for its path: an unchanged file is a no-op, a clean - * buffer adopts silently, a dirty buffer parks the conflict banner. The index + * buffer adopts silently, a dirty buffer merges three-way. The index * lifecycle calls this after each completed reconcile pass, because the pass * reads changed files into the index without emitting per-file events. On iOS * there is no file watcher, so a resume reconcile is the only signal a remote diff --git a/apps/desktop/src/editor/rename-coordinator.test.ts b/apps/desktop/src/editor/rename-coordinator.test.ts index 58c70d378..a27f795ce 100644 --- a/apps/desktop/src/editor/rename-coordinator.test.ts +++ b/apps/desktop/src/editor/rename-coordinator.test.ts @@ -71,14 +71,10 @@ function managed(content: string): string { return upsertFrontmatter(content, { id: MANAGED_ID }) } -function makeCoordinator(overrides?: { - generation?: () => number | null - canFire?: () => boolean -}) { +function makeCoordinator(overrides?: { generation?: () => number | null }) { return createRenameCoordinator({ path: PATH, generation: overrides?.generation ?? (() => 7), - canFire: overrides?.canFire ?? (() => true), }) } @@ -111,12 +107,10 @@ function fakeSession(content: string): NoteSession & { editorChanged: () => {}, externalChanged: () => {}, flush: vi.fn(async () => {}), - keepMine: () => {}, isDirty: () => false, isUnpersisted: () => false, prepareDelete: async () => false, cancelDelete: () => {}, - loadTheirs: () => {}, commitFrontmatter: async () => true, content: () => content, liveContent: () => content, @@ -303,22 +297,6 @@ describe('rename coordinator', () => { expect(consoleError).toHaveBeenCalledWith(expect.stringContaining('rename dropped')) }) - it('a blocked settle keeps the rename pending; the next settle fires it', async () => { - let armed = false - const coordinator = makeCoordinator({ canFire: () => armed }) - io.readNote.mockResolvedValue(managed('# New Title\n')) - coordinator.content(managed('# Old Title\n'), 'load') - coordinator.content(managed('# New Title\n'), 'saved') - coordinator.settle() // conflict parked: must not fire - await coordinator.settled() - expect(io.rewriteLinksForTitleChange).not.toHaveBeenCalled() - - armed = true // "keep mine" resolved the conflict - coordinator.settle() - await coordinator.settled() - expect(io.rewriteLinksForTitleChange).toHaveBeenCalledTimes(1) - }) - it('external content re-baselines: no rewrite for titles the user did not author', async () => { const coordinator = makeCoordinator() coordinator.content(managed('# Old Title\n'), 'load') @@ -484,7 +462,6 @@ describe('rename coordinator', () => { const coordinator = createRenameCoordinator({ path: 'Projects/subject.md', generation: () => 7, - canFire: () => true, }) coordinator.content(managed('# Old Title\n'), 'load') coordinator.content(managed('# New Title\n'), 'saved') @@ -510,7 +487,6 @@ describe('rename coordinator', () => { const coordinator = createRenameCoordinator({ path: `notes/${base}.md`, generation: () => 7, - canFire: () => true, }) coordinator.content(oldSource, 'load') coordinator.content(newSource, 'saved') diff --git a/apps/desktop/src/editor/rename-coordinator.ts b/apps/desktop/src/editor/rename-coordinator.ts index a4dd0d2ad..57c253db7 100644 --- a/apps/desktop/src/editor/rename-coordinator.ts +++ b/apps/desktop/src/editor/rename-coordinator.ts @@ -55,11 +55,6 @@ export interface RenameCoordinatorOptions { path: string /** Read the graph generation at rewrite time — never captured early. */ generation: () => number | null - /** - * Gate: no rename fires while false (a parked conflict contests the very - * content the title came from; "keep mine" re-arms, "load theirs" cancels). - */ - canFire: () => boolean } export interface RenameCoordinator { @@ -73,7 +68,7 @@ export interface RenameCoordinator { } export function createRenameCoordinator(options: RenameCoordinatorOptions): RenameCoordinator { - const { generation, canFire } = options + const { generation } = options /** The note's current path — a landed move advances it (Plan 17). */ let currentPath = options.path /** Serializes rewrites — a second settle waits for the first. */ @@ -230,7 +225,6 @@ export function createRenameCoordinator(options: RenameCoordinatorOptions): Rena const tracker = createTitleRenameTracker({ path: options.path, onRename: runRename, - canFire, }) return { diff --git a/apps/desktop/src/editor/title-rename.test.ts b/apps/desktop/src/editor/title-rename.test.ts index 652187096..5717f72d3 100644 --- a/apps/desktop/src/editor/title-rename.test.ts +++ b/apps/desktop/src/editor/title-rename.test.ts @@ -8,7 +8,7 @@ afterEach(() => { vi.useRealTimers() }) -function tracked(options?: { canFire?: () => boolean }) { +function tracked() { const renames: TitleRename[] = [] const tracker = createTitleRenameTracker({ path: 'notes/x.md', @@ -16,7 +16,6 @@ function tracked(options?: { canFire?: () => boolean }) { onRename: (rename) => { renames.push(rename) }, - canFire: options?.canFire, }) return { tracker, renames } } @@ -105,21 +104,6 @@ describe('createTitleRenameTracker', () => { expect(renames[0]).toMatchObject({ from: 'Real Title', to: 'Renamed' }) }) - it('a blocked fire keeps the rename pending until the gate opens', () => { - let conflictParked = true - const { tracker, renames } = tracked({ canFire: () => !conflictParked }) - tracker.baseline('# A\n') - tracker.saved('# B\n') - tracker.settle() // blocked: conflict parked - vi.advanceTimersByTime(10_000) - expect(renames).toEqual([]) - - conflictParked = false - tracker.saved('# B\n') // "keep mine" re-saves the same title → re-arms - vi.advanceTimersByTime(5000) - expect(renames).toEqual([{ from: 'A', to: 'B', previousAutoAlias: null }]) - }) - it('an H1 edit under an explicit frontmatter title is not a rename', () => { // `title:` is authoritative (deriveTitle precedence, same as the indexer): // the heading isn't the title, links resolve against `title:` regardless, diff --git a/apps/desktop/src/editor/title-rename.ts b/apps/desktop/src/editor/title-rename.ts index c70da1624..804831db1 100644 --- a/apps/desktop/src/editor/title-rename.ts +++ b/apps/desktop/src/editor/title-rename.ts @@ -43,13 +43,6 @@ export interface TitleRenameTrackerOptions { /** Graph-relative path (feeds title derivation's path fallback). */ path: string onRename: (rename: TitleRename) => void - /** - * Gate checked at fire time (e.g. "no conflict is parked"). When false the - * pending rename is kept, not dropped: a post-resolution save re-arms it - * ("keep mine"), while adopted external content clears it via `baseline` - * ("load theirs") — exactly the two ways a conflict can end. - */ - canFire?: (() => boolean) | undefined quietMs?: number } @@ -66,7 +59,7 @@ export interface TitleRenameTracker { const DEFAULT_QUIET_MS = 5000 export function createTitleRenameTracker(options: TitleRenameTrackerOptions): TitleRenameTracker { - const { path, onRename, canFire } = options + const { path, onRename } = options const quietMs = options.quietMs ?? DEFAULT_QUIET_MS let baselineTitle: string | null = null @@ -100,9 +93,6 @@ export function createTitleRenameTracker(options: TitleRenameTrackerOptions): Ti if (disposed || pending === null) { return } - if (canFire !== undefined && !canFire()) { - return // blocked (conflict parked): keep pending, mutate nothing - } const rename: TitleRename = { from: baselineTitle, to: pending, diff --git a/apps/desktop/src/editor/use-note-document.test.tsx b/apps/desktop/src/editor/use-note-document.test.tsx index 47167971b..cb0a2a66a 100644 --- a/apps/desktop/src/editor/use-note-document.test.tsx +++ b/apps/desktop/src/editor/use-note-document.test.tsx @@ -21,6 +21,9 @@ setBridge({ /** The fake on-disk file + a write log, behind the mocked IPC. */ let disk: string +let created: Array<{ path: string; contents: string }> = [] +let copyWrites: Array<{ path: string; contents: string }> = [] +let createGate: (() => Promise) | null = null let writes: string[] const MANAGED_ID = '01hv3xq7c2dm8k4t9w5e6r1n98' @@ -54,6 +57,9 @@ function fakeEditor(): NoteEditorHandle & { applied: string[] } { beforeEach(() => { disk = '# Hello\n' writes = [] + created = [] + copyWrites = [] + createGate = null emitChange = null mockInvoke.mockReset() mockInvoke.mockImplementation(async (command, args) => { @@ -61,11 +67,41 @@ beforeEach(() => { return disk } if (command === 'note_write') { - const contents = (args as { contents: string }).contents + const { path, contents, expectedContents } = args as { + path: string + contents: string + expectedContents?: string + } + if (path.includes(' (conflict')) { + const copy = created.find((entry) => entry.path === path) + if ( + copy === undefined || + (expectedContents !== undefined && expectedContents !== copy.contents) + ) { + throw { kind: 'io', message: 'Note changed on disk; reload before retrying' } + } + copy.contents = contents + copyWrites.push({ path, contents }) + return null + } disk = contents writes.push(contents) return null } + if (command === 'conflict_merge_text') { + // The hook tests exercise the copy-aside path; merging is the session's job. + return { kind: 'unmergeable', content: (args as { theirs: string }).theirs } + } + if (command === 'note_create') { + const { path, contents } = args as { path: string; contents: string } + if (created.some((entry) => entry.path === path)) { + return { kind: 'collision' } + } + // The entry exists before the gated call resolves, like a file on disk. + created.push({ path, contents }) + await createGate?.() + return { kind: 'created', modifiedMs: null } + } return null }) }) @@ -631,10 +667,9 @@ describe('useNoteDocument', () => { releaseRewrite() await paneA2.act(() => vi.runAllTimersAsync()) - // The alias went through the live session — no conflict from our own + // The alias went through the live session — no merge against our own // background write, and the user's edit and the alias both persist // (carried to the slug path by the move). - expect(paneA2.result.current.conflict).toBeNull() expect(files['notes/new-title.md']).toContain('aliases:') expect(files['notes/new-title.md']).toContain('Old Title') expect(files['notes/new-title.md']).toContain('fresh edit') @@ -678,7 +713,7 @@ describe('useNoteDocument', () => { await act(() => emitChange?.([{ path: 'notes/a.md', kind: 'upsert' }])) await act(async () => {}) expect(editor.applied).toEqual([]) - expect(result.current.conflict).toBeNull() + expect(result.current.dirty).toBe(false) }) it('reloads a clean buffer on a real external change', async () => { @@ -689,11 +724,10 @@ describe('useNoteDocument', () => { disk = '# Changed outside\n' await act(() => emitChange?.([{ path: 'notes/a.md', kind: 'upsert' }])) await vi.waitFor(() => expect(editor.applied).toEqual(['# Changed outside\n'])) - expect(result.current.conflict).toBeNull() expect(result.current.dirty).toBe(false) }) - it('parks an external change as a conflict when the buffer is dirty', async () => { + it('keeps unmergeable edits beside the note and loads the external version', async () => { const { result, act } = await readyHook() const editor = fakeEditor() await act(() => result.current.bindEditor(editor)) @@ -701,16 +735,84 @@ describe('useNoteDocument', () => { await act(() => result.current.onEditorChange('# My unsaved edit\n')) disk = '# Theirs\n' await act(() => emitChange?.([{ path: 'notes/a.md', kind: 'upsert' }])) - await vi.waitFor(() => expect(result.current.conflict).toBe('# Theirs\n')) - expect(editor.applied).toEqual([]) // never clobbered - - // Load theirs: applies the external content and clears the conflict. - await act(() => result.current.loadTheirs()) - expect(editor.applied).toEqual(['# Theirs\n']) - expect(result.current.conflict).toBeNull() + await vi.waitFor(() => expect(editor.applied).toEqual(['# Theirs\n'])) + expect(created).toEqual([{ path: 'notes/a (conflict).md', contents: '# My unsaved edit\n' }]) + expect(writes).toEqual([]) // the external version was never clobbered expect(result.current.dirty).toBe(false) }) + it('keystrokes typed while the conflict copy is made overwrite the same copy', async () => { + const { result, act } = await readyHook() + const editor = fakeEditor() + await act(() => result.current.bindEditor(editor)) + await act(() => result.current.onEditorChange('# My unsaved edit\n')) + + let releaseCreate: (() => void) | null = null + createGate = () => + new Promise((resolve) => { + releaseCreate = resolve + }) + disk = '# Theirs\n' + await act(() => emitChange?.([{ path: 'notes/a.md', kind: 'upsert' }])) + await vi.waitFor(() => expect(releaseCreate).not.toBeNull()) + await act(() => result.current.onEditorChange('# My unsaved edit, and more\n')) + await act(() => releaseCreate?.()) + + await vi.waitFor(() => expect(editor.applied).toEqual(['# Theirs\n'])) + expect(created.map((entry) => entry.path)).toEqual(['notes/a (conflict).md']) + expect(copyWrites).toEqual([ + { path: 'notes/a (conflict).md', contents: '# My unsaved edit, and more\n' }, + ]) + expect(writes).toEqual([]) + }) + + it('a later conflict in the same session gets its own sibling, and a copy that moved is left alone', async () => { + const { result, act } = await readyHook() + const editor = fakeEditor() + await act(() => result.current.bindEditor(editor)) + + await act(() => result.current.onEditorChange('# Version A\n')) + disk = '# Theirs\n' + await act(() => emitChange?.([{ path: 'notes/a.md', kind: 'upsert' }])) + await vi.waitFor(() => expect(editor.applied).toEqual(['# Theirs\n'])) + + await act(() => result.current.onEditorChange('# Version B\n')) + disk = '# Theirs, again\n' + await act(() => emitChange?.([{ path: 'notes/a.md', kind: 'upsert' }])) + await vi.waitFor(() => expect(editor.applied).toEqual(['# Theirs\n', '# Theirs, again\n'])) + expect(created).toEqual([ + { path: 'notes/a (conflict).md', contents: '# Version A\n' }, + { path: 'notes/a (conflict 2).md', contents: '# Version B\n' }, + ]) + + // Keystrokes during a copy retake it, but a copy another writer changed + // meanwhile is kept and the newer buffer goes to a fresh sibling. + let releaseCreate: (() => void) | null = null + createGate = () => + new Promise((resolve) => { + releaseCreate = resolve + }) + await act(() => result.current.onEditorChange('# Version C\n')) + disk = '# Theirs, third\n' + await act(() => emitChange?.([{ path: 'notes/a.md', kind: 'upsert' }])) + await vi.waitFor(() => expect(releaseCreate).not.toBeNull()) + await act(() => result.current.onEditorChange('# Version C, and more\n')) + created.find((entry) => entry.path === 'notes/a (conflict 3).md')!.contents = + '# edited elsewhere\n' + createGate = null + await act(() => releaseCreate?.()) + await vi.waitFor(() => + expect(created.at(-1)).toEqual({ + path: 'notes/a (conflict 4).md', + contents: '# Version C, and more\n', + }), + ) + expect(created.find((entry) => entry.path === 'notes/a (conflict 3).md')?.contents).toBe( + '# edited elsewhere\n', + ) + expect(copyWrites).toEqual([]) + }) + it('opens a note the editor would corrupt in protected mode and never saves it', async () => { vi.useFakeTimers() try { @@ -813,34 +915,6 @@ describe('useNoteDocument', () => { } }) - it('pauses saves while a conflict is parked (no clobbering theirs)', async () => { - vi.useFakeTimers() - try { - const hook = await renderHook(() => useNoteDocument('notes/a.md', 1)) - await hook.act(() => vi.advanceTimersByTimeAsync(0)) - - // An edit schedules a save, then an external change parks a conflict - // before the debounce fires. - await hook.act(() => hook.result.current.onEditorChange('# Mine\n')) - disk = '# Theirs\n' - await hook.act(() => emitChange?.([{ path: 'notes/a.md', kind: 'upsert' }])) - await hook.act(() => vi.advanceTimersByTimeAsync(0)) - expect(hook.result.current.conflict).toBe('# Theirs\n') - - // Neither the pending debounce nor an explicit flush may write now. - await hook.act(() => hook.result.current.onEditorChange('# Mine v2\n')) - await hook.act(() => vi.advanceTimersByTimeAsync(5000)) - expect(writes).toEqual([]) - - // Resolution unblocks: keepMine rewrites with the buffer. - await hook.act(() => hook.result.current.keepMine()) - await hook.act(() => vi.advanceTimersByTimeAsync(0)) - expect(writes).toEqual(['# Mine v2\n']) - } finally { - vi.useRealTimers() - } - }) - it('treats a watcher event for an in-flight save as an echo, not a conflict', async () => { vi.useFakeTimers() try { @@ -872,7 +946,8 @@ describe('useNoteDocument', () => { await hook.act(() => hook.result.current.onEditorChange('# Saved and more\n')) await hook.act(() => emitChange?.([{ path: 'notes/a.md', kind: 'upsert' }])) await hook.act(() => vi.advanceTimersByTimeAsync(0)) - expect(hook.result.current.conflict).toBeNull() // echo, not a conflict + expect(created).toEqual([]) // echo: nothing to merge or keep aside + expect(hook.result.current.dirty).toBe(true) await hook.act(() => { resolveWrite?.() @@ -1122,16 +1197,4 @@ describe('useNoteDocument', () => { vi.useRealTimers() } }) - - it('keepMine rewrites the file with the buffer', async () => { - const { result, act } = await readyHook() - await act(() => result.current.onEditorChange('# My unsaved edit\n')) - disk = '# Theirs\n' - await act(() => emitChange?.([{ path: 'notes/a.md', kind: 'upsert' }])) - await vi.waitFor(() => expect(result.current.conflict).toBe('# Theirs\n')) - - await act(() => result.current.keepMine()) - await vi.waitFor(() => expect(writes).toContain('# My unsaved edit\n')) - expect(result.current.conflict).toBeNull() - }) }) diff --git a/apps/desktop/src/editor/use-note-document.ts b/apps/desktop/src/editor/use-note-document.ts index 1d22fde0d..1d9686c73 100644 --- a/apps/desktop/src/editor/use-note-document.ts +++ b/apps/desktop/src/editor/use-note-document.ts @@ -1,5 +1,6 @@ import { useCallback, useEffect, useRef, useState } from 'react' -import { readNote, writeNote, type FileChange } from '@reflect/core' +import { createNoteIfAbsent, mergeText, readNote, writeNote, type FileChange } from '@reflect/core' +import { startOperation } from '@/lib/operations.ts' import { useFileChanges } from '@/lib/use-file-changes.ts' import { createDocumentBinding, type DocumentBinding } from './document-binding.ts' import type { NoteEditorHandle } from './note-editor.tsx' @@ -7,6 +8,7 @@ import { createRenameCoordinator } from './rename-coordinator.ts' import { createNoteSession, INITIAL_NOTE_SNAPSHOT, + type ConflictCopy, type NoteSessionSnapshot, } from './note-session.ts' import { checkRoundTrip } from './roundtrip.ts' @@ -26,10 +28,6 @@ export interface NoteDocument extends NoteSessionSnapshot { onEditorChange: (markdown: string) => void /** Wire to the editor's imperative handle (reload/conflict application). */ bindEditor: (handle: NoteEditorHandle | null) => void - /** Resolve a conflict by keeping the buffer (rewrites the file). */ - keepMine: () => void - /** Resolve a conflict by loading the external content (discards the buffer). */ - loadTheirs: () => void /** * Stable identity of the underlying session: increments when a session is * *created*, not when a rename retargets one (Plan 17). Key the editor on @@ -61,6 +59,53 @@ export interface NoteDocumentOptions { missingSeed?: string | undefined } +/** + * Keep edits beside a note as ` (conflict).md` (then `(conflict 2)`, + * … up to the same bound as other claimed note paths) when they could not be + * merged into an external change. Newer + * keystrokes during one reconciliation overwrite the copy it already made, + * checked against what was written: a copy that moved meanwhile (another + * window, another device) is left alone and a fresh sibling is made. + */ +async function keepBesideNote( + path: string, + contents: string, + previous: ConflictCopy | null, + generation: number | null, +): Promise { + if (generation === null) { + throw new Error('no graph generation available for the conflict copy') + } + if (previous !== null) { + try { + await writeNote(previous.path, contents, generation, previous.contents) + return previous.path + } catch { + // The copy changed under us: keep it, and make a fresh one below. + } + } + const slash = path.lastIndexOf('/') + const dot = path.lastIndexOf('.') + const [stem, ext] = dot > slash ? [path.slice(0, dot), path.slice(dot)] : [path, ''] + for (let n = 1; n <= 1000; n += 1) { + const copy = `${stem} (conflict${n === 1 ? '' : ` ${n}`})${ext}` + const outcome = await createNoteIfAbsent(copy, contents, generation) + if (outcome.kind === 'created') { + // Stays until acknowledged: the editor has just swapped to the other + // version, and this line is what says where the replaced text went. + const notice = startOperation('Edits kept beside the note', { + persistent: true, + action: { label: 'OK', run: () => notice.dismiss() }, + }) + notice.warn( + `${path} changed on disk in a way that could not be merged. Your version is at ${copy}.`, + ) + return copy + } + } + throw new Error('no free name for the conflict copy') +} + /** * @param path graph-relative path of the open note * @param generation the open graph's session generation (`GraphInfo.generation`); @@ -77,8 +122,6 @@ export function useNoteDocument( const missingSeed = options?.missingSeed const [snapshot, setSnapshot] = useState(INITIAL_NOTE_SNAPSHOT) const editorRef = useRef(null) - /** Mirrors the snapshot's conflict for non-reactive checks (rename gating). */ - const conflictRef = useRef(null) /** The pane's lifecycle policy object — one per hook instance. */ const [binding] = useState(() => createDocumentBinding()) @@ -111,7 +154,6 @@ export function useNoteDocument( ? createRenameCoordinator({ path, generation: () => generationRef.current, - canFire: () => conflictRef.current === null, }) : null, session: (coordinator) => @@ -128,10 +170,14 @@ export function useNoteDocument( return writeNote(forPath, contents, current, expectedContents) } : null, + mergeText: canWrite ? mergeText : undefined, + copyAside: canWrite + ? (forPath, contents, previous) => + keepBesideNote(forPath, contents, previous, generationRef.current) + : undefined, }, classify: checkRoundTrip, onSnapshot: (next) => { - conflictRef.current = next.conflict setSnapshot(next) }, applyContent: (markdown) => editorRef.current?.setMarkdown(markdown), @@ -215,20 +261,10 @@ export function useNoteDocument( editorRef.current = handle }, []) - const keepMine = useCallback(() => { - binding.session()?.keepMine() - }, [binding]) - - const loadTheirs = useCallback(() => { - binding.session()?.loadTheirs() - }, [binding]) - return { ...snapshot, onEditorChange, bindEditor, - keepMine, - loadTheirs, sessionEpoch: binding.epoch(), } } diff --git a/docs/git-backup-safety.md b/docs/git-backup-safety.md index 49a5840a8..0828da782 100644 --- a/docs/git-backup-safety.md +++ b/docs/git-backup-safety.md @@ -81,8 +81,19 @@ the same marked-up file. ## S8. Edits in an open note are never silently discarded When external content arrives while the buffer has unsaved edits, the -session merges, or parks the conflict and keeps both sides. Leaving the note -with a parked conflict archives the buffer instead of dropping it. - -- Test: `note-session.test.ts` "keepMine rewrites the file even when the - conflict content equals the buffer"; archive-on-dispose pending. +session merges three-way. Disjoint edits apply and keep saving; overlapping +edits are written into the file as labeled markers (the note opens +protected, like a conflicted pull); edits that cannot be merged are kept as +` (conflict).md` beside the note. The buffer is never left in a state +it cannot save from, so no exit path has to rescue it: a final flush that an +external change refuses (the pane closes right after another device wrote +the note) runs the same reconciliation to completion after the session is +disposed. When the copy itself cannot be written (a full disk), the buffer +stays dirty with the error shown, and its next save tries again. + +- Test: `note-session.test.ts` "a clean three-way merge applies silently and + keeps saving", "overlapping edits are written into the file as markers and + open protected", "edits that cannot be merged are kept beside the note", + "a dispose flush refused by an external change still merges the buffer", + "a failed conflict copy keeps the dirty buffer, and the next save retries + it".