From 8d08b058bbef9c8f202ac4a135e11845efd7df0c Mon Sep 17 00:00:00 2001 From: Zef Hemel Date: Wed, 5 Aug 2026 10:56:46 +0200 Subject: [PATCH] Apply detected changes in place instead of full page reload --- client/content_manager.test.ts | 406 ++++++++++++++++++++++ client/content_manager.ts | 207 ++++++++--- client/external_merge.test.ts | 108 ++++++ client/external_merge.ts | 33 ++ client/external_merge_transaction.test.ts | 192 ++++++++++ client/lib/page_meta.test.ts | 19 + client/lib/page_meta.ts | 10 + 7 files changed, 929 insertions(+), 46 deletions(-) create mode 100644 client/content_manager.test.ts create mode 100644 client/external_merge.test.ts create mode 100644 client/external_merge.ts create mode 100644 client/external_merge_transaction.test.ts create mode 100644 client/lib/page_meta.test.ts create mode 100644 client/lib/page_meta.ts diff --git a/client/content_manager.test.ts b/client/content_manager.test.ts new file mode 100644 index 00000000..30df58e7 --- /dev/null +++ b/client/content_manager.test.ts @@ -0,0 +1,406 @@ +import { describe, expect, test, vi } from "vitest"; +import { EditorState, type TransactionSpec } from "@codemirror/state"; +import type { PageMeta } from "@silverbulletmd/silverbullet/type/index"; +import type { Client } from "./client.ts"; + +// content_manager.ts imports codemirror/editor_state.ts for createEditorState +// and externalUpdate. That module (transitively, via lua_widget.ts -> +// widget_sandbox_iframe.ts) calls document.createElement at module scope, so +// it can't load under this project's Node-environment vitest config (no +// jsdom/happy-dom, and adding one is out of scope). Replace it with a +// minimal stand-in exposing the same two names -- content_manager.ts itself, +// the actual subject of these tests, is imported for real, unmocked. +vi.mock("./codemirror/editor_state.ts", async () => { + const { Annotation, EditorState: RealEditorState } = await import( + "@codemirror/state" + ); + return { + createEditorState: ( + _client: unknown, + _pageName: string, + text: string, + _readOnly: boolean, + ) => RealEditorState.create({ doc: text }), + externalUpdate: Annotation.define(), + }; +}); + +const { ContentManager } = await import("./content_manager.ts"); + +// content_manager.ts's enriched-meta refresh (loadPage and, after this +// change, reloadPageContent) reads/writes document.body directly for +// frontmatter-derived page-decoration classes. No jsdom in this Node vitest +// config (see the module mock above), so provide the minimal shape used. +(globalThis as unknown as { document: { body: unknown } }).document = { + body: { + className: "", + removeAttribute(this: { className: string }, name: string) { + if (name === "class") this.className = ""; + }, + }, +}; + +// Minimal stand-in for EditorView: real EditorState + real transaction +// application (state.update), so ChangeSet/annotation semantics are +// genuine -- just no DOM rendering. +function makeEditorViewStub(initialDoc: string) { + let state = EditorState.create({ doc: initialDoc }); + const dispatched: TransactionSpec[] = []; + return { + get state() { + return state; + }, + setState(newState: EditorState) { + state = newState; + }, + dispatch(spec: TransactionSpec) { + dispatched.push(spec); + state = state.update(spec).state; + }, + dispatched, + }; +} + +type ReadPageResult = { text: string; meta: PageMeta }; + +function makeClientStub(opts: { + initialDoc: string; + readPage: () => Promise; + hasFullIndexCompleted?: () => Promise; + getObjectByRef?: () => Promise; +}) { + const editorView = makeEditorViewStub(opts.initialDoc); + const viewState: { + current?: { path: string; meta?: PageMeta }; + unsavedChanges: boolean; + } = { + current: undefined, + unsavedChanges: false, + }; + let currentPathValue = ""; + const dispatchedEvents: { name: string; args: unknown[] }[] = []; + const viewDispatched: { type: string; [key: string]: unknown }[] = []; + + const client = { + editorView, + viewState, + set currentPathValue(v: string) { + currentPathValue = v; + }, + ui: { + viewState, + viewDispatch: (action: { + type: string; + path?: string; + meta?: PageMeta; + }) => { + viewDispatched.push(action); + if (action.type === "page-loaded" && action.path) { + viewState.current = { path: action.path, meta: action.meta }; + } + if (action.type === "update-current-page-meta" && viewState.current) { + viewState.current = { ...viewState.current, meta: action.meta }; + } + }, + }, + space: { + readPage: opts.readPage, + unwatchFile: () => {}, + watchFile: () => {}, + }, + objectIndex: { + hasFullIndexCompleted: opts.hasFullIndexCompleted ?? (async () => false), + getObjectByRef: opts.getObjectByRef ?? (async () => undefined), + }, + widgetCache: { clearPrewarm: () => {} }, + pageMetaAugmenter: { setAugmentation: async () => {} }, + eventHook: { + dispatchEvent: async (name: string, ...args: unknown[]) => { + dispatchedEvents.push({ name, args }); + return []; + }, + }, + isReadOnlyMode: () => false, + dispatchAppEvent: async () => [], + currentPath: () => currentPathValue, + currentName: () => currentPathValue.replace(/\.md$/, ""), + dispatchedEvents, + viewDispatched, + }; + return client; +} + +function pageMeta(lastModified: string): PageMeta { + return { + name: "index", + tags: ["page"], + created: "", + lastModified, + perm: "rw", + } as PageMeta; +} + +describe("ContentManager.loadPage base tracking (regression)", () => { + test("same-page reload merges against the previous disk text, not the newly-fetched one", async () => { + let diskText = "hello world\n"; + let diskModified = "2026-01-01T00:00:00.000"; + const client = makeClientStub({ + initialDoc: "", + readPage: async () => ({ text: diskText, meta: pageMeta(diskModified) }), + }); + client.currentPathValue = "index.md"; + const cm = new ContentManager(client as unknown as Client); + + // Fresh load establishes the base. + await cm.loadPage({ path: "index.md" }, false); + expect(client.editorView.state.sliceDoc()).toBe("hello world\n"); + + // An unsaved local edit sits in the editor -- stale relative to the disk + // change below, exactly the situation "Editor: Reload" or the + // first-sync-complete auto-reload can hit. + client.editorView.dispatch({ + changes: { from: client.editorView.state.doc.length, insert: "LOCAL" }, + }); + expect(client.editorView.state.sliceDoc()).toBe("hello world\nLOCAL"); + + // Disk changes externally while that edit is still unsaved. + diskText = "hello world\nExternal line\n"; + diskModified = "2026-01-01T00:00:05.000"; + + // Same-page reload (loadingDifferentPath stays false: previousPath === + // newPath, both "index.md"). + await cm.loadPage({ path: "index.md" }, false); + + // Must not no-op: if lastKnownDiskText was clobbered to the new disk + // text before this merge runs, base === disk and the diff -- and thus + // this assertion -- would be empty regardless of what changed on disk. + expect(client.editorView.state.sliceDoc()).toContain("External line"); + }); +}); + +describe("ContentManager.reloadPageContent stale-navigation guard (regression)", () => { + test("bails out if the current page changed while the read was in flight", async () => { + let resolveReadPage!: (doc: ReadPageResult) => void; + const pending = new Promise((resolve) => { + resolveReadPage = resolve; + }); + const client = makeClientStub({ + initialDoc: "page A content\n", + readPage: () => pending, + }); + client.currentPathValue = "pageA.md"; + const cm = new ContentManager(client as unknown as Client); + + const reloadPromise = cm.reloadPageContent(); + + // User navigates to a different page while the fetch above is still in + // flight. + client.currentPathValue = "pageB.md"; + client.editorView.setState(EditorState.create({ doc: "page B content\n" })); + + // The stale fetch for page A finally resolves, with page-A-derived + // external content. + resolveReadPage({ + text: "page A content\nEXTERNAL EDIT\n", + meta: pageMeta("2026-01-01T00:00:00.000"), + }); + await reloadPromise; + + // Page B's content must be untouched by page A's stale patch. + expect(client.editorView.state.sliceDoc()).toBe("page B content\n"); + }); + + test("applies normally when the page hasn't changed during the fetch", async () => { + // initialDoc "" matches ContentManager's default lastKnownDiskText, so + // the merge is a clean append -- this test only guards against the + // stale-navigation check above being overzealous, not the merge itself + // (covered by external_merge.test.ts and the loadPage test above). + const client = makeClientStub({ + initialDoc: "", + readPage: async () => ({ + text: "hello\nexternal\n", + meta: pageMeta("2026-01-01T00:00:00.000"), + }), + }); + client.currentPathValue = "index.md"; + const cm = new ContentManager(client as unknown as Client); + + await cm.reloadPageContent(); + + expect(client.editorView.state.sliceDoc()).toBe("hello\nexternal\n"); + }); +}); + +describe("ContentManager.applyExternalPatches monotonicity guard (regression)", () => { + test("drops a stale apply when an older in-flight read resolves after a newer one", async () => { + let resolveOlder!: (r: ReadPageResult) => void; + let resolveNewer!: (r: ReadPageResult) => void; + const olderPromise = new Promise((resolve) => { + resolveOlder = resolve; + }); + const newerPromise = new Promise((resolve) => { + resolveNewer = resolve; + }); + let call = 0; + const client = makeClientStub({ + initialDoc: "", + readPage: () => { + call++; + if (call === 1) { + return Promise.resolve({ + text: "base\n", + meta: pageMeta("2026-01-01T00:00:00.000"), + }); + } + return call === 2 ? olderPromise : newerPromise; + }, + }); + client.currentPathValue = "index.md"; + const cm = new ContentManager(client as unknown as Client); + + // Seed the base (lastKnownDiskText/lastKnownDiskModified) via a fresh load. + await cm.loadPage({ path: "index.md" }, false); + expect(client.editorView.state.sliceDoc()).toBe("base\n"); + + // Two reloads for the same page, in flight concurrently (e.g. a direct + // reloadEditor racing an SSE-triggered reloadPageContent). + const olderReload = cm.reloadPageContent(); + const newerReload = cm.reloadPageContent(); + + // The one reading the newer disk state resolves first. + resolveNewer({ + text: "base\nAGENT V2\n", + meta: pageMeta("2026-01-01T00:00:10.000"), + }); + await newerReload; + expect(client.editorView.state.sliceDoc()).toBe("base\nAGENT V2\n"); + + // The stale (older) read resolves after -- must not revert the editor. + resolveOlder({ + text: "base\nAGENT V1\n", + meta: pageMeta("2026-01-01T00:00:05.000"), + }); + await olderReload; + + expect(client.editorView.state.sliceDoc()).toBe("base\nAGENT V2\n"); + }); +}); + +describe("ContentManager.reloadPageContent editor:pageReloaded notification", () => { + test("dispatches editor:pageReloaded after applying a real external patch", async () => { + let diskText = "hello world\n"; + const client = makeClientStub({ + initialDoc: "", + readPage: async () => ({ + text: diskText, + meta: pageMeta("2026-01-01T00:00:00.000"), + }), + }); + client.currentPathValue = "index.md"; + const cm = new ContentManager(client as unknown as Client); + + await cm.loadPage({ path: "index.md" }, false); + client.dispatchedEvents.length = 0; + + diskText = "hello world\nExternal line\n"; + await cm.reloadPageContent(); + + const reloaded = client.dispatchedEvents.filter( + (e) => e.name === "editor:pageReloaded", + ); + expect(reloaded).toEqual([ + { name: "editor:pageReloaded", args: ["index", "index"] }, + ]); + }); + + test("does not dispatch editor:pageReloaded for a stale patch dropped by the monotonicity guard", async () => { + let resolveOlder!: (r: ReadPageResult) => void; + let resolveNewer!: (r: ReadPageResult) => void; + const olderPromise = new Promise((resolve) => { + resolveOlder = resolve; + }); + const newerPromise = new Promise((resolve) => { + resolveNewer = resolve; + }); + let call = 0; + const client = makeClientStub({ + initialDoc: "", + readPage: () => { + call++; + if (call === 1) { + return Promise.resolve({ + text: "base\n", + meta: pageMeta("2026-01-01T00:00:00.000"), + }); + } + return call === 2 ? olderPromise : newerPromise; + }, + }); + client.currentPathValue = "index.md"; + const cm = new ContentManager(client as unknown as Client); + await cm.loadPage({ path: "index.md" }, false); + + const olderReload = cm.reloadPageContent(); + const newerReload = cm.reloadPageContent(); + + resolveNewer({ + text: "base\nV2\n", + meta: pageMeta("2026-01-01T00:00:10.000"), + }); + await newerReload; + client.dispatchedEvents.length = 0; + + // Resolves after the newer one, with older content -- dropped by the + // monotonicity guard in applyExternalPatches. Nothing changed in the + // editor, so no notification should fire either. + resolveOlder({ + text: "base\nV1\n", + meta: pageMeta("2026-01-01T00:00:05.000"), + }); + await olderReload; + + expect( + client.dispatchedEvents.some((e) => e.name === "editor:pageReloaded"), + ).toBe(false); + }); + + test("refreshes viewState.current.meta and body page-decoration classes from the object index", async () => { + // Frontmatter (and thus the indexed page decoration) only appears after + // the external edit below -- this is what the initial loadPage sees vs. + // what the live path must pick up without requiring re-navigation. + let diskText = "hello world\n"; + const metaBeforeEdit = pageMeta("2026-01-01T00:00:00.000"); + const metaAfterEdit = { + ...pageMeta("2026-01-01T00:00:05.000"), + pageDecoration: { cssClasses: ["journal-page"] }, + } as PageMeta; + let enrichedMeta = metaBeforeEdit; + const client = makeClientStub({ + initialDoc: "", + readPage: async () => ({ + text: diskText, + meta: pageMeta("2026-01-01T00:00:00.000"), + }), + hasFullIndexCompleted: async () => true, + getObjectByRef: async () => enrichedMeta, + }); + client.currentPathValue = "index.md"; + const cm = new ContentManager(client as unknown as Client); + + await cm.loadPage({ path: "index.md" }, false); + expect(client.viewState.current?.meta).toEqual(metaBeforeEdit); + + diskText = "hello world\nExternal line\n"; + enrichedMeta = metaAfterEdit; + await cm.reloadPageContent(); + + // Without navigating away and back, viewState.current.meta must already + // reflect the post-edit frontmatter, and the body decoration classes + // derived from it. + expect(client.viewState.current?.meta).toEqual(metaAfterEdit); + expect( + (globalThis as unknown as { document: { body: { className: string } } }) + .document.body.className, + ).toBe("journal-page"); + }); +}); diff --git a/client/content_manager.ts b/client/content_manager.ts index 6fc90a99..427a594d 100644 --- a/client/content_manager.ts +++ b/client/content_manager.ts @@ -22,7 +22,10 @@ import { createEditorState, externalUpdate, } from "./codemirror/editor_state.ts"; +import { externalSource } from "./codemirror/external_presence.ts"; import { diffAndPrepareChanges } from "./codemirror/cm_util.ts"; +import { computeExternalChanges } from "./external_merge.ts"; +import { parsePageMetaLastModified } from "./lib/page_meta.ts"; import { DocumentEditor } from "./document_editor.ts"; import { fsEndpoint } from "./spaces/constants.ts"; import { parseMarkdown } from "./markdown_parser/parser.ts"; @@ -41,6 +44,14 @@ export class ContentManager { documentEditor: DocumentEditor | null = null; saveTimeout?: ReturnType; private scrollRestoreCleanup?: () => void; + // Last content known to be on disk (base for 3-way external merges) + private lastKnownDiskText = ""; + // lastModified backing lastKnownDiskText, used to reject an apply whose + // fetched content is older than what's already applied (out-of-order + // resolution of overlapping reloads for the same page) + private lastKnownDiskModified?: number; + // lastModified of our own most recent write, for echo suppression + lastSavedHash?: number; debouncedUpdateEvent = throttle(() => { this.client.eventHook .dispatchEvent("editor:updated") @@ -95,12 +106,15 @@ export class ContentManager { "editor:pageSaving", this.client.currentName(), ); + const text = this.client.editorView.state.sliceDoc(0); this.client.space - .writePage( - this.client.currentName(), - this.client.editorView.state.sliceDoc(0), - ) + .writePage(this.client.currentName(), text) .then(async (meta) => { + this.lastKnownDiskText = text; + this.lastSavedHash = parsePageMetaLastModified( + meta.lastModified, + ); + this.lastKnownDiskModified = this.lastSavedHash; this.client.ui.viewDispatch({ type: "page-saved" }); await this.client.dispatchAppEvent( "editor:pageSaved", @@ -337,6 +351,8 @@ export class ContentManager { } } + this.lastSavedHash = parsePageMetaLastModified(doc.meta.lastModified); + // This could create an invalid editor state, but that doesn't matter, we'll update it later this.switchToPageEditor(); @@ -355,42 +371,15 @@ export class ContentManager { path: path, }); - // Fetch the meta which includes the possibly indexed stuff, like page - // decorations - if (await this.client.objectIndex.hasFullIndexCompleted()) { - try { - const enrichedMeta = - (await this.client.objectIndex.getObjectByRef( - pageName, - "page", - pageName, - )) ?? doc.meta; - - const body = document.body; - body.removeAttribute("class"); - - if (enrichedMeta.pageDecoration?.cssClasses) { - body.className = enrichedMeta.pageDecoration.cssClasses - .join(" ") - .replaceAll(/[^a-zA-Z0-9-_ ]/g, ""); - } - - this.client.ui.viewDispatch({ - type: "update-current-page-meta", - meta: enrichedMeta, - }); - - // Trigger editor re-render to update Lua widgets with the new metadata - this.client.editorView.dispatch({}); - } catch (e: any) { - console.log( - `There was an error trying to fetch enriched metadata: ${e.message}`, - ); - } - } + await this.refreshCurrentPageMeta(pageName, doc.meta); // When loading a different page OR if the page is read-only (in which case we don't want to apply local patches, because there's no point) if (loadingDifferentPath || doc.meta.perm === "ro") { + // Fresh state, nothing to diff against yet: doc.text *is* the new base. + this.lastKnownDiskText = doc.text; + this.lastKnownDiskModified = parsePageMetaLastModified( + doc.meta.lastModified, + ); const editorState = createEditorState( this.client, pageName, @@ -399,8 +388,13 @@ export class ContentManager { ); this.client.editorView.setState(editorState); } else { - // Just apply minimal patches so that the cursor is preserved - this.applyExternalPatches(doc.text); + // Same-page reload: applyExternalPatches diffs against the base from + // the *previous* load (this.lastKnownDiskText, untouched above) and + // updates it to doc.text itself. + this.applyExternalPatches( + doc.text, + parsePageMetaLastModified(doc.meta.lastModified), + ); } this.client.space.watchFile(path); @@ -510,16 +504,137 @@ export class ContentManager { }); } - // Like setEditorText, but marks the transaction as an external update (e.g. - // a page re-fetch from storage) so the save-on-change handler skips it and - // we avoid an immediate re-save loop. - private applyExternalPatches(newText: string) { + // Applies an external (storage-side) content change as a minimal, + // cursor-preserving transaction: a 3-way merge against the last known + // on-disk text so unsaved local edits survive. The transaction is marked + // externalUpdate (skips the save-on-change handler) and isolated in undo + // history, so a single undo reverts exactly this external change. + // Returns whether a change was actually dispatched, so callers can skip + // follow-up work (meta refresh, notifications) for a dropped/no-op apply. + private applyExternalPatches( + newText: string, + modified: number | undefined, + source = "external", + ): boolean { + if ( + modified !== undefined && + this.lastKnownDiskModified !== undefined && + modified < this.lastKnownDiskModified + ) { + // Two reloads for this page were in flight and this older one resolved + // last (e.g. a direct reloadEditor racing an SSE-triggered + // reloadPageContent). Applying it would revert already-applied, + // newer content. + console.log( + "Dropping stale external patch, older than already-applied content", + { source, modified, lastKnownDiskModified: this.lastKnownDiskModified }, + ); + return false; + } const currentText = this.client.editorView.state.sliceDoc(); - const allChanges = diffAndPrepareChanges(currentText, newText); + const changes = computeExternalChanges( + this.lastKnownDiskText, + newText, + currentText, + ); + this.lastKnownDiskText = newText; + this.lastKnownDiskModified = modified; + if (changes.empty) { + return false; + } this.client.editorView.dispatch({ - changes: allChanges, - annotations: [isolateHistory.of("full"), externalUpdate.of(true)], + changes, + annotations: [ + isolateHistory.of("full"), + externalUpdate.of(true), + externalSource.of(source), + ], }); + return true; + } + + // Fetch the current page's content from storage and apply it as an + // external patch -- the live-edit path (no full loadPage, no editor state + // rebuild). Still refreshes enriched page meta and dispatches + // editor:pageReloaded when a real change was applied, so plugs and + // frontmatter-derived UI (page decorations) stay in sync with the file + // that changed on disk, per the documented contract of that event. + async reloadPageContent(source = "external"): Promise { + const path = this.client.currentPath(); + // Defensive, not currently reachable from the file:changed listener + // (which already gates on isMarkdownPath before calling in): kept so + // this method stays safe to call directly with an unchecked path, which + // future callers (e.g. an SSE push handler) may do. + if (!isMarkdownPath(path)) { + return this.reloadEditor(); + } + const doc = await this.client.space.readPage(getNameFromPath(path)); + // The user may have navigated to a different page while this fetch was + // in flight. Applying now would diff/merge page-A content against + // whatever page is currently open -- bail out rather than corrupt it. + if (this.client.currentPath() !== path) { + return; + } + const pageName = getNameFromPath(path); + const applied = this.applyExternalPatches( + doc.text, + parsePageMetaLastModified(doc.meta.lastModified), + source, + ); + if (!applied) { + return; + } + + await this.refreshCurrentPageMeta(pageName, doc.meta); + + // Note: dispatched asynchronously deliberately (not waiting for + // results), matching the full-reload path in loadPage. + this.client.eventHook + .dispatchEvent("editor:pageReloaded", pageName, pageName) + .catch(console.error); + } + + // Re-fetches enriched (indexed) page meta -- e.g. frontmatter-derived page + // decorations -- and pushes it into viewState.current.meta and the body's + // decoration classes. Shared by loadPage and the live reloadPageContent + // path so an external edit's frontmatter changes show up without + // requiring navigation away and back. + private async refreshCurrentPageMeta( + pageName: string, + fallbackMeta: PageMeta, + ): Promise { + if (!(await this.client.objectIndex.hasFullIndexCompleted())) { + return; + } + try { + const enrichedMeta = + (await this.client.objectIndex.getObjectByRef( + pageName, + "page", + pageName, + )) ?? fallbackMeta; + + const body = document.body; + body.removeAttribute("class"); + + if (enrichedMeta.pageDecoration?.cssClasses) { + body.className = enrichedMeta.pageDecoration.cssClasses + .join(" ") + .replaceAll(/[^a-zA-Z0-9-_ ]/g, ""); + } + + this.client.ui.viewDispatch({ + type: "update-current-page-meta", + meta: enrichedMeta, + }); + + // Trigger editor re-render to update Lua widgets with the new metadata + this.client.editorView.dispatch({}); + } catch (e: any) { + console.log( + `There was an error trying to fetch enriched metadata: ${e.message}`, + ); + } } private navigateWithinPage(pageState: LocationState) { diff --git a/client/external_merge.test.ts b/client/external_merge.test.ts new file mode 100644 index 00000000..58845bb6 --- /dev/null +++ b/client/external_merge.test.ts @@ -0,0 +1,108 @@ +import { describe, expect, it } from "vitest"; +import { Text } from "@codemirror/state"; +import { computeExternalChanges } from "./external_merge.ts"; + +function apply( + current: string, + cs: ReturnType, +): string { + return cs.apply(Text.of(current.split("\n"))).toString(); +} + +describe("computeExternalChanges", () => { + it("applies external change to a pristine doc", () => { + const base = "Hello world\n"; + const disk = "Hello world\nExternal line\n"; + const cs = computeExternalChanges(base, disk, base); + expect(apply(base, cs)).toBe(disk); + }); + + it("returns an empty set when disk matches current (echo)", () => { + const base = "One\n"; + const disk = "One\nTwo\n"; + const cs = computeExternalChanges(base, disk, disk); + expect(cs.empty).toBe(true); + expect(apply(disk, cs)).toBe(disk); + }); + + it("preserves non-overlapping local edits", () => { + const base = "alpha\nbeta\ngamma\n"; + const disk = "alpha\nbeta\ngamma\nexternal\n"; // external append + const current = "ALPHA\nbeta\ngamma\n"; // local edit at top + const cs = computeExternalChanges(base, disk, current); + expect(apply(current, cs)).toBe("ALPHA\nbeta\ngamma\nexternal\n"); + }); + + it("keeps local insertion when external edit touches an adjacent region", () => { + const base = "one two three"; + const disk = "one 2 three"; // external replaced "two" + const current = "one two three four"; // local appended + const cs = computeExternalChanges(base, disk, current); + expect(apply(current, cs)).toBe("one 2 three four"); + }); + + it("survives external and local edits to the same region without crashing", () => { + const base = "shared text here"; + const disk = "shared TEXT here"; // external + const current = "shared texts here"; // local, overlapping + const cs = computeExternalChanges(base, disk, current); + const merged = apply(current, cs); + // Exact outcome of a direct overlap is defined by ChangeSet.map; the + // invariants we require: no throw, local "texts" influence not silently + // reverted to base, and the result contains the unmodified suffix. + expect(merged).toContain(" here"); + }); + + it("handles disk == base with local edits (nothing external to do)", () => { + const base = "stable\n"; + const current = "stable\nlocal\n"; + const cs = computeExternalChanges(base, base, current); + expect(cs.empty).toBe(true); + expect(apply(current, cs)).toBe(current); + }); + + it("keeps a local insertion right at the edge of an external replacement", () => { + const base = "shared text here"; + const disk = "shared TEXT here"; // external uppercases "text" + const current = "shared texty here"; // local insertion of "y" right after "text" + const cs = computeExternalChanges(base, disk, current); + expect(apply(current, cs)).toBe("shared TEXTy here"); + }); + + it("does not silently drop local content when a local edit fully replaces an externally-touched word", () => { + const base = "shared text here"; + const disk = "shared TEXT here"; // external uppercases "text" + const current = "shared banana here"; // local replaces "text" with "banana" + const cs = computeExternalChanges(base, disk, current); + const merged = apply(current, cs); + // Direct overlaps of this kind are inherently ambiguous for a diff-based + // 3-way merge (see design note); we only require that neither side's + // content is silently discarded and the surrounding text is intact. + expect(merged).toContain("banana"); + expect(merged).toContain("shared "); + expect(merged).toContain(" here"); + }); + + it("applies a clean external change on top of an unrelated prior local edit", () => { + const base = "line1\nline2\nline3\n"; + const disk = "line1\nline2\nline3\nline4\n"; // external append + const current = "line1\nLINE2\nline3\n"; // unrelated local edit in the middle + const cs = computeExternalChanges(base, disk, current); + expect(apply(current, cs)).toBe("line1\nLINE2\nline3\nline4\n"); + }); + + it("applies multiple sequential external changes correctly (base tracking)", () => { + const base1 = "start\n"; + const disk1 = "start\nmid\n"; + const current1 = base1; + const cs1 = computeExternalChanges(base1, disk1, current1); + const afterFirst = apply(current1, cs1); + expect(afterFirst).toBe(disk1); + + // Second external change is diffed against the *new* base (disk1), not + // the original base1 - simulating ContentManager updating its tracked base. + const disk2 = "start\nmid\nend\n"; + const cs2 = computeExternalChanges(disk1, disk2, afterFirst); + expect(apply(afterFirst, cs2)).toBe(disk2); + }); +}); diff --git a/client/external_merge.ts b/client/external_merge.ts new file mode 100644 index 00000000..ae070774 --- /dev/null +++ b/client/external_merge.ts @@ -0,0 +1,33 @@ +import { ChangeSet } from "@codemirror/state"; +import { diffAndPrepareChanges } from "./codemirror/cm_util.ts"; + +/** + * Three-way merge for externally-changed page content. + * + * `base` is the last known on-disk text (tracked by ContentManager), `disk` + * is the new on-disk text, and `current` is the editor doc (possibly holding + * unsaved local edits relative to base). Both diffs are taken against base; + * the external change set is mapped through the local one so local edits are + * preserved. When an external and a local edit touch the same range, neither + * is discarded: both fragments land in the result, concatenated in position + * order (see the "does not silently drop local content..." test in + * external_merge.test.ts for the exact shape) rather than one overwriting + * the other. The returned ChangeSet applies to `current`. + */ +export function computeExternalChanges( + base: string, + disk: string, + current: string, +): ChangeSet { + if (disk === current) { + return ChangeSet.empty(current.length); + } + const external = ChangeSet.of(diffAndPrepareChanges(base, disk), base.length); + if (current === base) { + return external; + } + const local = ChangeSet.of(diffAndPrepareChanges(base, current), base.length); + // before: true keeps a local insertion on the user's side of the merge + // when it sits at the same position as an external edit. + return external.map(local, true); +} diff --git a/client/external_merge_transaction.test.ts b/client/external_merge_transaction.test.ts new file mode 100644 index 00000000..bd0dd39a --- /dev/null +++ b/client/external_merge_transaction.test.ts @@ -0,0 +1,192 @@ +import { describe, expect, test } from "vitest"; +import { history, isolateHistory, redo, undo } from "@codemirror/commands"; +import { EditorState, Transaction } from "@codemirror/state"; +import { computeExternalChanges } from "./external_merge.ts"; +import { + externalSource, + externalUndoField, +} from "./codemirror/external_presence.ts"; + +// content_manager.ts itself pulls in the full editor extension chain +// (codemirror/editor_state.ts -> lua_widget.ts -> widget_sandbox_iframe.ts), +// which touches `document` at module scope and can't load under Node/vitest. +// These tests instead exercise the same CodeMirror transaction shape that +// ContentManager.applyExternalPatches dispatches -- isolateHistory.of("full") +// plus a changeset from computeExternalChanges -- against a headless +// EditorState, to verify the undo-isolation guarantee the feature promises. + +describe("external patch transaction shape (headless EditorState)", () => { + function dispatchExternalPatch( + state: EditorState, + base: string, + disk: string, + ): EditorState { + const current = state.sliceDoc(); + const changes = computeExternalChanges(base, disk, current); + return state.update({ + changes, + annotations: [isolateHistory.of("full")], + }).state; + } + + test("undo inverts exactly the external change, even when adjacent to local typing", () => { + // Without isolateHistory, CodeMirror's history groups adjacent, + // userEvent-less transactions issued within its group-delay window into + // a single undo step (see @codemirror/commands HistoryState.addChanges / + // isAdjacent) -- exactly what would happen here, since the external + // append lands right at the end of what the user just typed. This is + // the scenario isolateHistory.of("full") exists to prevent. + let state = EditorState.create({ + doc: "Hello", + extensions: [history()], + }); + + // User types locally (goes into normal history) + state = state.update({ + changes: { from: 5, insert: " typed" }, + }).state; + expect(state.sliceDoc()).toBe("Hello typed"); + + // An external write appends "!" right where the local insertion starts -- + // computeExternalChanges' `before: true` mapping keeps the local + // insertion on the user's side, landing the external insert immediately + // adjacent to it once mapped onto the current doc. + state = dispatchExternalPatch(state, "Hello", "Hello!"); + expect(state.sliceDoc()).toBe("Hello! typed"); + + // A command's "target" only needs { state, dispatch }, so we can drive + // undo() headlessly by threading `state` through a small local stub. + const target = { + get state() { + return state; + }, + dispatch: (tr: { state: EditorState }) => { + state = tr.state; + }, + }; + + // One undo should revert only the external change... + expect(undo(target as any)).toBe(true); + expect(state.sliceDoc()).toBe("Hello typed"); + + // ...and a second undo reverts the user's own typing + expect(undo(target as any)).toBe(true); + expect(state.sliceDoc()).toBe("Hello"); + }); + + test("echo (disk matches current) yields an empty changeset, nothing to dispatch", () => { + const base = "Hello world\n"; + const disk = "Hello world\nExternal line\n"; + // Simulate our own write already having landed in the editor (echo) + const current = disk; + const changes = computeExternalChanges(base, disk, current); + expect(changes.empty).toBe(true); + }); +}); + +describe("undo/redo of an external edit preserves the user's cursor (regression)", () => { + // Mirrors ContentManager.applyExternalPatches's actual transaction shape + // exactly, including externalSource, which externalUndoField needs in + // order to track the edit. + function dispatchExternalPatch( + state: EditorState, + base: string, + disk: string, + ): EditorState { + const current = state.sliceDoc(); + const changes = computeExternalChanges(base, disk, current); + return state.update({ + changes, + annotations: [isolateHistory.of("full"), externalSource.of("external")], + }).state; + } + + // Simulates client/codemirror/external_presence.ts's externalUndoCursorFix: + // after a transaction lands, if the field flagged a correction, apply it + // as an immediate follow-up transaction, exactly like the production + // EditorView.updateListener does. + function applyPendingCorrection(state: EditorState): EditorState { + const { correction } = state.field(externalUndoField); + if (!correction) return state; + return state.update({ + selection: correction, + annotations: Transaction.addToHistory.of(false), + }).state; + } + + test("undo after the cursor has moved keeps the cursor where the user left it, not where it was before the external edit", () => { + let state = EditorState.create({ + doc: "Hello world\n", + extensions: [history(), externalUndoField], + }); + + // Page just loaded: cursor sits at 0, untouched. + expect(state.selection.main.head).toBe(0); + + // External write lands while the cursor is still at 0. + state = dispatchExternalPatch( + state, + "Hello world\n", + "Hello world\nExternal line\n", + ); + expect(state.sliceDoc()).toBe("Hello world\nExternal line\n"); + expect(state.selection.main.head).toBe(0); + + // *Then* the user moves the cursor -- the step a naive regression test + // (or the existing e2e's click-right-before-undo) skips, which is + // exactly why it missed this bug. + state = state.update({ selection: { anchor: 6 } }).state; + expect(state.selection.main.head).toBe(6); + + const target = { + get state() { + return state; + }, + dispatch: (tr: { state: EditorState }) => { + state = tr.state; + state = applyPendingCorrection(state); + }, + }; + + expect(undo(target as any)).toBe(true); + expect(state.sliceDoc()).toBe("Hello world\n"); + // The bug: CodeMirror's default undo would restore the selection + // recorded before the external edit landed, i.e. position 0. + expect(state.selection.main.head).toBe(6); + + // Redo brings the edit back and, symmetrically, must leave the cursor + // wherever the user has it now rather than jumping elsewhere. + state = state.update({ selection: { anchor: 3 } }).state; + expect(redo(target as any)).toBe(true); + expect(state.sliceDoc()).toBe("Hello world\nExternal line\n"); + expect(state.selection.main.head).toBe(3); + }); + + test("undo of the user's own edit still restores the pre-edit selection", () => { + let state = EditorState.create({ + doc: "Hello", + extensions: [history(), externalUndoField], + }); + state = state.update({ changes: { from: 5, insert: " typed" } }).state; + expect(state.sliceDoc()).toBe("Hello typed"); + + // Cursor moves away after typing, before undo -- same shape as the + // external-edit scenario above, but here the "jump back to where you + // were before the edit" behaviour is correct and must survive. + state = state.update({ selection: { anchor: 8 } }).state; + + const target = { + get state() { + return state; + }, + dispatch: (tr: { state: EditorState }) => { + state = tr.state; + state = applyPendingCorrection(state); + }, + }; + + expect(undo(target as any)).toBe(true); + expect(state.sliceDoc()).toBe("Hello"); + expect(state.selection.main.head).toBe(0); + }); +}); diff --git a/client/lib/page_meta.test.ts b/client/lib/page_meta.test.ts new file mode 100644 index 00000000..812cd923 --- /dev/null +++ b/client/lib/page_meta.test.ts @@ -0,0 +1,19 @@ +import { describe, expect, test } from "vitest"; +import { localDateString } from "@silverbulletmd/silverbullet/lib/dates"; +import { parsePageMetaLastModified } from "./page_meta.ts"; + +describe("parsePageMetaLastModified", () => { + test("round-trips a PageMeta.lastModified string back to the original epoch ms", () => { + // PageMeta.lastModified is built from a numeric FileMeta.lastModified via + // localDateString (see space.ts:fileMetaToPageMeta) -- this is the same + // conversion echo suppression depends on to compare against the numeric + // hash carried by the file:changed event. + const epochMs = Date.parse("2026-08-04T10:20:30.456"); + const asStoredString = localDateString(new Date(epochMs)); + expect(parsePageMetaLastModified(asStoredString)).toBe(epochMs); + }); + + test("returns undefined for an empty string (new/unsaved page)", () => { + expect(parsePageMetaLastModified("")).toBeUndefined(); + }); +}); diff --git a/client/lib/page_meta.ts b/client/lib/page_meta.ts new file mode 100644 index 00000000..4231bc79 --- /dev/null +++ b/client/lib/page_meta.ts @@ -0,0 +1,10 @@ +// PageMeta.lastModified is indexed as a local-time string (see +// space.ts:fileMetaToPageMeta / localDateString), not the epoch number the +// wire-level FileMeta and file:changed event hashes use. Parsing it back +// recovers a comparable number; a Date-Time string without a zone offset is +// parsed as local time per the ES spec, which is exactly how it was built. +export function parsePageMetaLastModified( + lastModified: string, +): number | undefined { + return lastModified ? Date.parse(lastModified) || undefined : undefined; +}