From 1f74d8a7ee69e5552a56918ce4958d34437f6fa6 Mon Sep 17 00:00:00 2001 From: martian56 Date: Fri, 1 May 2026 01:29:49 +0400 Subject: [PATCH 1/7] feat(collab): sync image files via Yjs and ignore drift-only element changes --- .../src/lib/collab/yjs-bridge.test.ts | 165 ++++++++++++++++++ apps/frontend/src/lib/collab/yjs-bridge.ts | 62 ++++++- 2 files changed, 224 insertions(+), 3 deletions(-) diff --git a/apps/frontend/src/lib/collab/yjs-bridge.test.ts b/apps/frontend/src/lib/collab/yjs-bridge.test.ts index 23f8b99..918b605 100644 --- a/apps/frontend/src/lib/collab/yjs-bridge.test.ts +++ b/apps/frontend/src/lib/collab/yjs-bridge.test.ts @@ -2,8 +2,11 @@ import { describe, expect, it } from "vitest"; import * as Y from "yjs"; import { applyElementsToYMap, + applyFilesToYMap, readElementsFromYMap, + readFilesFromYMap, type ElementJson, + type FileJson, } from "./yjs-bridge"; function makeDoc() { @@ -117,6 +120,168 @@ describe("in-place element mutation (regression: 'only a dot' bug)", () => { }); }); +describe("echo suppression (regression: peer's drag snaps back)", () => { + it("does not write when only versionNonce changed but content is identical", () => { + const { doc, ymap } = makeDoc(); + applyElementsToYMap(doc, ymap, [ + { id: "x", versionNonce: 1, x: 100, y: 100 }, + ]); + + let updates = 0; + doc.on("update", () => updates++); + // Excalidraw bumps versionNonce while applying an inbound scene update + // even though the visible state is unchanged. Without echo suppression + // we'd write this back to the peer mid-drag. + applyElementsToYMap(doc, ymap, [ + { id: "x", versionNonce: 999, x: 100, y: 100 }, + ]); + expect(updates).toBe(0); + }); + + it("still writes when content actually changed (different x/y)", () => { + const { doc, ymap } = makeDoc(); + applyElementsToYMap(doc, ymap, [ + { id: "x", versionNonce: 1, x: 100, y: 100 }, + ]); + + let updates = 0; + doc.on("update", () => updates++); + applyElementsToYMap(doc, ymap, [ + { id: "x", versionNonce: 2, x: 200, y: 100 }, + ]); + expect(updates).toBeGreaterThan(0); + expect(ymap.get("x")?.x).toBe(200); + }); + + it("ignores `version` field drift the same way", () => { + const { doc, ymap } = makeDoc(); + applyElementsToYMap(doc, ymap, [ + { id: "x", versionNonce: 1, version: 1, x: 0 }, + ]); + + let updates = 0; + doc.on("update", () => updates++); + applyElementsToYMap(doc, ymap, [ + { id: "x", versionNonce: 2, version: 5, x: 0 }, + ]); + expect(updates).toBe(0); + }); + + it("ignores `updated` timestamp drift (the field Excalidraw bumps when applying inbound updateScene)", () => { + const { doc, ymap } = makeDoc(); + applyElementsToYMap(doc, ymap, [ + { id: "x", versionNonce: 1, updated: 1000, x: 50 }, + ]); + + let updates = 0; + doc.on("update", () => updates++); + applyElementsToYMap(doc, ymap, [ + { id: "x", versionNonce: 2, updated: 9999, x: 50 }, + ]); + expect(updates).toBe(0); + }); + + it("ignores `seed` drift", () => { + const { doc, ymap } = makeDoc(); + applyElementsToYMap(doc, ymap, [ + { id: "x", versionNonce: 1, seed: 111, x: 0 }, + ]); + + let updates = 0; + doc.on("update", () => updates++); + applyElementsToYMap(doc, ymap, [ + { id: "x", versionNonce: 2, seed: 222, x: 0 }, + ]); + expect(updates).toBe(0); + }); + + it("simulates the full peer-drag echo path: B drags, A's stamp comes back, B holds", () => { + // Models the snap-back bug end-to-end inside one doc. + const { doc, ymap } = makeDoc(); + // Initial state: image at x=100. + applyElementsToYMap(doc, ymap, [ + { id: "img", versionNonce: 1, version: 1, updated: 1000, x: 100, y: 0 }, + ]); + + // Peer B drags it to x=200 (real change). + applyElementsToYMap(doc, ymap, [ + { id: "img", versionNonce: 2, version: 2, updated: 2000, x: 200, y: 0 }, + ]); + expect(ymap.get("img")?.x).toBe(200); + + // Peer A's Excalidraw applied that update via updateScene, then + // fired onChange with the same x=200 but with version/nonce/updated + // re-stamped. The bridge MUST NOT echo this back. + let updates = 0; + doc.on("update", () => updates++); + applyElementsToYMap(doc, ymap, [ + { id: "img", versionNonce: 3, version: 3, updated: 3000, x: 200, y: 0 }, + ]); + expect(updates).toBe(0); + expect(ymap.get("img")?.x).toBe(200); + }); +}); + +describe("applyFilesToYMap", () => { + function makeFilesDoc() { + const doc = new Y.Doc(); + const ymap = doc.getMap("files"); + return { doc, ymap }; + } + + it("inserts files keyed by id", () => { + const { doc, ymap } = makeFilesDoc(); + applyFilesToYMap(doc, ymap, { + f1: { id: "f1", dataURL: "data:a", mimeType: "image/png" }, + f2: { id: "f2", dataURL: "data:b", mimeType: "image/png" }, + }); + expect(ymap.size).toBe(2); + expect(ymap.get("f1")?.dataURL).toBe("data:a"); + }); + + it("never overwrites an existing entry by the same id", () => { + const { doc, ymap } = makeFilesDoc(); + applyFilesToYMap(doc, ymap, { + f1: { id: "f1", dataURL: "data:a" }, + }); + applyFilesToYMap(doc, ymap, { + f1: { id: "f1", dataURL: "data:OVERWRITE" }, + }); + expect(ymap.get("f1")?.dataURL).toBe("data:a"); + }); + + it("does not delete files missing from the input (insert-only)", () => { + const { doc, ymap } = makeFilesDoc(); + applyFilesToYMap(doc, ymap, { + f1: { id: "f1", dataURL: "data:a" }, + f2: { id: "f2", dataURL: "data:b" }, + }); + applyFilesToYMap(doc, ymap, { + f1: { id: "f1", dataURL: "data:a" }, + }); + expect(ymap.has("f2")).toBe(true); + }); + + it("ignores entries without a string id", () => { + const { doc, ymap } = makeFilesDoc(); + applyFilesToYMap(doc, ymap, { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + bogus: { dataURL: "x" } as any, + }); + expect(ymap.size).toBe(0); + }); + + it("syncs files between two docs via Yjs updates", () => { + const a = makeFilesDoc(); + const b = makeFilesDoc(); + applyFilesToYMap(a.doc, a.ymap, { + f1: { id: "f1", dataURL: "data:a" }, + }); + Y.applyUpdate(b.doc, Y.encodeStateAsUpdate(a.doc)); + expect(readFilesFromYMap(b.ymap).map((f) => f.id)).toEqual(["f1"]); + }); +}); + describe("Yjs round-trip between two docs", () => { it("converges to the same map after exchanging updates", () => { const a = makeDoc(); diff --git a/apps/frontend/src/lib/collab/yjs-bridge.ts b/apps/frontend/src/lib/collab/yjs-bridge.ts index b6c150c..c4d4538 100644 --- a/apps/frontend/src/lib/collab/yjs-bridge.ts +++ b/apps/frontend/src/lib/collab/yjs-bridge.ts @@ -51,12 +51,68 @@ export function readElementsFromYMap(ymap: Y.Map): ElementJson[] { return values; } +export type FileJson = Record & { id: string }; + +export function applyFilesToYMap( + doc: Y.Doc, + ymap: Y.Map, + files: Readonly>, +): void { + // Files are insert-only by stable id. Excalidraw retains BinaryFileData + // blobs even after the referencing image element is deleted, and the + // local `files` map can transiently lack an entry that was uploaded by + // another peer — never delete from the shared map on absence, only add. + const ids = Object.keys(files); + if (ids.length === 0) return; + doc.transact(() => { + for (const id of ids) { + const f = files[id]; + if (!f || typeof f.id !== "string") continue; + if (!ymap.has(id)) ymap.set(id, { ...f }); + } + }, LOCAL_ORIGIN); +} + +export function readFilesFromYMap(ymap: Y.Map): FileJson[] { + return Array.from(ymap.values()); +} + function elementsEqual(a: ElementJson, b: ElementJson): boolean { + // Fast path — matching versionNonce always implies identical content, + // so we can skip the structural compare on the hot drag path. if ( typeof a.versionNonce === "number" && - typeof b.versionNonce === "number" + typeof b.versionNonce === "number" && + a.versionNonce === b.versionNonce ) { - return a.versionNonce === b.versionNonce; + return true; } - return JSON.stringify(a) === JSON.stringify(b); + // Slow path — Excalidraw sometimes bumps an element's version / + // versionNonce without changing visible content (e.g. when applying + // an inbound updateScene). If we treated those as different, we'd + // echo a "no-op" change back to the peer, and that echo lands on + // their canvas mid-drag and snaps them back to the pre-drag spot. + return canonicalize(a) === canonicalize(b); +} + +// Fields Excalidraw bumps on every "touch" without any user-visible +// change. `updated` is the epoch ms of the last modification (re-stamped +// when updateScene applies an inbound element); `version` / +// `versionNonce` are Excalidraw's own collab-reconciliation cursors; +// `seed` is for roughjs shape stability and shouldn't drift, but is +// excluded defensively in case a clone path regenerates it. +const DRIFT_FIELDS: ReadonlySet = new Set([ + "version", + "versionNonce", + "updated", + "seed", +]); + +function canonicalize(el: ElementJson): string { + const keys = Object.keys(el) + .filter((k) => !DRIFT_FIELDS.has(k)) + .sort(); + const out: Record = {}; + for (const k of keys) out[k] = (el as Record)[k]; + return JSON.stringify(out); } From 2fdc57db87197d302cf75937bc92a544b7203517 Mon Sep 17 00:00:00 2001 From: martian56 Date: Fri, 1 May 2026 01:29:52 +0400 Subject: [PATCH 2/7] feat(collab): atomic syncLocalChanges and remote files observer --- apps/frontend/src/hooks/useBoardCollab.ts | 61 ++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) diff --git a/apps/frontend/src/hooks/useBoardCollab.ts b/apps/frontend/src/hooks/useBoardCollab.ts index 986030d..6f44481 100644 --- a/apps/frontend/src/hooks/useBoardCollab.ts +++ b/apps/frontend/src/hooks/useBoardCollab.ts @@ -6,8 +6,11 @@ import { PERSISTENCE_LOAD_ORIGIN, REMOTE_ORIGIN, applyElementsToYMap, + applyFilesToYMap, readElementsFromYMap, + readFilesFromYMap, type ElementJson, + type FileJson, } from "@/lib/collab/yjs-bridge"; import { decodeYjsUpdate, encodeYjsDoc } from "@/lib/collab/yjs-persistence"; import { @@ -20,6 +23,7 @@ export interface UseBoardCollabOptions extends Omit< "onRemoteBinary" > { onRemoteElements?: (elements: ElementJson[]) => void; + onRemoteFiles?: (files: FileJson[]) => void; } export function useBoardCollab( @@ -27,11 +31,15 @@ export function useBoardCollab( token: string | null, options: UseBoardCollabOptions = {}, ) { - const { onRemoteElements, ...wsOptions } = options; + const { onRemoteElements, onRemoteFiles, ...wsOptions } = options; const onRemoteElementsRef = useRef(onRemoteElements); + const onRemoteFilesRef = useRef(onRemoteFiles); useEffect(() => { onRemoteElementsRef.current = onRemoteElements; }, [onRemoteElements]); + useEffect(() => { + onRemoteFilesRef.current = onRemoteFiles; + }, [onRemoteFiles]); // boardId is the dependency even though the factory doesn't read it: // changing boards must produce a fresh Y.Doc. @@ -44,6 +52,7 @@ export function useBoardCollab( }, [doc]); const ymap = useMemo(() => doc.getMap("elements"), [doc]); + const yfiles = useMemo(() => doc.getMap("files"), [doc]); // Per-user undo scope: only the local client's edits are in the stack. // Remote edits don't end up on our undo history — you can only undo @@ -109,6 +118,22 @@ export function useBoardCollab( }; }, [ymap]); + useEffect(() => { + const observer = (event: Y.YMapEvent) => { + if (event.transaction.origin === LOCAL_ORIGIN) return; + const added: FileJson[] = []; + for (const id of event.keysChanged) { + const f = yfiles.get(id); + if (f) added.push(f); + } + if (added.length > 0) onRemoteFilesRef.current?.(added); + }; + yfiles.observe(observer); + return () => { + yfiles.unobserve(observer); + }; + }, [yfiles]); + const syncLocalElements = useCallback( (elements: readonly ElementJson[]) => { applyElementsToYMap(doc, ymap, elements); @@ -116,6 +141,37 @@ export function useBoardCollab( [doc, ymap], ); + const syncLocalFiles = useCallback( + (files: Readonly> | null | undefined) => { + if (!files) return; + applyFilesToYMap(doc, yfiles, files); + }, + [doc, yfiles], + ); + + // Combined writer used by the canvas onChange handler. One outer + // transact means files + elements travel as a single Yjs update, + // so a peer never sees an image element whose fileId hasn't been + // populated in the files map yet (which renders the image as a + // pending placeholder Excalidraw won't let you interact with). + const syncLocalChanges = useCallback( + ( + elements: readonly ElementJson[], + files: Readonly> | null | undefined, + ) => { + doc.transact(() => { + if (files) applyFilesToYMap(doc, yfiles, files); + applyElementsToYMap(doc, ymap, elements); + }, LOCAL_ORIGIN); + }, + [doc, ymap, yfiles], + ); + + const readAllFiles = useCallback( + (): FileJson[] => readFilesFromYMap(yfiles), + [yfiles], + ); + const seedFromSnapshot = useCallback( (elements: readonly ElementJson[] | null | undefined) => { if (!elements || elements.length === 0) return; @@ -143,6 +199,9 @@ export function useBoardCollab( return { ...ws, syncLocalElements, + syncLocalFiles, + syncLocalChanges, + readAllFiles, seedFromSnapshot, encodeYjsState, applyYjsState, From f4a526a4ce65bd67b73f937b63b2a9c79cd32433 Mon Sep 17 00:00:00 2001 From: martian56 Date: Fri, 1 May 2026 01:29:52 +0400 Subject: [PATCH 3/7] feat(boards): persist files map in board snapshot --- apps/frontend/src/hooks/useBoardContent.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apps/frontend/src/hooks/useBoardContent.ts b/apps/frontend/src/hooks/useBoardContent.ts index 4fbbd85..18e2399 100644 --- a/apps/frontend/src/hooks/useBoardContent.ts +++ b/apps/frontend/src/hooks/useBoardContent.ts @@ -6,6 +6,9 @@ const SNAPSHOT_TYPE = "excalidraw_snapshot"; export type ExcalidrawSnapshot = { elements?: readonly unknown[] | null; appState?: Readonly> | null; + // Image blobs keyed by Excalidraw fileId. Image elements only carry + // a fileId reference, so without this map the image renders blank. + files?: Readonly> | null; // Authoritative shared state when present (base64 Yjs update). // Legacy boards have only `elements`/`appState`. yjs_update?: string | null; From 961747a81b82120c9c2dc1ff002ca2eec98c5549 Mon Sep 17 00:00:00 2001 From: martian56 Date: Fri, 1 May 2026 01:29:53 +0400 Subject: [PATCH 4/7] fix(board): register image files before applying remote elements --- apps/frontend/src/pages/board/BoardPage.tsx | 84 +++++++++++++++++++-- 1 file changed, 78 insertions(+), 6 deletions(-) diff --git a/apps/frontend/src/pages/board/BoardPage.tsx b/apps/frontend/src/pages/board/BoardPage.tsx index 182f881..735e39e 100644 --- a/apps/frontend/src/pages/board/BoardPage.tsx +++ b/apps/frontend/src/pages/board/BoardPage.tsx @@ -14,7 +14,7 @@ import type { ExcalidrawSnapshot } from "@/hooks/useBoardContent"; import { useBoardInit } from "@/hooks/useBoardInit"; import { useTheme } from "@/context/ThemeContext"; import { apiFetch } from "@/lib/api"; -import type { ElementJson } from "@/lib/collab/yjs-bridge"; +import type { ElementJson, FileJson } from "@/lib/collab/yjs-bridge"; import { useAuthStore } from "@/stores/authStore"; type ExcalidrawAPI = Parameters< @@ -184,9 +184,25 @@ export function BoardPage() { [excalidrawAPI], ); + // Hoisted via ref so the callback can reach into useBoardCollab's + // return value without a forward-reference TDZ — the hook is called + // below and consumes onRemoteElements as one of its options. + const readAllFilesRef = useRef<(() => FileJson[]) | null>(null); const onRemoteElements = useCallback( (elements: ElementJson[]) => { if (!excalidrawAPI) return; + // Register every known file first. If an image element references + // a fileId whose blob hasn't reached this client yet, Excalidraw + // marks the element as not-fully-loaded and disables interaction — + // the peer who didn't add the image can't move it. addFiles is + // idempotent, so calling it on every remote elements update is + // safe and only meaningful when a new file actually landed. + const files = readAllFilesRef.current?.() ?? []; + if (files.length > 0) { + excalidrawAPI.addFiles( + files as unknown as Parameters[0], + ); + } excalidrawAPI.updateScene({ elements: elements as unknown as Parameters< ExcalidrawAPI["updateScene"] @@ -196,10 +212,21 @@ export function BoardPage() { [excalidrawAPI], ); + const onRemoteFiles = useCallback( + (files: FileJson[]) => { + if (!excalidrawAPI || files.length === 0) return; + excalidrawAPI.addFiles( + files as unknown as Parameters[0], + ); + }, + [excalidrawAPI], + ); + const { remoteCursors, sendCursor, - syncLocalElements, + syncLocalChanges, + readAllFiles, seedFromSnapshot, encodeYjsState, applyYjsState, @@ -209,9 +236,14 @@ export function BoardPage() { currentUserId: user?.id ?? null, onRemoteDocument, onRemoteElements, + onRemoteFiles, onRemoteEvent: fanoutBoardEvent, }); + useEffect(() => { + readAllFilesRef.current = readAllFiles; + }, [readAllFiles]); + // Ctrl/Cmd+Z and Ctrl/Cmd+Shift+Z drive the collab-aware undo stack // (only undoes the local user's own ops, not remote ones). useEffect(() => { @@ -303,23 +335,33 @@ export function BoardPage() { // reason the canvas felt sluggish). const latestElementsRef = useRef([]); const latestAppStateRef = useRef(undefined); + const latestFilesRef = useRef>({}); const handleChange = useCallback( - (elements: readonly unknown[], appState: unknown) => { - syncLocalElements(elements as readonly ElementJson[]); + ( + elements: readonly unknown[], + appState: unknown, + files: Readonly>, + ) => { + syncLocalChanges( + elements as readonly ElementJson[], + files as Readonly>, + ); latestElementsRef.current = elements; latestAppStateRef.current = appState; + latestFilesRef.current = files as Record; scheduleSave(() => { const content: ExcalidrawSnapshot = { elements: [...latestElementsRef.current], appState: latestAppStateRef.current as ExcalidrawSnapshot["appState"], + files: latestFilesRef.current, yjs_update: encodeYjsState(), }; latestContentRef.current = content; return content; }); }, - [scheduleSave, syncLocalElements, encodeYjsState], + [scheduleSave, syncLocalChanges, encodeYjsState], ); useEffect(() => { @@ -347,6 +389,34 @@ export function BoardPage() { snapshot.appState ?? undefined; + const seedFiles = () => { + // Prefer files materialized in the Y.Doc (authoritative); fall + // back to the legacy `files` field on the snapshot. Either way, + // every entry must reach Excalidraw via addFiles or image + // elements render as blank. + const fromDoc = readAllFiles(); + if (fromDoc.length > 0) { + excalidrawAPI.addFiles( + fromDoc as unknown as Parameters[0], + ); + return; + } + const fromSnapshot = snapshot.files; + if (fromSnapshot && typeof fromSnapshot === "object") { + const arr = Object.values(fromSnapshot).filter( + (f): f is FileJson => + !!f && + typeof f === "object" && + typeof (f as FileJson).id === "string", + ); + if (arr.length > 0) { + excalidrawAPI.addFiles( + arr as unknown as Parameters[0], + ); + } + } + }; + if (snapshot.yjs_update) { applyYjsState(snapshot.yjs_update); if (appState != null) { @@ -359,6 +429,7 @@ export function BoardPage() { >[0]["appState"], }); } + seedFiles(); initialSnapshotAppliedRef.current = true; return; } @@ -378,9 +449,10 @@ export function BoardPage() { if (elements) { seedFromSnapshot(elements as readonly ElementJson[]); } + seedFiles(); initialSnapshotAppliedRef.current = true; } - }, [excalidrawAPI, snapshot, seedFromSnapshot, applyYjsState]); + }, [excalidrawAPI, snapshot, seedFromSnapshot, applyYjsState, readAllFiles]); useEffect(() => { if (boardId != null) initialSnapshotAppliedRef.current = false; From 597e7b550408e163d25bca7e88b5a1eeb8a8b953 Mon Sep 17 00:00:00 2001 From: martian56 Date: Fri, 1 May 2026 01:29:54 +0400 Subject: [PATCH 5/7] fix(board): pass files into shared board initial data --- apps/frontend/src/pages/board/SharedBoardPage.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/frontend/src/pages/board/SharedBoardPage.tsx b/apps/frontend/src/pages/board/SharedBoardPage.tsx index 5e7e425..20a0665 100644 --- a/apps/frontend/src/pages/board/SharedBoardPage.tsx +++ b/apps/frontend/src/pages/board/SharedBoardPage.tsx @@ -44,6 +44,7 @@ const VIEWER_UI_OPTIONS = { type Snapshot = { elements?: readonly unknown[] | null; appState?: Record | null; + files?: Record | null; }; export function SharedBoardPage() { @@ -109,6 +110,7 @@ export function SharedBoardPage() { normalizeAppStateForExcalidraw(snapshot.appState) ?? snapshot.appState ?? undefined, + files: snapshot.files ?? undefined, } as React.ComponentProps["initialData"]; return ( From 1be07ba4dbbbebf666aea3227db0538263ce73a4 Mon Sep 17 00:00:00 2001 From: martian56 Date: Fri, 1 May 2026 01:33:20 +0400 Subject: [PATCH 6/7] chore: version bump --- apps/frontend/package-lock.json | 4 ++-- apps/frontend/package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/frontend/package-lock.json b/apps/frontend/package-lock.json index d9e95e4..c79a1c1 100644 --- a/apps/frontend/package-lock.json +++ b/apps/frontend/package-lock.json @@ -1,12 +1,12 @@ { "name": "loomy", - "version": "0.3.0", + "version": "0.4.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "loomy", - "version": "0.3.0", + "version": "0.4.0", "dependencies": { "@excalidraw/excalidraw": "^0.18.0", "@tailwindcss/vite": "^4.2.1", diff --git a/apps/frontend/package.json b/apps/frontend/package.json index 4f04f7d..89d1e88 100644 --- a/apps/frontend/package.json +++ b/apps/frontend/package.json @@ -1,7 +1,7 @@ { "name": "loomy", "private": true, - "version": "0.3.0", + "version": "0.4.0", "type": "module", "scripts": { "dev": "vite", From 3e6ce2e13cb46deb7ed45c26ea995d025dc92c5f Mon Sep 17 00:00:00 2001 From: martian56 Date: Fri, 1 May 2026 08:53:05 +0400 Subject: [PATCH 7/7] fix: resolve copilot warnings --- apps/frontend/src/hooks/useBoardCollab.ts | 8 +- apps/frontend/src/lib/collab/yjs-bridge.ts | 156 ++++++++++++------ apps/frontend/src/pages/board/BoardPage.tsx | 32 +--- .../src/pages/board/SharedBoardPage.tsx | 54 +++++- 4 files changed, 167 insertions(+), 83 deletions(-) diff --git a/apps/frontend/src/hooks/useBoardCollab.ts b/apps/frontend/src/hooks/useBoardCollab.ts index 6f44481..1246307 100644 --- a/apps/frontend/src/hooks/useBoardCollab.ts +++ b/apps/frontend/src/hooks/useBoardCollab.ts @@ -6,7 +6,9 @@ import { PERSISTENCE_LOAD_ORIGIN, REMOTE_ORIGIN, applyElementsToYMap, + applyElementsToYMapInner, applyFilesToYMap, + applyFilesToYMapInner, readElementsFromYMap, readFilesFromYMap, type ElementJson, @@ -154,14 +156,16 @@ export function useBoardCollab( // so a peer never sees an image element whose fileId hasn't been // populated in the files map yet (which renders the image as a // pending placeholder Excalidraw won't let you interact with). + // Calls the *Inner helpers (no nested transacts) so this is the + // single place that defines the transaction boundary and origin. const syncLocalChanges = useCallback( ( elements: readonly ElementJson[], files: Readonly> | null | undefined, ) => { doc.transact(() => { - if (files) applyFilesToYMap(doc, yfiles, files); - applyElementsToYMap(doc, ymap, elements); + if (files) applyFilesToYMapInner(yfiles, files); + applyElementsToYMapInner(ymap, elements); }, LOCAL_ORIGIN); }, [doc, ymap, yfiles], diff --git a/apps/frontend/src/lib/collab/yjs-bridge.ts b/apps/frontend/src/lib/collab/yjs-bridge.ts index c4d4538..21ad0b2 100644 --- a/apps/frontend/src/lib/collab/yjs-bridge.ts +++ b/apps/frontend/src/lib/collab/yjs-bridge.ts @@ -14,29 +14,39 @@ export const PERSISTENCE_LOAD_ORIGIN: unique symbol = Symbol( "loomy-persistence-load", ); +// Inner: assumes the caller is already inside a Y transaction. Lets the +// combined writer (`syncLocalChanges`) batch element + file writes into +// one transaction without nesting. +export function applyElementsToYMapInner( + ymap: Y.Map, + elements: readonly ElementJson[], +): void { + const seen = new Set(); + for (const el of elements) { + if (!el || typeof el.id !== "string") continue; + seen.add(el.id); + const existing = ymap.get(el.id); + if (!existing || !elementsEqual(existing, el)) { + // Clone before storing. Y.Map holds the value by reference, + // and Excalidraw mutates its own elements in place during drag. + // If we stored `el` directly, `existing === el` would make the + // diff see no change on the next onChange, so updates would + // stop propagating after the first pointer-down. + ymap.set(el.id, { ...el }); + } + } + for (const id of Array.from(ymap.keys())) { + if (!seen.has(id)) ymap.delete(id); + } +} + export function applyElementsToYMap( doc: Y.Doc, ymap: Y.Map, elements: readonly ElementJson[], ): void { doc.transact(() => { - const seen = new Set(); - for (const el of elements) { - if (!el || typeof el.id !== "string") continue; - seen.add(el.id); - const existing = ymap.get(el.id); - if (!existing || !elementsEqual(existing, el)) { - // Clone before storing. Y.Map holds the value by reference, - // and Excalidraw mutates its own elements in place during drag. - // If we stored `el` directly, `existing === el` would make the - // diff see no change on the next onChange, so updates would - // stop propagating after the first pointer-down. - ymap.set(el.id, { ...el }); - } - } - for (const id of Array.from(ymap.keys())) { - if (!seen.has(id)) ymap.delete(id); - } + applyElementsToYMapInner(ymap, elements); }, LOCAL_ORIGIN); } @@ -53,23 +63,30 @@ export function readElementsFromYMap(ymap: Y.Map): ElementJson[] { export type FileJson = Record & { id: string }; +// Inner: see applyElementsToYMapInner for the rationale on the split. +// Files are insert-only by stable id. Excalidraw retains BinaryFileData +// blobs even after the referencing image element is deleted, and the +// local `files` map can transiently lack an entry that was uploaded by +// another peer — never delete from the shared map on absence, only add. +export function applyFilesToYMapInner( + ymap: Y.Map, + files: Readonly>, +): void { + for (const id of Object.keys(files)) { + const f = files[id]; + if (!f || typeof f.id !== "string") continue; + if (!ymap.has(id)) ymap.set(id, { ...f }); + } +} + export function applyFilesToYMap( doc: Y.Doc, ymap: Y.Map, files: Readonly>, ): void { - // Files are insert-only by stable id. Excalidraw retains BinaryFileData - // blobs even after the referencing image element is deleted, and the - // local `files` map can transiently lack an entry that was uploaded by - // another peer — never delete from the shared map on absence, only add. - const ids = Object.keys(files); - if (ids.length === 0) return; + if (Object.keys(files).length === 0) return; doc.transact(() => { - for (const id of ids) { - const f = files[id]; - if (!f || typeof f.id !== "string") continue; - if (!ymap.has(id)) ymap.set(id, { ...f }); - } + applyFilesToYMapInner(ymap, files); }, LOCAL_ORIGIN); } @@ -77,24 +94,6 @@ export function readFilesFromYMap(ymap: Y.Map): FileJson[] { return Array.from(ymap.values()); } -function elementsEqual(a: ElementJson, b: ElementJson): boolean { - // Fast path — matching versionNonce always implies identical content, - // so we can skip the structural compare on the hot drag path. - if ( - typeof a.versionNonce === "number" && - typeof b.versionNonce === "number" && - a.versionNonce === b.versionNonce - ) { - return true; - } - // Slow path — Excalidraw sometimes bumps an element's version / - // versionNonce without changing visible content (e.g. when applying - // an inbound updateScene). If we treated those as different, we'd - // echo a "no-op" change back to the peer, and that echo lands on - // their canvas mid-drag and snaps them back to the pre-drag spot. - return canonicalize(a) === canonicalize(b); -} - // Fields Excalidraw bumps on every "touch" without any user-visible // change. `updated` is the epoch ms of the last modification (re-stamped // when updateScene applies an inbound element); `version` / @@ -108,11 +107,62 @@ const DRIFT_FIELDS: ReadonlySet = new Set([ "seed", ]); -function canonicalize(el: ElementJson): string { - const keys = Object.keys(el) - .filter((k) => !DRIFT_FIELDS.has(k)) - .sort(); - const out: Record = {}; - for (const k of keys) out[k] = (el as Record)[k]; - return JSON.stringify(out); +function elementsEqual(a: ElementJson, b: ElementJson): boolean { + // Fast path — matching versionNonce always implies identical content, + // so we can skip the structural compare on the hot drag path. + if ( + typeof a.versionNonce === "number" && + typeof b.versionNonce === "number" && + a.versionNonce === b.versionNonce + ) { + return true; + } + // Slow path — direct walk that ignores drift fields. Short-circuits + // on the first real mismatch and never allocates (no JSON.stringify, + // no Object.keys, no sort). On the steady drag echo, the walk visits + // each non-drift field once and returns true the moment it confirms + // they all match. + return contentEqual(a, b); +} + +function contentEqual(a: ElementJson, b: ElementJson): boolean { + const ao = a as Record; + const bo = b as Record; + let aCount = 0; + for (const k in ao) { + if (DRIFT_FIELDS.has(k)) continue; + aCount++; + if (!deepEqual(ao[k], bo[k])) return false; + } + let bCount = 0; + for (const k of Object.keys(bo)) { + if (!DRIFT_FIELDS.has(k)) bCount++; + } + return aCount === bCount; +} + +function deepEqual(a: unknown, b: unknown): boolean { + if (a === b) return true; + if (a == null || b == null) return false; + if (typeof a !== "object" || typeof b !== "object") return false; + const aArr = Array.isArray(a); + const bArr = Array.isArray(b); + if (aArr !== bArr) return false; + if (aArr) { + const arrA = a as unknown[]; + const arrB = b as unknown[]; + if (arrA.length !== arrB.length) return false; + for (let i = 0; i < arrA.length; i++) { + if (!deepEqual(arrA[i], arrB[i])) return false; + } + return true; + } + const objA = a as Record; + const objB = b as Record; + let countA = 0; + for (const k in objA) { + countA++; + if (!deepEqual(objA[k], objB[k])) return false; + } + return countA === Object.keys(objB).length; } diff --git a/apps/frontend/src/pages/board/BoardPage.tsx b/apps/frontend/src/pages/board/BoardPage.tsx index 735e39e..7d98687 100644 --- a/apps/frontend/src/pages/board/BoardPage.tsx +++ b/apps/frontend/src/pages/board/BoardPage.tsx @@ -184,25 +184,13 @@ export function BoardPage() { [excalidrawAPI], ); - // Hoisted via ref so the callback can reach into useBoardCollab's - // return value without a forward-reference TDZ — the hook is called - // below and consumes onRemoteElements as one of its options. - const readAllFilesRef = useRef<(() => FileJson[]) | null>(null); + // Files are registered separately via onRemoteFiles (which only + // fires for keys actually added to the Y.Doc files map), and via the + // initial seedFiles() pass below — so this handler doesn't need to + // re-read the whole files map on every drag tick. const onRemoteElements = useCallback( (elements: ElementJson[]) => { if (!excalidrawAPI) return; - // Register every known file first. If an image element references - // a fileId whose blob hasn't reached this client yet, Excalidraw - // marks the element as not-fully-loaded and disables interaction — - // the peer who didn't add the image can't move it. addFiles is - // idempotent, so calling it on every remote elements update is - // safe and only meaningful when a new file actually landed. - const files = readAllFilesRef.current?.() ?? []; - if (files.length > 0) { - excalidrawAPI.addFiles( - files as unknown as Parameters[0], - ); - } excalidrawAPI.updateScene({ elements: elements as unknown as Parameters< ExcalidrawAPI["updateScene"] @@ -240,10 +228,6 @@ export function BoardPage() { onRemoteEvent: fanoutBoardEvent, }); - useEffect(() => { - readAllFilesRef.current = readAllFiles; - }, [readAllFiles]); - // Ctrl/Cmd+Z and Ctrl/Cmd+Shift+Z drive the collab-aware undo stack // (only undoes the local user's own ops, not remote ones). useEffect(() => { @@ -332,10 +316,12 @@ export function BoardPage() { // Keep the latest elements+appState in refs so the deferred getter // below can read them at save time without re-encoding the Y.Doc on // every pointer move (that ran 60x/sec during drag and was the main - // reason the canvas felt sluggish). + // reason the canvas felt sluggish). Files are deliberately NOT + // mirrored here — they're already encoded inside `yjs_update`, and + // duplicating image dataURLs in the saved snapshot would double the + // autosave payload on image-heavy boards. const latestElementsRef = useRef([]); const latestAppStateRef = useRef(undefined); - const latestFilesRef = useRef>({}); const handleChange = useCallback( ( @@ -349,12 +335,10 @@ export function BoardPage() { ); latestElementsRef.current = elements; latestAppStateRef.current = appState; - latestFilesRef.current = files as Record; scheduleSave(() => { const content: ExcalidrawSnapshot = { elements: [...latestElementsRef.current], appState: latestAppStateRef.current as ExcalidrawSnapshot["appState"], - files: latestFilesRef.current, yjs_update: encodeYjsState(), }; latestContentRef.current = content; diff --git a/apps/frontend/src/pages/board/SharedBoardPage.tsx b/apps/frontend/src/pages/board/SharedBoardPage.tsx index 20a0665..0dcaea2 100644 --- a/apps/frontend/src/pages/board/SharedBoardPage.tsx +++ b/apps/frontend/src/pages/board/SharedBoardPage.tsx @@ -1,11 +1,19 @@ import React, { useEffect, useState } from "react"; import { useParams } from "react-router-dom"; +import * as Y from "yjs"; import { Excalidraw } from "@excalidraw/excalidraw"; import "@excalidraw/excalidraw/index.css"; import "@/styles/excalidraw-board.css"; import { PageTitle } from "@/components/PageTitle"; import { useTheme } from "@/context/ThemeContext"; +import { + readElementsFromYMap, + readFilesFromYMap, + type ElementJson, + type FileJson, +} from "@/lib/collab/yjs-bridge"; +import { decodeYjsUpdate } from "@/lib/collab/yjs-persistence"; const API_BASE = import.meta.env.VITE_API_URL || "http://localhost:8000"; @@ -45,8 +53,45 @@ type Snapshot = { elements?: readonly unknown[] | null; appState?: Record | null; files?: Record | null; + yjs_update?: string | null; }; +// New saves omit `files` because the same blobs are already inside +// `yjs_update`. Decode the Y.Doc here so the read-only viewer renders +// images regardless of whether the snapshot was written before or after +// that change. +function unpackSnapshot(snapshot: Snapshot): { + elements?: readonly unknown[]; + files?: Record; + appState?: Record; +} { + if (snapshot.yjs_update) { + try { + const doc = new Y.Doc(); + Y.applyUpdate(doc, decodeYjsUpdate(snapshot.yjs_update)); + const ymap = doc.getMap("elements"); + const yfiles = doc.getMap("files"); + const elements = readElementsFromYMap(ymap); + const fileEntries = readFilesFromYMap(yfiles); + doc.destroy(); + const files: Record = {}; + for (const f of fileEntries) files[f.id] = f; + return { + elements: elements.length > 0 ? elements : undefined, + files: fileEntries.length > 0 ? files : undefined, + appState: snapshot.appState ?? undefined, + }; + } catch { + // Corrupt yjs_update — fall through to legacy fields. + } + } + return { + elements: snapshot.elements ?? undefined, + files: snapshot.files ?? undefined, + appState: snapshot.appState ?? undefined, + }; +} + export function SharedBoardPage() { const { token } = useParams<{ token: string }>(); const { theme } = useTheme(); @@ -104,13 +149,14 @@ export function SharedBoardPage() { ); } + const unpacked = unpackSnapshot(snapshot); const initialData = { - elements: snapshot.elements ?? undefined, + elements: unpacked.elements, appState: - normalizeAppStateForExcalidraw(snapshot.appState) ?? - snapshot.appState ?? + normalizeAppStateForExcalidraw(unpacked.appState) ?? + unpacked.appState ?? undefined, - files: snapshot.files ?? undefined, + files: unpacked.files, } as React.ComponentProps["initialData"]; return (