diff --git a/client/client.ts b/client/client.ts index 20f0578d..cf7ee96a 100644 --- a/client/client.ts +++ b/client/client.ts @@ -403,43 +403,21 @@ export class Client { this.space = space; - let lastSaveTimestamp: number | undefined; - - const updateLastSaveTimestamp = () => { - lastSaveTimestamp = Date.now(); - }; - - this.eventHook.addLocalListener( - "editor:pageSaving", - updateLastSaveTimestamp, - ); - - this.eventHook.addLocalListener( - "editor:documentSaving", - updateLastSaveTimestamp, - ); - this.eventHook.addLocalListener( "file:changed", - (path: string, oldHash: number, newHash: number) => { + (path: string, oldHash: number, _newHash: number, ownWrite: boolean) => { if ( !this.space.watchInterval || this.currentPath() !== path || - oldHash === undefined + oldHash === undefined || + // our own save landing, not somebody else's edit + ownWrite ) { return; } if (isMarkdownPath(path)) { - // Live path: precise echo suppression by our own last write's hash; - // content-level no-op inside applyExternalPatches covers the rest - if (newHash === this.contentManager.lastSavedHash) { - return; - } this.contentManager.reloadPageContent().catch(console.error); - } else if ( - !lastSaveTimestamp || - lastSaveTimestamp < Date.now() - 5000 - ) { + } else { // Documents (non-markdown) keep the reload path this.ui.flashNotification("Document changed elsewhere, reloading"); void this.reloadEditor(); diff --git a/client/content_manager.test.ts b/client/content_manager.test.ts index 30df58e7..a2ca77ee 100644 --- a/client/content_manager.test.ts +++ b/client/content_manager.test.ts @@ -66,6 +66,7 @@ type ReadPageResult = { text: string; meta: PageMeta }; function makeClientStub(opts: { initialDoc: string; readPage: () => Promise; + writePage?: (name: string, text: string) => Promise; hasFullIndexCompleted?: () => Promise; getObjectByRef?: () => Promise; }) { @@ -89,6 +90,7 @@ function makeClientStub(opts: { }, ui: { viewState, + flashNotification: () => {}, viewDispatch: (action: { type: string; path?: string; @@ -105,6 +107,8 @@ function makeClientStub(opts: { }, space: { readPage: opts.readPage, + writePage: + opts.writePage ?? (async () => pageMeta("2026-01-01T00:00:00.000")), unwatchFile: () => {}, watchFile: () => {}, }, @@ -404,3 +408,116 @@ describe("ContentManager.reloadPageContent editor:pageReloaded notification", () ).toBe("journal-page"); }); }); + +// The base only moves forward when a write's *response* arrives. A read that +// resolves before it sees content newer than the base it gets diffed against, +// and the merge re-inserts the difference into a document that already has it. +describe("ContentManager merge base vs. in-flight writes (regression: text duplicates while typing)", () => { + // Lets the debounced save() timeout fire and promise chains settle. + const flush = () => new Promise((resolve) => setTimeout(resolve, 0)); + + function typeInto( + client: ReturnType, + appended: string, + ) { + client.editorView.dispatch({ + changes: { from: client.editorView.state.doc.length, insert: appended }, + }); + client.viewState.unsavedChanges = true; + } + + test("a fetch that returns content from a save still in flight does not duplicate it", async () => { + let resolveRead!: (r: ReadPageResult) => void; + let resolveWrite!: (meta: PageMeta) => void; + let readCall = 0; + const client = makeClientStub({ + initialDoc: "", + readPage: () => { + readCall++; + if (readCall === 1) { + return Promise.resolve({ + text: "one\n", + meta: pageMeta("2026-01-01T00:00:00.000"), + }); + } + return new Promise((resolve) => { + resolveRead = resolve; + }); + }, + writePage: () => + new Promise((resolve) => { + resolveWrite = resolve; + }), + }); + client.currentPathValue = "index.md"; + const cm = new ContentManager(client as unknown as Client); + + await cm.loadPage({ path: "index.md" }, false); + typeInto(client, "two"); + + // A foreign change is reported, so a fetch starts. No write in flight yet. + const reload = cm.reloadPageContent(); + + // Only now does the autosave fire, and the user keeps typing after it. + void cm.save(true); + await flush(); + typeInto(client, "three"); + + // The fetch returns what that save is putting on disk... + resolveRead({ + text: "one\ntwo", + meta: pageMeta("2026-01-01T00:00:05.000"), + }); + await flush(); + // ...and only then does the write's response carry the base forward. + resolveWrite(pageMeta("2026-01-01T00:00:05.000")); + await reload; + + expect(client.editorView.state.sliceDoc()).toBe("one\ntwothree"); + }); + + test("two saves acknowledged out of order leave the base on the newer one", async () => { + // save() does not wait for the previous write, and nothing orders the two + // responses. + const resolvers: ((meta: PageMeta) => void)[] = []; + let diskText = "one\n"; + let diskModified = "2026-01-01T00:00:00.000"; + const client = makeClientStub({ + initialDoc: "", + readPage: async () => ({ text: diskText, meta: pageMeta(diskModified) }), + writePage: () => + new Promise((resolve) => { + resolvers.push(resolve); + }), + }); + client.currentPathValue = "index.md"; + const cm = new ContentManager(client as unknown as Client); + + await cm.loadPage({ path: "index.md" }, false); + + typeInto(client, "two"); + void cm.save(true); + await flush(); + typeInto(client, "three"); + void cm.save(true); + await flush(); + expect(resolvers).toHaveLength(2); + + // The later write is acknowledged first; the earlier one lands after. + resolvers[1](pageMeta("2026-01-01T00:00:09.000")); + await flush(); + resolvers[0](pageMeta("2026-01-01T00:00:05.000")); + await flush(); + + // Keeps the document ahead of disk, so the merge below is a real one + // rather than a same-text no-op. + typeInto(client, "four"); + + // With the base dragged back to the earlier write, this re-inserts "three". + diskText = "one\ntwothree"; + diskModified = "2026-01-01T00:00:09.000"; + await cm.reloadPageContent(); + + expect(client.editorView.state.sliceDoc()).toBe("one\ntwothreefour"); + }); +}); diff --git a/client/content_manager.ts b/client/content_manager.ts index 427a594d..26b09c80 100644 --- a/client/content_manager.ts +++ b/client/content_manager.ts @@ -50,8 +50,8 @@ export class ContentManager { // 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; + // Resolves once the in-flight write has updated the base above + private pendingWrite?: Promise; debouncedUpdateEvent = throttle(() => { this.client.eventHook .dispatchEvent("editor:updated") @@ -107,14 +107,28 @@ export class ContentManager { this.client.currentName(), ); const text = this.client.editorView.state.sliceDoc(0); - this.client.space - .writePage(this.client.currentName(), text) + const writePromise = this.client.space.writePage( + this.client.currentName(), + text, + ); + // Separate from the promise save() returns: waiters need the base + // updated, not the events and meta fetch that follow it. + const baseUpdated = writePromise.then( + (meta) => + this.adoptOwnWriteAsBase( + text, + parsePageMetaLastModified(meta.lastModified), + ), + () => {}, // errors are reported by the .catch() below + ); + this.pendingWrite = baseUpdated; + void baseUpdated.then(() => { + if (this.pendingWrite === baseUpdated) { + this.pendingWrite = undefined; + } + }); + writePromise .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", @@ -161,6 +175,23 @@ export class ContentManager { }); } + /** + * Records the text we just wrote as the new base, unless a newer state was + * adopted while this write was in flight: dragging the base backwards would + * make the next merge apply that newer change a second time. + */ + private adoptOwnWriteAsBase(text: string, modified: number | undefined) { + if ( + modified !== undefined && + this.lastKnownDiskModified !== undefined && + modified < this.lastKnownDiskModified + ) { + return; + } + this.lastKnownDiskText = text; + this.lastKnownDiskModified = modified; + } + async reloadEditor() { if (!this.client.systemReady) return; @@ -351,8 +382,6 @@ 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(); @@ -569,6 +598,14 @@ export class ContentManager { return this.reloadEditor(); } const doc = await this.client.space.readPage(getNameFromPath(path)); + // A write that finished during the read left the base behind what the read + // returned, so merging now would re-insert our own text. After the read, + // not before: it's writes overlapping the *fetch* that strand the base. + // Not covered by ownWrite -- a write overlapping another space operation + // dispatches no event of its own, and surfaces later via a probe instead. + if (this.pendingWrite) { + await this.pendingWrite; + } // 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. diff --git a/client/spaces/evented_space_primitives.test.ts b/client/spaces/evented_space_primitives.test.ts new file mode 100644 index 00000000..4e9a8da0 --- /dev/null +++ b/client/spaces/evented_space_primitives.test.ts @@ -0,0 +1,84 @@ +import { describe, expect, test } from "vitest"; +import type { FileMeta } from "@silverbulletmd/silverbullet/type/index"; +import { EventedSpacePrimitives } from "./evented_space_primitives.ts"; +import type { SpacePrimitives } from "./space_primitives.ts"; +import type { EventHook } from "../plugos/hooks/event.ts"; +import type { DataStore } from "../data/datastore.ts"; + +function meta(name: string, lastModified: number): FileMeta { + return { + name, + lastModified, + created: lastModified, + contentType: "text/markdown", + size: 0, + perm: "rw", + }; +} + +function setup(lastModified = 2000) { + const events: { name: string; args: unknown[] }[] = []; + const wrapped = { + readFile: async (path: string) => ({ + data: new Uint8Array(), + meta: meta(path, lastModified), + }), + writeFile: async (path: string) => meta(path, lastModified), + getFileMeta: async (path: string) => meta(path, lastModified), + } as unknown as SpacePrimitives; + const store = new Map(); + const ds = { + get: async (key: string[]) => store.get(key.join("/")), + set: async (key: string[], value: unknown) => { + store.set(key.join("/"), value); + }, + } as unknown as DataStore; + const eventHook = { + dispatchEvent: async (name: string, ...args: unknown[]) => { + events.push({ name, args }); + return []; + }, + } as unknown as EventHook; + const sp = new EventedSpacePrimitives(wrapped, eventHook, ds); + return { sp, events }; +} + +const fileChanged = (events: { name: string; args: unknown[] }[]) => + events.filter((e) => e.name === "file:changed"); + +// file:changed fires from inside writeFile, before it returns, so ownWrite is +// a listener's only way to tell its own write from somebody else's. +describe("EventedSpacePrimitives file:changed ownWrite flag", () => { + test("a write of our own is flagged as such", async () => { + const { sp, events } = setup(); + await sp.enable(); + + await sp.writeFile("index.md", new Uint8Array()); + + expect(fileChanged(events).map((e) => e.args)).toEqual([ + ["index.md", undefined, 2000, true], + ]); + }); + + test("a change merely observed by a metadata probe is not", async () => { + const { sp, events } = setup(); + await sp.enable(); + + await sp.getFileMeta("index.md"); + + expect(fileChanged(events).map((e) => e.args)).toEqual([ + ["index.md", undefined, 2000, false], + ]); + }); + + test("a change merely observed by a read is not", async () => { + const { sp, events } = setup(); + await sp.enable(); + + await sp.readFile("index.md"); + + expect(fileChanged(events).map((e) => e.args)).toEqual([ + ["index.md", undefined, 2000, false], + ]); + }); +}); diff --git a/client/spaces/evented_space_primitives.ts b/client/spaces/evented_space_primitives.ts index 5d56aa1b..aca653a1 100644 --- a/client/spaces/evented_space_primitives.ts +++ b/client/spaces/evented_space_primitives.ts @@ -7,7 +7,9 @@ import { sleep } from "@silverbulletmd/silverbullet/lib/async"; /** * Events exposed: - * - file:changed (string, oldHash, newHash) + * - file:changed (string, oldHash, newHash, ownWrite): dispatched from inside + * writeFile, before it returns, so ownWrite is a listener's only way to tell + * our own write from someone else's. * - file:deleted (string) * - file:listed (FileMeta[]) * - file:initial: triggered in case of an initially empty snapshot, after the first set of events has gone out @@ -199,7 +201,7 @@ export class EventedSpacePrimitives implements SpacePrimitives { try { const newMeta = await this.wrapped.writeFile(path, data, meta); if (this.operationCount === 1) { - await this.triggerEventsAndCache(path, newMeta.lastModified); + await this.triggerEventsAndCache(path, newMeta.lastModified, true); } if (path.endsWith(".md")) { const pageName = path.substring(0, path.length - 3); @@ -216,14 +218,21 @@ export class EventedSpacePrimitives implements SpacePrimitives { /** * @param name * @param newHash + * @param ownWrite * @return whether something changed in the snapshot */ - async triggerEventsAndCache(name: string, newHash: number) { + async triggerEventsAndCache(name: string, newHash: number, ownWrite = false) { const oldHash = this.spaceSnapshot[name]; // if (oldHash && newHash && oldHash !== newHash) { if (oldHash !== newHash) { // Page changed since last cached metadata, trigger event - await this.dispatchEvent("file:changed", name, oldHash, newHash); + await this.dispatchEvent( + "file:changed", + name, + oldHash, + newHash, + ownWrite, + ); } this.updateInSnapshot(name, newHash); await this.saveSnapshot(); diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 3be2257f..19be263a 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -3,7 +3,7 @@ An attempt at documenting the changes/new features introduced in each release. ## Edge Whenever a commit is pushed to the `main` branch, within ~5 minutes, it will be released as a docker image with the `:v2` tag, and a binary in the [edge release](https://github.com/silverbulletmd/silverbullet/releases/tag/edge). If you want to live on the bleeding edge of SilverBullet goodness (or regression) this is where to do it. -* **Live external edits**: changes made to pages by other programs (or users, but don’t rely on this for real-time collaboration) now show up in an open pages almost instantly, instead of waiting for the next sync. The client applies them to the open page as minimal, cursor-preserving edits that land in the undo history, so `Cmd/Ctrl-z` reverts an external edit like any other. The externally inserted text is briefly highlighted with a marker showing where it landed, fading after a few seconds. +* **Live external edits**: changes made to pages by other programs (or users, but don’t rely on this for real-time collaboration) now show up in an open pages almost instantly, instead of waiting for the next sync. The client applies them to the open page as minimal, cursor-preserving edits land in the undo history, so `Cmd/Ctrl-z` reverts an external edit like any other. * This relies on active file-system, watching which can be configured server-wide with the new `SB_FS_WATCH` environment variable (`auto` (default) / `poll` / `off`): set `poll` when the space lives on a network mount (NFS/SMB) where changes made may not result in FS events. * **Inline comments** ([[Comment]]): any HTML comment (``) is now a parsed and rendered as a note. * [[Space Manager|Multi-space]] mode: the [[Runtime API]] (`runtimeApi`) is now **on by default** for new and existing spaces, instead of off. It only actually runs when the server found a Chrome or Chromium install at startup and only booted upon first use of the API. @@ -23,8 +23,7 @@ Whenever a commit is pushed to the `main` branch, within ~5 minutes, it will be ## 2.10.0 * [[Space Manager]]: multi-space hosting with multiple accounts is here. A fresh install pointed at an empty folder opens a browser-based first-run **setup wizard** that creates an admin account and your first space, then serves it in place with no restart. One server can host any number of [[Space|spaces]], each bound to a URL prefix or hostname. -* [[Baked Sections]]: bake `${...}` Lua expressions and widgets into - HTML-comment-delimited markdown (`` … ``). +* [[Baked Sections]]: bake `${...}` Lua expressions and widgets into HTML-comment-delimited markdown (`` … ``). * Space Lua: **code complete now shows documentation** (where available), all available via [[API/spacelua]] reflection APIs. * Backend and CLI have been ported to Rust ([see background on this](https://no.silverbullet.plus/tech-stacks)), both should be behavior preserving (that is: you shouldn’t really notice): * The server backend (previously written in Go) has now been replaced by an adapted version of [SilverBullet+](https://silverbullet.plus/)’s backend written in Rust, more unifying those code bases. @@ -290,8 +289,7 @@ Whenever a commit is pushed to the `main` branch, within ~5 minutes, it will be * `item` and `task` now also index (wiki) links and inherited (wiki) links (links appearing in parent nodes), as [requested here](https://community.silverbullet.md/t/coming-from-logseq-outlines-and-linked-mentions/290) under `links` and `ilinks`. Updated the "Linked Tasks" widget now to rely on `ilinks`. * Rewrote snippet text for links (used in [[Linked Mention|Linked Mentions]]) to be more contextual, now also includes child bullet items, see [community discussion](https://community.silverbullet.md/t/coming-from-logseq-outlines-and-linked-mentions/290). * For consistency with items, `task` `refs` now point to the item’s position resulting in a slight positional shift, if you have code relying on this, you may have to adjust it. - * Disabled indexing all paragraph text by default, this caused significant indexing overhead. [See discussion](https://community.silverbullet.md/t/who-is-using-paragraph-for-queries/3686). - To re-enable: `config.set("index.paragraph.all", true)` + * Disabled indexing all paragraph text by default, this caused significant indexing overhead. [See discussion](https://community.silverbullet.md/t/who-is-using-paragraph-for-queries/3686). To re-enable: `config.set("index.paragraph.all", true)` * Better link support in frontmatter (by [Tomasz Gorochowik](https://github.com/silverbulletmd/silverbullet/pull/1711)) * The `page:index` event now also receives a `text` and `meta` attributes. * [[Transclusions]] improvements: diff --git a/e2e/external-edit.test.ts b/e2e/external-edit.test.ts index 824d662b..ed39ec0e 100644 --- a/e2e/external-edit.test.ts +++ b/e2e/external-edit.test.ts @@ -8,6 +8,7 @@ import { redoChord, spawnServerProcess, test, + waitForSaveAndReadFromServer, waitForServer, } from "./fixtures.ts"; @@ -160,6 +161,63 @@ test("external edit merges with unsaved local typing", async ({ .toContain("LOCAL"); }); +test("the client's own save is not treated as an external change", async ({ + page, + sbServer, +}) => { + // Not what this test is about: an SSE notification makes the client probe + // file metadata, and a probe overlapping our own write suppresses that + // write's event entirely (operationCount guard in EventedSpacePrimitives), + // making the count below timing-dependent. + await page.route("**/.events", (route) => route.fulfill({ status: 404 })); + await gotoSilverBulletPage(page, sbServer); + + const editor = page.locator(".cm-content"); + await expect(editor).toContainText("Hello world"); + + await editor.click(); + await page.evaluate(() => { + const client = (globalThis as any).client; + (globalThis as any).__reloads = 0; + (globalThis as any).__ownWriteEvents = 0; + client.eventHook.addLocalListener( + "file:changed", + (name: string, _o: number, _n: number, ownWrite: boolean) => { + if (name === "index.md" && ownWrite) { + (globalThis as any).__ownWriteEvents++; + } + }, + ); + const cm = client.contentManager; + const original = cm.reloadPageContent.bind(cm); + cm.reloadPageContent = (...args: unknown[]) => { + (globalThis as any).__reloads++; + return original(...args); + }; + client.editorView.dispatch({ + selection: { anchor: client.editorView.state.doc.length }, + }); + }); + + await page.keyboard.type("typed by me"); + const saved = await waitForSaveAndReadFromServer(page, sbServer, "index.md"); + expect(saved).toBe("Hello world\ntyped by me"); + await page.waitForTimeout(1500); + + // The save was announced, and the announcement said we caused it... + expect( + await page.evaluate(() => (globalThis as any).__ownWriteEvents), + ).toBeGreaterThan(0); + // ...so it never reached the path that fetches the page back and merges it. + expect(await page.evaluate(() => (globalThis as any).__reloads)).toBe(0); + await expect(page.locator(".sb-external-edit")).toHaveCount(0); + expect( + await page.evaluate(() => + (globalThis as any).client.editorView.state.sliceDoc(), + ), + ).toBe("Hello world\ntyped by me"); +}); + test("undo isolates the external edit from the user's own prior typing", async ({ sbPage, sbServer,