From 17fff2d1c733b396c694fc50235e9c483505da60 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Wed, 12 Aug 2026 18:27:52 -0700 Subject: [PATCH 1/3] fix(files): stop the collaborative editor rewriting and reflowing a document on open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening a file rewrote it. Binding an editor to a seeded document emits a Yjs update of its own — ProseMirror appends an empty paragraph to any doc that does not end in one — which the relay saw as a real edit and persisted. Every open therefore uploaded the file under a FRESH storage key and deleted the old one, 404ing the page's own in-flight content read, bumping "Last Updated" just from viewing, and churning a blob per open. Worse, a trailing blank line cannot serialize, so the file never recorded that paragraph and nothing reconciled the two: each client that seeded without seeing another's contribution stacked one more. A real document reached 18 against the placeholder's 1 — measured as the pane growing several hundred pixels the instant the live editor took over. - Seed and merge through the editor's own normal form (`editorNormalForm`), so binding is a no-op and `canonicalizeYDoc` collapses an accumulated run back to one. Placed at the collab boundary, not in `parseMarkdownToDoc`: only the CRDT has to agree with the editor — every other consumer of the parse renders through a real editor that normalizes itself. - Skip a persist whose projection already matches the durable bytes. Byte length is the free reject, so the compare read only happens when a no-op write is actually on the table. - Revoke collaborative readiness on a fatal join. The sticky `syncedOnce` latch outlived the document: after a readiness timeout the provider drops `synced` so the gate closes, but the latch re-opened it on the offline fallback's seed flag — handing back an EDITABLE editor on a document the provider had abandoned, with client autosave gated off because collaboration is nominally on. Keystrokes went nowhere and vanished on reload, with no error shown. - Recover from a superseded storage key instead of stranding the reader: a 404 re-resolves the file record, so the read re-keys onto the current object. And do not focus-refetch durable bytes while the relay owns durability. - Prefetch the workspace file list in the layout, where the sidebar already reads it. `HydrationBoundary` defers an already-seen query to an effect that SSR never runs, so a page-level prefetch of that key could not reach the server render — the file route rendered a spinner and disagreed with the client about the header's markup (a hydration mismatch). - Load the document font with `display: block`. A swap repaints prose in metric-adjusted Arial first, so paragraphs re-wrap when the real face lands. Also: a detail-route `loading.tsx` (the segment was inheriting the list chrome), `normalize.ts` renamed to `field.ts` now that it holds only the field constant, and the duplicate `COLLAB_DOC_FIELD` in the streaming path folded into it. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/app/_styles/fonts/season/season.ts | 15 +- .../[workspaceId]/files/[fileId]/loading.tsx | 35 +++ .../[workspaceId]/files/[fileId]/page.tsx | 41 +++- .../collaboration/apply-streamed-markdown.ts | 12 +- .../collab-streaming-integration.test.ts | 49 +++- .../collaboration/readiness.test.ts | 83 +++++-- .../collaboration/readiness.ts | 18 +- .../rich-markdown-editor/markdown-fidelity.ts | 9 +- .../markdown-parse.test.ts | 190 +++++++++++---- .../rich-markdown-editor/markdown-parse.ts | 222 ++++++++++++++---- .../rich-markdown-editor.tsx | 62 ++++- .../file-viewer/use-editable-file-content.ts | 10 +- .../workspace/[workspaceId]/files/prefetch.ts | 41 ++-- .../[workspaceId]/lib/prefetch.test.ts | 24 +- .../app/workspace/[workspaceId]/prefetch.ts | 21 ++ .../hooks/queries/workspace-files.test.tsx | 97 +++++++- apps/sim/hooks/queries/workspace-files.ts | 90 ++++++- apps/sim/lib/collab-doc/converter.test.ts | 214 ++++++++++++++++- apps/sim/lib/collab-doc/converter.ts | 52 +++- apps/sim/lib/collab-doc/field.ts | 8 + apps/sim/lib/collab-doc/normalize.test.ts | 127 ---------- apps/sim/lib/collab-doc/normalize.ts | 43 ---- apps/sim/lib/collab-doc/persist.test.ts | 170 ++++++++++++++ apps/sim/lib/collab-doc/persist.ts | 45 +++- apps/sim/lib/collab-doc/seed.test.ts | 50 +++- apps/sim/lib/collab-doc/seed.ts | 25 +- 26 files changed, 1383 insertions(+), 370 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/files/[fileId]/loading.tsx create mode 100644 apps/sim/lib/collab-doc/field.ts delete mode 100644 apps/sim/lib/collab-doc/normalize.test.ts delete mode 100644 apps/sim/lib/collab-doc/normalize.ts create mode 100644 apps/sim/lib/collab-doc/persist.test.ts diff --git a/apps/sim/app/_styles/fonts/season/season.ts b/apps/sim/app/_styles/fonts/season/season.ts index b778b47e985..eff2a3cec31 100644 --- a/apps/sim/app/_styles/fonts/season/season.ts +++ b/apps/sim/app/_styles/fonts/season/season.ts @@ -3,13 +3,26 @@ import localFont from 'next/font/local' /** * Season Sans variable font configuration * Uses variable font file to support any weight from 300-800 + * + * `display: 'block'`, not `swap`: this is the document font, so a swap is not a cosmetic change of + * typeface — the fallback's glyph advances differ, so paragraphs re-wrap and everything below them + * moves. In long-form prose (the Files editor) that reads as the line and paragraph spacing visibly + * correcting itself a beat after the text appears, on every hard refresh (a normal reload serves the + * font from cache and never swaps). `swap` is the setting that says "painting the wrong font first is + * fine"; for a brand face it is not. + * + * The block period costs nothing here because delivery is already optimal: `preload` emits a + * `Link: rel=preload` RESPONSE header, so the fetch starts before the HTML is parsed, and the file is + * one same-origin, immutably-cached 87KB woff2. The metric-adjusted Arial below stays as the safety + * net for the >3s tail, where the browser gives up blocking and swaps — i.e. the worst case is + * today's behavior, not a regression. */ export const season = localFont({ src: [ // Variable font - supports all weights from 300 to 800 { path: './SeasonSansUprightsVF.woff2', weight: '300 800', style: 'normal' }, ], - display: 'swap', + display: 'block', preload: true, variable: '--font-season', fallback: ['system-ui', 'Segoe UI', 'Roboto', 'Helvetica Neue', 'Arial', 'Noto Sans'], diff --git a/apps/sim/app/workspace/[workspaceId]/files/[fileId]/loading.tsx b/apps/sim/app/workspace/[workspaceId]/files/[fileId]/loading.tsx new file mode 100644 index 00000000000..da22416378e --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/files/[fileId]/loading.tsx @@ -0,0 +1,35 @@ +'use client' + +import { File as FileIcon } from '@sim/emcn/icons' +import { noop } from '@sim/utils/helpers' +import { + type BreadcrumbItem, + ResourceChromeFallback, +} from '@/app/workspace/[workspaceId]/components' +import { FOLDERED_RESOURCE_HEADERS } from '@/app/workspace/[workspaceId]/components/folders/foldered-resources' + +const FILES_HEADER = FOLDERED_RESOURCE_HEADERS.file + +/** + * Transcribes the trail the loaded page shows while its record resolves (`loadingBreadcrumbs` in + * `files.tsx`): the root crumb plus a terminal `…`, with no icon on the leaf — so the fallback and + * the page paint the same two crumbs and only the label changes. + */ +const BREADCRUMBS: BreadcrumbItem[] = [ + { label: FILES_HEADER.rootLabel, icon: FileIcon, onClick: noop }, + { label: '…', terminal: true }, +] + +/** + * Fallback for the file DETAIL route. Without it the segment inherits the Files list fallback, which + * paints an options bar and a table header row that a document page does not have — chrome that has + * to be torn down a frame later. A detail page is header + body, so this is the header alone. + * + * Header actions are deliberately omitted: they are a function of the open file (a previewable + * non-markdown file gets a mode toggle, an editable one gets Share/Delete), which is exactly what is + * not yet known here. Chips appearing beside the title reads as content arriving; chips appearing + * and then changing reads as a glitch. + */ +export default function FilesFileLoading() { + return +} diff --git a/apps/sim/app/workspace/[workspaceId]/files/[fileId]/page.tsx b/apps/sim/app/workspace/[workspaceId]/files/[fileId]/page.tsx index 590e94b0816..b4808ec50dd 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/[fileId]/page.tsx +++ b/apps/sim/app/workspace/[workspaceId]/files/[fileId]/page.tsx @@ -1,17 +1,48 @@ import { Suspense } from 'react' +import { dehydrate, HydrationBoundary } from '@tanstack/react-query' import type { Metadata } from 'next' +import { getSession } from '@/lib/auth' +import { getQueryClient } from '@/app/_shell/providers/get-query-client' +import FilesFileLoading from '@/app/workspace/[workspaceId]/files/[fileId]/loading' import { Files } from '@/app/workspace/[workspaceId]/files/files' -import FilesLoading from '@/app/workspace/[workspaceId]/files/loading' +import { prefetchFilesBrowser } from '@/app/workspace/[workspaceId]/files/prefetch' export const metadata: Metadata = { title: 'Files', robots: { index: false }, } -export default function FilesFilePage() { +/** + * File detail entry. `Files` resolves the open file out of the workspace file LIST, so this route + * needs the same prefetch its sibling list page does — without it the server can only ever render + * the "resolving the record" spinner, and the real header (breadcrumbs, actions) has to pop in a + * frame later on the client. + * + * It also removes a whole class of hydration mismatch: which branch `Files` renders is decided by + * whether that list is in the cache, so a server render without it and a client render with it + * disagree on the header's markup (a static `…` crumb vs. the file's dropdown crumb). Prefetching + * here makes both sides read the same cache and pick the same branch by construction. + * + * `Files` reads URL query params via nuqs (`useSearchParams` internally), so it must sit under a + * Suspense boundary; the fallback is the detail chrome, matching the route's own `loading.tsx`. + */ +export default async function FilesFilePage({ + params, +}: { + params: Promise<{ workspaceId: string; fileId: string }> +}) { + const [{ workspaceId }, session] = await Promise.all([params, getSession()]) + + const queryClient = getQueryClient() + if (session?.user?.id) { + await prefetchFilesBrowser(queryClient, workspaceId, session.user.id) + } + return ( - }> - - + + }> + + + ) } diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/apply-streamed-markdown.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/apply-streamed-markdown.ts index 88f54d6c0b7..708bb00444c 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/apply-streamed-markdown.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/apply-streamed-markdown.ts @@ -2,10 +2,8 @@ import type { Editor } from '@tiptap/core' import { Node as PMNode } from '@tiptap/pm/model' import { initProseMirrorDoc, updateYFragment, ySyncPluginKey } from '@tiptap/y-tiptap' import * as Y from 'yjs' -import { parseMarkdownToDoc } from '../markdown-parse' - -/** The Yjs fragment name TipTap's Collaboration extension binds to (its default `field`). */ -const COLLAB_DOC_FIELD = 'default' +import { COLLAB_DOC_FIELD } from '@/lib/collab-doc/field' +import { editorNormalForm } from '../markdown-parse' /** * Transaction origin for agent-streamed writes into a live collaborative doc. It is deliberately NOT @@ -63,7 +61,11 @@ export function applyAgentStreamFrame( ): boolean { const binding = ySyncPluginKey.getState(editor.state)?.binding if (!binding) return false - const target = PMNode.fromJSON(editor.schema, parseMarkdownToDoc(body)) + // Through the editor's normal form, like every other writer to the shared document. A frame whose + // body ends on a list, heading, table, or rule parses WITHOUT the editor's trailing paragraph, so + // reconciling toward the bare parse deletes the one the seed put there — and the next client to bind + // writes it back, which is the divergence this normalization exists to prevent. + const target = PMNode.fromJSON(editor.schema, editorNormalForm(body)) let delta: Uint8Array | null = null const capture = (update: Uint8Array, origin: unknown) => { if (origin === AGENT_STREAM_ORIGIN) delta = update diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/collab-streaming-integration.test.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/collab-streaming-integration.test.ts index b8996800923..46183927ec1 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/collab-streaming-integration.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/collab-streaming-integration.test.ts @@ -164,7 +164,7 @@ describe('collab streaming integration — moving pieces', () => { reopened.destroy() }) - it('EMPTY-COLLAPSE ON THE STREAM PATH: an agent body with a huge blank run does not strand empties', () => { + it('EMPTY-BOUND ON THE STREAM PATH: an agent body with a huge blank run does not strand empties', () => { const A = makeCollabEditor() A.editor.commands.setContent(parseMarkdownToDoc('# Title\n\nintro'), { contentType: 'json' }) const session = beginAgentStream(A.editor)! @@ -173,9 +173,11 @@ describe('collab streaming integration — moving pieces', () => { endAgentStream(session) console.log( - `\n[STREAM-COLLAPSE] text=${JSON.stringify(A.editor.state.doc.textContent)} empty=${emptyParas(A.editor)}` + `\n[STREAM-BOUND] text=${JSON.stringify(A.editor.state.doc.textContent)} empty=${emptyParas(A.editor)}` ) - expect(emptyParas(A.editor)).toBe(0) // collapse protects the live streaming path, not just static open + // ~200 blank paragraphs' worth of run arrives; the parse bound caps what reaches the live doc, so the + // streaming path is protected exactly like a static open — no unbounded node explosion in the CRDT. + expect(emptyParas(A.editor)).toBe(20) expect(A.editor.state.doc.textContent).toContain('tail paragraph') }) @@ -197,4 +199,45 @@ describe('collab streaming integration — moving pieces', () => { expect(D.editor.state.doc.textContent).toContain('streamed body') expect(emptyParas(D.editor)).toBe(0) }) + + /** + * Every writer to the shared document has to produce the editor's normal form, or the one that does + * not silently removes what the others add. A frame whose body ends on a list, heading, table, or rule + * parses WITHOUT the trailing paragraph the seed puts there — reconciling toward that bare parse + * deleted it from the live room, and the next client to bind wrote it back, reopening the + * placeholder-vs-live divergence the seed normalization exists to close. + */ + it.each([ + ['ends on a list', ['# T\n\nintro\n\n- a', '# T\n\nintro\n\n- a\n- b']], + ['ends on a heading', ['# T\n\nintro\n\n## Sec', '# T\n\nintro\n\n## Section']], + ['ends on a table', ['# T\n\n| a |\n| --- |\n| 1 |']], + ])('AGENT STREAM KEEPS THE EDITOR NORMAL FORM: %s', (_label, frames) => { + const doc = markdownToYDoc('# T\n\nintro\n\n- seed') + const awareness = new Awareness(doc) + const editor = new Editor({ + extensions: createMarkdownEditorExtensions({ + placeholder: '', + collaboration: { + doc, + awareness, + user: { name: 'U', color: '#fff', clientId: doc.clientID }, + }, + }), + }) + const trailingIsEmptyParagraph = () => { + const fragment = doc.getXmlFragment('default') + const last = fragment.get(fragment.length - 1) + return last instanceof Y.XmlElement && last.nodeName === 'paragraph' && last.length === 0 + } + expect(trailingIsEmptyParagraph()).toBe(true) + + const session = beginAgentStream(editor)! + for (const frame of frames) applyAgentStreamFrame(editor, session, frame) + endAgentStream(session) + + expect(trailingIsEmptyParagraph()).toBe(true) + editor.destroy() + awareness.destroy() + doc.destroy() + }) }) diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/readiness.test.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/readiness.test.ts index 39bb68590fc..667638a78ea 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/readiness.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/readiness.test.ts @@ -4,11 +4,20 @@ import { describe, expect, it } from 'vitest' import { type CollabReadinessInputs, nextCollabReadiness } from './readiness' +/** An observation, with the healthy defaults filled in so each case states only what it exercises. */ +const at = (input: Partial): CollabReadinessInputs => ({ + synced: false, + seeded: false, + offlineSeed: false, + fatal: false, + ...input, +}) + /** Drive a sequence of observations through the latch, returning the readiness at each step. */ -function run(steps: CollabReadinessInputs[]): boolean[] { +function run(steps: Partial[]): boolean[] { let syncedOnce = false - return steps.map((input) => { - const next = nextCollabReadiness(syncedOnce, input) + return steps.map((step) => { + const next = nextCollabReadiness(syncedOnce, at(step)) syncedOnce = next.syncedOnce return next.ready }) @@ -16,21 +25,27 @@ function run(steps: CollabReadinessInputs[]): boolean[] { describe('nextCollabReadiness', () => { it('is not ready before syncing or seeding', () => { - const { syncedOnce, ready } = nextCollabReadiness(false, { - synced: false, - seeded: false, - offlineSeed: false, - }) + const { syncedOnce, ready } = nextCollabReadiness( + false, + at({ + synced: false, + seeded: false, + offlineSeed: false, + }) + ) expect(syncedOnce).toBe(false) expect(ready).toBe(false) }) it('is not ready when synced but not yet seeded', () => { - const { syncedOnce, ready } = nextCollabReadiness(false, { - synced: true, - seeded: false, - offlineSeed: false, - }) + const { syncedOnce, ready } = nextCollabReadiness( + false, + at({ + synced: true, + seeded: false, + offlineSeed: false, + }) + ) expect(syncedOnce).toBe(true) // latched expect(ready).toBe(false) // waits for the seed }) @@ -50,11 +65,14 @@ describe('nextCollabReadiness', () => { it('opens even if the seed lands before we ever observed synced (server seed proves a sync)', () => { // If the flap beat our first observation, the seed flag alone (not the offline fallback) proves a // completed sync happened. - const { syncedOnce, ready } = nextCollabReadiness(false, { - synced: false, - seeded: true, - offlineSeed: false, - }) + const { syncedOnce, ready } = nextCollabReadiness( + false, + at({ + synced: false, + seeded: true, + offlineSeed: false, + }) + ) expect(syncedOnce).toBe(true) expect(ready).toBe(true) }) @@ -74,4 +92,33 @@ describe('nextCollabReadiness', () => { ]) expect(readiness).toEqual([true, true]) }) + /** + * The reported bug. A brand-new file syncs EMPTY (latching `syncedOnce`), its server seed never + * lands, and the readiness deadline fires: the provider goes fatal and drops `synced` precisely so + * this gate closes. The offline fallback then seeds locally — and the sticky latch used to re-open + * the gate on that, handing back an editable editor bound to a document the provider had abandoned. + * Every keystroke was dropped (the provider ignores frames and never rejoins) and client autosave + * stayed off (collaboration is nominally on), so the edits vanished on reload with no error shown. + */ + it('stays read-only after the readiness deadline goes fatal, even though a sync was latched', () => { + const readiness = run([ + { synced: false }, + { synced: true }, // initial EMPTY sync — latches syncedOnce + { synced: false, fatal: true }, // deadline: provider drops synced and gives up + { seeded: true, offlineSeed: true, fatal: true }, // fallback seeds locally + ]) + expect(readiness).toEqual([false, false, false, false]) + }) + + /** + * The same revocation on an ALREADY-ready doc: access is withdrawn mid-session, the provider goes + * fatal, and readiness must be taken back rather than left latched open. + */ + it('revokes readiness when a live document turns fatal', () => { + const readiness = run([ + { synced: true, seeded: true }, // ready + { synced: false, seeded: true, fatal: true }, // access revoked mid-session + ]) + expect(readiness).toEqual([true, false]) + }) }) diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/readiness.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/readiness.ts index 343b301b394..9974de6ed43 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/readiness.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/readiness.ts @@ -22,16 +22,32 @@ export interface CollabReadinessInputs { seeded: boolean /** Whether the seed flag was set by the offline fallback (no server sync) rather than the server. */ offlineSeed: boolean + /** + * Whether the provider has GIVEN UP on this document — a non-retryable rejection, an access + * revocation, or the readiness deadline lapsing. A fatal provider ignores every inbound frame and + * never rejoins, so nothing typed after this point reaches the server. + */ + fatal: boolean } /** * Pure transition for the readiness latch. `syncedOnce` is the sticky prior state — pass the returned * `syncedOnce` back in on the next call. `ready` is whether the doc is synced-and-seeded. + * + * `fatal` overrides the latch, and that override is the whole reason it is an input. The latch is + * sticky on purpose, but stickiness must not outlive the document: a doc that syncs empty and never + * receives its server seed trips the readiness deadline, and the provider answers by dropping `synced` + * so this gate closes. The latch ignored that — `syncedOnce` was already set by the empty sync — so the + * offline fallback's seed flag re-opened the gate and handed back an EDITABLE editor on a document the + * provider had already abandoned. Nothing typed into it could persist: the provider drops every frame + * and never rejoins, and the client's own autosave stays gated off because collaboration is nominally + * on. The user types, sees no error, and loses the edits on reload. Revoking readiness on `fatal` is + * what makes the fallback what it is documented to be — a READ-ONLY view of the stored content. */ export function nextCollabReadiness( syncedOnce: boolean, input: CollabReadinessInputs ): { syncedOnce: boolean; ready: boolean } { const next = syncedOnce || input.synced || (input.seeded && !input.offlineSeed) - return { syncedOnce: next, ready: next && input.seeded } + return { syncedOnce: next, ready: next && input.seeded && !input.fatal } } diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity.ts index 43466f49861..4470187fefa 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity.ts @@ -173,10 +173,11 @@ function stripEmptyListItemLines(markdown: string): string { * round-trip ({@link stripEmptyListItemLines}), restores callout markers the serializer * backslash-escapes (`> \[!NOTE\]` → `> [!NOTE]`), and collapses trailing blank lines to a single * newline. Interior blank runs are NOT collapsed here — blank lines inside a fenced code block (or a - * verbatim raw-markdown-snippet) are significant, and a global collapse would corrupt them. Spurious - * interior blank runs between top-level blocks are removed upstream instead, by - * {@link parseMarkdownToDoc} stripping empty paragraphs, so a doc that has been through the editor - * never serializes with an interior blank run outside code in the first place. The table serializer's + * verbatim raw-markdown-snippet) are significant, and a global collapse would corrupt them. An interior + * run between top-level blocks is significant too: it is how an empty paragraph is written, and + * {@link parseMarkdownToDoc} reads exactly the count back out, so collapsing it here would delete the + * document's spacing. Only the TRAILING run is collapsed — it can carry no paragraph (see + * `clampEmptyParagraphs`) and would otherwise churn the file on every save. The table serializer's * spurious surrounding blank lines are trimmed at the source (PipeSafeTable), so no global * leading-newline strip is needed here — avoiding clobbering content that legitimately begins with * whitespace. diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-parse.test.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-parse.test.ts index 0067ef31f47..94c645fc2e2 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-parse.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-parse.test.ts @@ -91,45 +91,101 @@ describe('parseMarkdownToDoc (chunked)', () => { expect(splitMarkdownBlocks('\n\n \n')).toEqual([]) }) - // Asserts the collapse documented on `stripEmptyParagraphs` — at document edges, between blocks, for - // one or many blank lines, and around lists. (Blank runs are insignificant in markdown, so a collapsed - // file renders identically everywhere it's viewed; the pathological case is a run of thousands.) - describe('collapses blank-line runs to markdown-standard spacing', () => { - /** Block-type shape of a doc after `parseMarkdownToDoc`, `∅` for any surviving empty paragraph. */ - function shapeOf(md: string): string { - return (parseMarkdownToDoc(md).content ?? []) - .map((n) => (isEmptyPara(n) ? '∅' : n.type)) - .join(',') - } + /** Block-type shape of a doc after `parseMarkdownToDoc`, `∅` for each empty paragraph. */ + function shapeOf(md: string): string { + return (parseMarkdownToDoc(md).content ?? []) + .map((n) => (isEmptyPara(n) ? '∅' : n.type)) + .join(',') + } + // A blank line an author left between two blocks is part of the document, so parse must read back the + // exact count the serializer wrote (`blocks.join('\n\n')` ⇒ an empty paragraph costs TWO blank lines, + // the first separator is free). Getting this wrong is visible: the static placeholder is built from + // markdown while the live collaborative doc is the CRDT, so any drift shows up as the doc reflowing + // its spacing a beat after the file appears. + describe('preserves authored blank lines', () => { it.each([ - ['one blank gap between paragraphs', 'a\n\n\n\nb', 'paragraph,paragraph'], - ['many blank lines between paragraphs', 'a\n\n\n\n\n\n\n\nb', 'paragraph,paragraph'], - ['leading blank lines', '\n\n\n\na', 'paragraph'], - ['leading + interior', '\n\n\na\n\n\n\nb', 'paragraph,paragraph'], - ['blank gap between a heading and text', '# H\n\n\n\ntext', 'heading,paragraph'], - ['blank gap after a tight list', '- a\n- b\n\n\n\ntext', 'bulletList,paragraph'], - ['blank gap before a tight list', 'text\n\n\n\n- a\n- b', 'paragraph,bulletList'], - // Line-ending variants normalize first, so `\r`-only / CRLF blank runs collapse identically. - ['CRLF between blocks', 'a\r\n\r\n\r\n\r\nb', 'paragraph,paragraph'], - ['CR-only (classic Mac) between blocks', 'a\r\r\r\rb', 'paragraph,paragraph'], - ])('collapses to no empty paragraphs: %s', (_label, md, expected) => { + ['single separator — no empty paragraph', 'a\n\nb', 'paragraph,paragraph'], + ['odd blank line is insignificant', 'a\n\n\nb', 'paragraph,paragraph'], + ['one authored blank line', 'a\n\n\n\nb', 'paragraph,∅,paragraph'], + ['three authored blank lines', 'a\n\n\n\n\n\n\n\nb', 'paragraph,∅,∅,∅,paragraph'], + ['leading blank lines', '\n\n\n\na', '∅,∅,paragraph'], + ['leading + interior', '\n\n\na\n\n\n\nb', '∅,paragraph,∅,paragraph'], + ['between a heading and text', '# H\n\n\n\ntext', 'heading,∅,paragraph'], + ['after a tight list', '- a\n- b\n\n\n\ntext', 'bulletList,∅,paragraph'], + ['before a tight list', 'text\n\n\n\n- a\n- b', 'paragraph,∅,bulletList'], + // Line-ending variants normalize first, so `\r`-only / CRLF runs count identically. + ['CRLF between blocks', 'a\r\n\r\n\r\n\r\nb', 'paragraph,∅,paragraph'], + ['CR-only (classic Mac) between blocks', 'a\r\r\r\rb', 'paragraph,∅,paragraph'], + ])('%s', (_label, md, expected) => { + expect(shapeOf(md)).toBe(expected) + }) + + // A loose list's own internal blank lines are absorbed into its merged block, so they stay list + // spacing rather than becoming top-level paragraphs that would split the list in two. + it('a loose list keeps its internal blank lines as one list', () => { + expect(shapeOf('- a\n\n- b\n\n- c')).toBe('bulletList') + }) + + // …but a gap WIDE enough to carry an empty paragraph is a top-level block boundary: the serializer + // only writes one by emitting the two sides as separate blocks, so re-merging them made parse stop + // inverting serialize. That swallowed the paragraph, fused the two blocks, and — because the file + // then never reached a fixpoint — silently opened it READ-ONLY. + it.each([ + ['between two bullet lists', '- a\n\n\n\n- b', 'bulletList,∅,bulletList'], + ['between two blockquotes', '> a\n\n\n\n> b', 'blockquote,∅,blockquote'], + ['before an indented continuation', 'a\n\n\n\n indented', 'paragraph,∅,paragraph'], + ])('a gap that carries a paragraph breaks the merge: %s', (_label, md, expected) => { expect(shapeOf(md)).toBe(expected) }) it('a pathological blank run does not explode into empty paragraph nodes', () => { // The production incident: an agent/paste artifact with a huge blank run became ~1959 empty - // paragraphs baked into the doc. Collapsing on parse neutralizes any such source. + // paragraphs baked into the doc. The run is bounded on parse, so no source can reach that. const body = `Para A${'\n'.repeat(4000)}Para B` const content = parseMarkdownToDoc(body).content ?? [] - expect(content.filter(isEmptyPara).length).toBe(0) - expect(content.map((n) => n.type)).toEqual(['paragraph', 'paragraph']) + expect(content.filter(isEmptyPara).length).toBe(20) + expect(content.length).toBe(22) + }) + + // The per-gap ceiling bounds one run; the realistic artifact shape is a moderate run between EVERY + // paragraph, which scales with file size. Without a document budget an 86KB body produced ~40k empty + // paragraphs — twenty times the incident the per-gap ceiling exists to prevent. + it('many blank runs cannot explode the document either', () => { + const body = `${'x'.padEnd(1)}${`${'\n'.repeat(42)}x`.repeat(2000)}` + const content = parseMarkdownToDoc(body).content ?? [] + expect(content.filter(isEmptyPara).length).toBe(500) + }) + + // The bounds have to be fixpoints too, or a clamped file would churn on every save. + it.each([ + ['one huge run', `Para A${'\n'.repeat(4000)}Para B`], + ['many runs past the document budget', `x${`${'\n'.repeat(42)}x`.repeat(2000)}`], + ])('a bounded document re-serializes to itself: %s', (_label, md) => { + const once = serializeMarkdownBody(md) + expect(serializeMarkdownBody(once)).toBe(once) + }) + + // The whole-document path hands blank runs to @tiptap/markdown, which keeps them after a paragraph + // but swallows them after a heading/ordered list/table. Preserving only some would break the fixpoint + // for the same document, so that path keeps none — consistently zero, which IS a fixpoint. + it.each([ + ['block HTML', '# H\n\n\n\ntext\n\n
x
', 'heading,paragraph,rawHtmlBlock'], + [ + 'a reference definition', + '# H\n\n\n\nsee [y][r]\n\n[r]: https://e.com', + 'heading,paragraph', + ], + ])('a document that must parse whole keeps no empty paragraphs: %s', (_label, md, expected) => { + expect(shapeOf(md)).toBe(expected) + const once = serializeMarkdownBody(md) + expect(serializeMarkdownBody(once)).toBe(once) }) }) - // Regression: a file with blank lines (leading, interior, or trailing) must stay EDITABLE. Collapsing - // blank runs keeps serialize→parse idempotent, so the round-trip-safety probe reaches a fixed point - // instead of flipping the file read-only. + // Regression: a file with blank lines (leading, interior, or trailing) must stay EDITABLE — parse and + // serialize have to agree on the blank count, or the round-trip-safety probe never reaches a fixed + // point and the file silently opens read-only. describe('blank lines stay editable (regression)', () => { it.each([ ['plain paragraph', 'abc\n\n'], @@ -137,15 +193,27 @@ describe('parseMarkdownToDoc (chunked)', () => { ['three trailing newlines', 'hello\n\n\n'], ['two paragraphs', 'para one\n\npara two\n\n'], ['interior blank run + trailing', 'a\n\n\n\nb\n\n'], + ['many interior blank runs', '# T\n\n\n\na\n\n\n\n\n\nb\n\n\n\n- x\n- y\n\n'], + ['leading blank run', '\n\n\n\nabc\n'], + // These regressed to read-only when a gap carrying a paragraph was still merged away: the merge + // fused the two blocks, so the second pass produced different markdown from the first. + ['gap before a list glued to a lead-in line', 'text\n1. one\n\n\n\n- bullet'], + ['gap between two glued list kinds', 'text\n- bullet\n\n\n\n1. one'], + ['gap between two blockquotes after a lead-in', 'text\n> a\n\n\n\n> b'], + [ + 'changelog shape', + '## v2\n\nHighlights:\n1. faster\n2. smaller\n\n\n\n- also: fixed a crash\n', + ], ])('a file with blank lines is round-trip-safe: %s', (_label, md) => { expect(isRoundTripSafe(md)).toBe(true) }) - it('removes only structurally-empty paragraphs — a paragraph with content survives', () => { - // The shape suite above already proves leading/interior/trailing blank runs collapse to zero empty - // paragraphs; this pins the complementary guarantee — a real (non-empty) paragraph is never dropped. + // Trailing empties are the one kind that cannot round-trip: `postProcessSerializedMarkdown` + // collapses trailing blank lines, so keeping them would make the doc differ from its own output. + it('drops trailing empty paragraphs', () => { + expect(shapeOf('abc\n\n')).toBe('paragraph') + expect(shapeOf('abc\n\n\n\n\n\n')).toBe('paragraph') const trailing = parseMarkdownToDoc('abc\n\n').content ?? [] - expect(trailing.at(-1)?.type).toBe('paragraph') expect(isEmptyPara(trailing.at(-1) ?? {})).toBe(false) }) }) @@ -247,16 +315,25 @@ const FUZZ_BLOCKS: Array<(r: () => number) => string> = [ () => 'See [the docs][ref].\n\n[ref]: https://example.com/docs', ] -function buildFuzzDoc(seed: number): string { +/** + * `blankRuns` widens the separator from a single blank line to a run of up to three, so the corpus + * exercises authored spacing. The single-separator corpus structurally could not: every document it + * built was `parts.join('\n\n')`, which is exactly the one gap width that carries no empty paragraph — + * so the whole blank-line design was invisible to the property test that claims to cover any input. + */ +function buildFuzzDoc(seed: number, blankRuns: boolean): string { const r = rng(seed) const count = 2 + Math.floor(r() * 8) const parts: string[] = [] - for (let i = 0; i < count; i++) parts.push(FUZZ_BLOCKS[Math.floor(r() * FUZZ_BLOCKS.length)](r)) - return parts.join('\n\n') + for (let i = 0; i < count; i++) { + if (i > 0) parts.push('\n'.repeat(blankRuns ? 2 + Math.floor(r() * 4) : 2)) + parts.push(FUZZ_BLOCKS[Math.floor(r() * FUZZ_BLOCKS.length)](r)) + } + return parts.join('') } describe('chunked parse — property test over randomized documents', () => { - it('chunked === one-shot for every document, and idempotent for every editable one', () => { + it('chunked === one-shot on single-separator documents, and idempotent for every editable one', () => { const failures: Array<{ seed: number; kind: string }> = [] // Compare modulo trailing whitespace: `parseMarkdownToDoc` strips trailing empty paragraphs (they // can't be serialized stably — postProcess collapses trailing newlines — so keeping them would flip @@ -264,17 +341,48 @@ describe('chunked parse — property test over randomized documents', () => { // intended and invisible after save; interior/leading fidelity is still compared exactly. const trimEnd = (md: string) => md.replace(/\n+$/, '') for (let seed = 1; seed <= 400; seed++) { - const body = buildFuzzDoc(seed) + const body = buildFuzzDoc(seed, false) const chunked = serializeMarkdownBody(body) - // Fidelity is the load-bearing invariant — chunked must never diverge from the whole-document - // parse, for ANY input; idempotency only needs to hold where the doc is editable (raw HTML is - // non-idempotent in the underlying editor regardless of chunking, which is why it opens read-only). + // On documents with no authored blank run the two paths must still agree exactly. They are allowed + // to differ once a gap carries an empty paragraph: the chunked path reconstructs it and the + // whole-document path deliberately keeps none (see `parseMarkdownToDoc`), and only ONE path ever + // runs for a given document. Idempotency is the invariant that must hold for both, and it is + // asserted for every editable document in the blank-run corpus below. if (trimEnd(chunked) !== trimEnd(oneShot(body))) failures.push({ seed, kind: 'fidelity' }) else if (isRoundTripSafe(body) && serializeMarkdownBody(chunked) !== chunked) { failures.push({ seed, kind: 'idempotency' }) } } expect(failures).toEqual([]) - // 400 docs each parsed+serialized twice — generous timeout so it can't flake under parallel load. - }, 30000) + // 400 docs each parsed+serialized twice. Measured ~10s alone; the whole-suite run gives each worker + // a fraction of a core, and at 30s BOTH property tests in this file timed out there while passing + // standalone. Sized off the loaded number, not the isolated one. + }, 60000) + + /** + * Idempotency is what keeps a file editable: `isRoundTripSafe` opens a document read-only unless + * serializing twice is byte-identical. Preserving blank lines put every gap width on that path, and a + * merge rule that swallowed a gap silently flipped ordinary documents (a changelog, a lead-in line + * followed by a list) to read-only. Fuzz the separator width so that class cannot come back. + * + * Gated on `isRoundTripSafe` for the same reason the single-separator test above is: a document the + * probe rejects opens read-only and is never re-serialized, so its instability is contained by design. + * This corpus does surface such documents — a blank run INSIDE a loose list parses to an empty + * paragraph nested in a list item, which `getMarkdown` writes as an indented `' '` marker line rather + * than a blank one, and that does not round-trip. That defect predates blank-line preservation (it + * reproduces identically with the empty-paragraph strip in place) and is only reachable through a gap + * width the old corpus could not generate; the probe correctly holds those files read-only. + */ + it('stays idempotent with authored blank runs of every width', () => { + const failures: Array<{ seed: number; body: string }> = [] + for (let seed = 1; seed <= 400; seed++) { + const body = buildFuzzDoc(seed, true) + if (!isRoundTripSafe(body)) continue + const once = serializeMarkdownBody(body) + if (serializeMarkdownBody(once) !== once) failures.push({ seed, body }) + } + expect(failures).toEqual([]) + // Same budget as the corpus above, for the same reason — this is the second ~10s property test in + // the file, and adding it is what pushed both past 30s under whole-suite parallelism. + }, 60000) }) diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-parse.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-parse.ts index 6cdeeabf4ae..fd8cb706a34 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-parse.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-parse.ts @@ -47,14 +47,64 @@ const FENCE_CLOSE = /^ {0,3}(`{3,}|~{3,})[ \t]*$/ const LIST_MARKER = /^[ ]{0,3}(?:[-*+]|\d+[.)])\s/ const BLOCKQUOTE = /^[ ]{0,3}>/ +/** + * Ceiling on the empty paragraphs one gap may carry. Deliberate spacing is a handful of blank lines; a + * run of thousands is an agent/paste artifact, and baking a node per blank would put thousands of empty + * paragraphs in the document forever (the reported incident: ~1959 nodes from one 4000-newline run). + * Well past any spacing a person types, low enough that no single gap can explode. + */ +const MAX_CONSECUTIVE_EMPTY_PARAGRAPHS = 20 + +/** + * Ceiling on a document's TOTAL empty paragraphs, enforced by {@link boundEmptyParagraphs}. The per-gap + * ceiling alone bounds nothing at document scale — the realistic artifact shape is a moderate blank run + * between every paragraph, not one giant run, and that scales linearly with file size. Generous enough + * that no hand-spaced document reaches it, finite so a machine-generated one cannot grow the node count + * without limit. + */ +const MAX_EMPTY_PARAGRAPHS_PER_DOC = 500 + +/** + * How many empty paragraphs a run of `blankLines` between two blocks carries. + * + * The serializer joins top-level blocks with a blank line (`blocks.join('\n\n')`), so an empty + * paragraph costs TWO blank lines — its own, plus the separator that follows it — while the first + * separator is free. Inverting that join is the whole rule: an interior gap of `b` blank lines carries + * `(b - 1) / 2` empty paragraphs, a leading gap (no preceding block, so no free separator) carries + * `b / 2`, and both round down. A hand-authored odd blank line is insignificant in markdown and + * collapses, exactly as every standard renderer shows it; a gap the editor itself wrote reconstructs + * exactly, which is what makes parse ∘ serialize a fixed point. + * + * The count is computed here rather than delegated to `@tiptap/markdown`, whose own blank-run handling + * is not self-consistent: after a paragraph, list, blockquote, code fence, rule, or image it follows the + * same `(b - 1) / 2`, but after a heading, an ordered list, or a table the token swallows the whole run + * and yields nothing. Delegating would mean a blank line after a heading could never survive a save. + * + * Bounded here as well as in {@link clampEmptyParagraphs} so a pathological run is never materialized + * in the first place — a megabyte of newlines would otherwise allocate half a million throwaway nodes + * on its way to being clamped back down to {@link MAX_CONSECUTIVE_EMPTY_PARAGRAPHS}. + */ +function emptyBlockCount(blankLines: number, leading: boolean): number { + const count = Math.floor((blankLines - (leading ? 0 : 1)) / 2) + return Math.max(0, Math.min(count, MAX_CONSECUTIVE_EMPTY_PARAGRAPHS)) +} + /** * Split a markdown body into top-level blocks that can each be parsed independently and reassembled - * without changing meaning. Blank lines separate candidate groups (fenced code blocks stay atomic), - * then adjacent groups are merged back together whenever they could form one logical block: any - * indented (continuation) group, and consecutive list/blockquote groups (which would otherwise be a - * single loose list/quote). Merging is intentionally conservative — over-merging only yields a larger - * chunk, whereas under-merging would shatter a structure — and every block is parsed by - * `@tiptap/markdown`'s own lexer, so block boundaries always match the parser. + * (by `join('\n\n')`) without changing meaning. Blank lines separate candidate groups (fenced code + * blocks stay atomic), then adjacent groups are merged back together whenever they could form one + * logical block: any indented (continuation) group, and consecutive list/blockquote groups (which + * would otherwise be a single loose list/quote). Merging is intentionally conservative — over-merging + * only yields a larger chunk, whereas under-merging would shatter a structure — and every non-empty + * block is parsed by `@tiptap/markdown`'s own lexer, so block boundaries always match the parser. + * + * An EMPTY string in the result is a blank line the author left between two blocks — the exact inverse + * of the serializer's block join (see {@link emptyBlockCount}), so a document's deliberate spacing + * survives the round-trip instead of being silently dropped. {@link parseMarkdownToDoc} turns each into + * an empty paragraph; a run is bounded there by {@link clampEmptyParagraphs}. Gaps are measured before + * merging, so blank lines absorbed INTO a merged block (a loose list's own internal spacing) never + * become paragraphs — only gaps between the final top-level blocks do. Trailing blank lines carry + * nothing: the serializer collapses them to a single newline, so keeping them would never round-trip. * * The indent-merge rule is load-bearing for fenced code indented past 3 spaces (e.g. inside a list * item): {@link FENCE_OPEN} only tracks fences at the document margin, so a nested fence's interior @@ -67,10 +117,17 @@ export function splitMarkdownBlocks(body: string): string[] { // block (defeating the chunker). The editor normalizes `\r` on parse anyway, so meaning is unchanged. const lines = body.replace(/\r\n?/g, '\n').split('\n') const groups: string[] = [] + /** Blank lines immediately preceding `groups[i]`, parallel to it. */ + const gaps: number[] = [] + let blanks = 0 let current: string[] = [] let fence: string | null = null const flush = () => { - if (current.length > 0) groups.push(current.join('\n')) + if (current.length > 0) { + groups.push(current.join('\n')) + gaps.push(blanks) + blanks = 0 + } current = [] } for (const line of lines) { @@ -87,7 +144,9 @@ export function splitMarkdownBlocks(body: string): string[] { continue } if (line.trim() === '') { + // Flush BEFORE counting: `blanks` is the gap that preceded the group being closed here. flush() + blanks++ continue } current.push(line) @@ -97,18 +156,33 @@ export function splitMarkdownBlocks(body: string): string[] { // Build continuation runs and join each once — concatenating onto the growing block per group would be // O(n²) for one long loose list. A group continues the run when indented, or when its first line and the // group open the same marker kind (list or blockquote) — i.e. they form one loose list/quote. - const runs: string[][] = [] - for (const group of groups) { - const head = runs.length > 0 ? runs[runs.length - 1][0] : null + const runs: Array<{ empties: number; parts: string[] }> = [] + for (let index = 0; index < groups.length; index++) { + const group = groups[index] + const previous = runs.length > 0 ? runs[runs.length - 1] : null + const head = previous?.parts[0] ?? null + // A gap wide enough to carry an empty paragraph IS a top-level block boundary: the serializer only + // writes one by emitting the two sides as separate blocks, so merging across it swallowed the + // paragraph AND fused the two blocks (`- a` ∅ `- b` became one list, `> a` ∅ `> b` one quote, an + // indented continuation absorbed the gap). Parse then stopped inverting serialize, so the file never + // reached a fixpoint and silently opened READ-ONLY on the next open. + const empties = emptyBlockCount(gaps[index], index === 0) const continues = head !== null && + empties === 0 && (/^\s/.test(group) || (LIST_MARKER.test(head) && LIST_MARKER.test(group)) || (BLOCKQUOTE.test(head) && BLOCKQUOTE.test(group))) - if (continues) runs[runs.length - 1].push(group) - else runs.push([group]) + if (continues) previous?.parts.push(group) + else runs.push({ empties, parts: [group] }) } - return runs.map((run) => run.join('\n\n')) + + const blocks: string[] = [] + for (const run of runs) { + for (let n = run.empties; n > 0; n--) blocks.push('') + blocks.push(run.parts.join('\n\n')) + } + return blocks } /** @@ -121,11 +195,18 @@ export function splitMarkdownBlocks(body: string): string[] { * Documents whose constructs span blocks ({@link NON_CHUNKABLE}) parse whole, and any failure falls * back to a single whole-document parse, so correctness never depends on the splitter. * - * Runs of blank lines take the fast chunked path too: the chunker parses each block stripped of the - * blank lines between them, which drops the empty paragraphs `@tiptap/markdown` reconstructs from a - * blank run — exactly what {@link stripEmptyParagraphs} does to the whole-parse output anyway. A blank - * run between blocks is insignificant in markdown, so collapsing it is the intended normalization (see - * {@link stripEmptyParagraphs}), and both parse paths converge on the same empty-paragraph-free result. + * A blank line the author left between two blocks is part of the document, not noise: the chunker hands + * it back as an empty block (see {@link splitMarkdownBlocks}) and it becomes an empty paragraph here, so + * the editor renders the spacing that is actually in the file — on the very first paint, with no reflow + * once a collaborative doc settles. + * + * The whole-document path CANNOT do that. It hands blank runs to `@tiptap/markdown`, whose handling is + * not self-consistent (see {@link emptyBlockCount}), so a blank line there survives after a paragraph but + * is swallowed after a heading, an ordered list, or a table. Preserving it on only some of those would + * make parse stop inverting serialize for the same document — the file would never reach a fixpoint and + * would open read-only. So that path keeps NO empty paragraphs: consistently zero is a fixpoint, and a + * document whose spacing cannot be represented is better rendered the way every other markdown renderer + * shows it than rendered one way and saved another. */ export function parseMarkdownToDoc(body: string): JSONContent { const manager = markdownManager() @@ -133,22 +214,23 @@ export function parseMarkdownToDoc(body: string): JSONContent { // the chunker and parser do — a classic `\r`-only body would otherwise slip past the reference-def / // block-HTML guard and be chunked, shattering a construct that must parse whole. const normalized = body.replace(/\r\n?/g, '\n') - let doc: JSONContent - if (NON_CHUNKABLE.test(normalized)) { - doc = manager.parse(normalized) - } else { - try { - const content: JSONContent[] = [] - for (const block of splitMarkdownBlocks(normalized)) { - // `MarkdownManager.parse` always returns a doc node with a `content` array; spread its blocks. - content.push(...(manager.parse(block).content ?? [])) + if (NON_CHUNKABLE.test(normalized)) return boundEmptyParagraphs(manager.parse(normalized), 0) + try { + const content: JSONContent[] = [] + for (const block of splitMarkdownBlocks(normalized)) { + // An empty block is the chunker's marker for an authored blank line, and + // `MarkdownManager.parse('')` yields a doc with no blocks — so materialize the node directly. + if (block === '') { + content.push({ type: 'paragraph' }) + continue } - doc = { type: 'doc', content } - } catch { - doc = manager.parse(normalized) + // `MarkdownManager.parse` always returns a doc node with a `content` array; spread its blocks. + content.push(...(manager.parse(block).content ?? [])) } + return boundEmptyParagraphs({ type: 'doc', content }, MAX_EMPTY_PARAGRAPHS_PER_DOC) + } catch { + return boundEmptyParagraphs(manager.parse(normalized), 0) } - return stripEmptyParagraphs(doc) } /** An empty paragraph node — the shape a blank line reconstructs to (no content, or `content: []`). */ @@ -157,26 +239,72 @@ function isEmptyParagraph(node: JSONContent): boolean { } /** - * Drop ALL top-level empty paragraphs from a parsed doc — leading, interior, and trailing. In markdown - * a run of blank lines between blocks is insignificant (CommonMark collapses it), so `@tiptap/markdown` - * reconstructing each blank as an empty paragraph node is not fidelity: it makes the editor render the - * file differently from every standard renderer (GitHub, the download, our own static preview), and a - * pathological blank run (an agent/paste artifact) explodes into thousands of empty nodes that persist - * forever and reflow the doc on open. Collapsing them here keeps normal single-blank-line block spacing - * while removing the spurious gaps, and stays idempotent so the round-trip-safety probe still passes: a - * doc parsed this way has no empty paragraphs, so re-serializing it never re-emits an interior blank run - * (the serializer is intentionally left alone — a blank line inside a fenced code block IS significant), - * and a second parse is a fixed point. Only TOP-LEVEL paragraphs are touched, so blank lines that carry - * meaning inside a construct (e.g. a loose list) are left to the block parser. TipTap re-adds its own - * trailing filler paragraph on `setContent`, so the editor still has a place to type. + * Bound the top-level empty paragraphs of a parsed doc to `budget` in total, and drop trailing ones + * entirely. `budget` is 0 for the whole-document path, which cannot represent them at all. + * + * The per-gap ceiling in {@link emptyBlockCount} bounds one run; this bounds the DOCUMENT. Without it the + * ceiling buys nothing against the shape a real artifact takes — an export that puts a moderate blank run + * between every paragraph, rather than one giant run. Measured before this budget existed: an 86KB body + * of `x` + 42 newlines produced 39,980 empty paragraphs, twenty times the incident the ceiling cites. + * + * Trailing empties cannot round-trip — `postProcessSerializedMarkdown` collapses trailing blank lines to + * a single newline, so a trailing empty paragraph would be re-serialized away and the doc would differ + * from its own output, flipping the file read-only. Dropping them here is what keeps the probe stable + * (TipTap re-adds its own trailing filler paragraph on `setContent`, so there is still somewhere to + * type). Interior and leading empties DO round-trip exactly, so they are kept. + * + * Only TOP-LEVEL paragraphs are considered — blank lines that carry meaning inside a construct (a loose + * list, a blockquote) live below the doc root and belong to the block parser. Returns the doc untouched, + * with no array copy, when nothing needs bounding (the overwhelmingly common case). */ -function stripEmptyParagraphs(doc: JSONContent): JSONContent { +function boundEmptyParagraphs(doc: JSONContent, budget: number): JSONContent { const content = doc.content if (!content || content.length === 0) return doc - // The dominant (chunked) parse already emits no top-level empty paragraphs, so scan before allocating: - // return the doc untouched — no array copy — unless there is actually something to strip. + // Most documents carry no empty paragraph at all, so scan before allocating anything. if (!content.some(isEmptyParagraph)) return doc - return { ...doc, content: content.filter((node) => !isEmptyParagraph(node)) } + let end = content.length + while (end > 0 && isEmptyParagraph(content[end - 1])) end-- + const kept: JSONContent[] = [] + let remaining = budget + for (let index = 0; index < end; index++) { + const node = content[index] + if (!isEmptyParagraph(node)) { + kept.push(node) + continue + } + if (remaining > 0) { + remaining-- + kept.push(node) + } + } + return kept.length === content.length ? doc : { ...doc, content: kept } +} + +/** + * The markdown parse in the form the EDITOR settles on — the only shape that may enter the shared + * document. + * + * ProseMirror appends an empty paragraph to any document that does not end in one, so a parse ending on + * a list, heading, table, or rule is NOT what a bound editor holds. Seeding the CRDT with the + * un-normalized shape means the first client to bind writes that paragraph back into the SHARED + * document — and because a trailing blank line does not survive serialization + * (`postProcessSerializedMarkdown` collapses it) the file never records it, so nothing reconciles the + * two and a client that seeds without seeing another's contribution adds one more. Measured on a + * heavily-reopened document: 18 stacked empty paragraphs in the live doc against the placeholder's 1 — + * the pane growing several hundred pixels the instant the live editor took over. + * + * Opt-in rather than folded into {@link parseMarkdownToDoc}, because only a writer to the SHARED + * document has to agree with the editor. Every other consumer of the parse (paste, the round-trip + * probe, the read-only placeholder) is rendered through a real editor that applies this itself, and + * baking it into the parse changes what those surfaces assert. Every CRDT writer — the seed, the agent + * merge, and the streaming frame reconciler — must go through here, or the one that does not silently + * removes what the others add. + */ +export function editorNormalForm(markdown: string): JSONContent { + const json = parseMarkdownToDoc(markdown) + const content = json.content ?? [] + if (content[content.length - 1]?.type === 'paragraph') return json + return { ...json, content: [...content, { type: 'paragraph' }] } } /** diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx index 4bc6285eedb..0eb5f3d72c0 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx @@ -721,7 +721,39 @@ export function LoadedRichMarkdownEditor({ * is latched, so a fatal rejection that fired before this subscription is not missed. */ useEffect(() => { + /** + * Reveal the live document only once it has stopped moving. + * + * `synced` fires when the sync handshake completes, but the room's remaining updates can still be + * in flight — reconnecting to a room that was edited moments ago, the handshake lands on the base + * state and the edit arrives a few milliseconds later. Flipping readiness on `synced` alone + * therefore un-hides the live editor onto an INTERMEDIATE state and lets the document correct + * itself in view: measured on a reload right after a block move, the editor was revealed showing + * the pre-move order and re-ordered 7ms later. The placeholder was showing the right thing the + * whole time, so the swap is the only reason anything moved. + * + * So hold the reveal until one animation frame passes with no update to the shared document; any + * update restarts the wait. Deferred readiness costs at most a frame of the placeholder — which is + * already rendering the correct content — while going NOT-ready stays immediate, so nothing can + * keep a fatal or unsynced document editable a moment longer than before. + */ + let settleFrame: number | null = null + const cancelSettle = () => { + if (settleFrame !== null) cancelAnimationFrame(settleFrame) + settleFrame = null + } const setReady = (ready: boolean) => { + cancelSettle() + if (ready) { + settleFrame = requestAnimationFrame(() => { + settleFrame = null + applyReady(true) + }) + return + } + applyReady(false) + } + const applyReady = (ready: boolean) => { // Child-local: gates editability (a user must never type into an unsynced/unseeded doc). setCollabReady(ready) // Parent: gates CLIENT autosave. In a collaborative session the relay persists the doc to @@ -730,14 +762,16 @@ export function LoadedRichMarkdownEditor({ // Only the non-collaborative (solo) path client-autosaves. onCollabReadyChange(collaboration ? false : ready) } + // No shared document to settle on the solo path, so apply directly — deferring a frame would only + // delay autosave, and this branch returns before the cleanup that cancels a pending frame. if (!collaboration) { - setReady(true) + applyReady(true) return } const { provider, doc } = collaboration if (!editor) { setReady(false) - return + return cancelSettle } const config = doc.getMap(FILE_DOC_SEED.configMap) @@ -766,12 +800,21 @@ export function LoadedRichMarkdownEditor({ const report = () => { const synced = provider.synced const seeded = config.get(FILE_DOC_SEED.flag) === true - const next = nextCollabReadiness(syncedOnce, { synced, seeded, offlineSeed }) + // `joinError` is latched ONLY on the provider's fatal paths (non-retryable rejection, access + // revocation, readiness deadline), so it is exactly "this document is abandoned". + const fatal = provider.joinError !== null + const next = nextCollabReadiness(syncedOnce, { synced, seeded, offlineSeed, fatal }) syncedOnce = next.syncedOnce setReady(next.ready) } + /** + * Re-report unconditionally, not just when the fallback seeds. A fatal that arrives on an ALREADY + * seeded doc (access revoked mid-session) leaves `seedFromLoaded` a no-op, so nothing else would + * fire an observer and the editor would stay editable on a document the provider has abandoned. + */ const onJoinError = (error: JoinFileDocError) => { if (error.retryable === false) seedFromLoaded() + report() } // A server edit that changes ONLY the frontmatter (e.g. copilot) updates the config map but not @@ -792,18 +835,31 @@ export function LoadedRichMarkdownEditor({ } } + /** An update while the reveal is pending means the document is still catching up — wait again. */ + const restartSettle = () => { + if (settleFrame === null) return + cancelSettle() + settleFrame = requestAnimationFrame(() => { + settleFrame = null + applyReady(true) + }) + } + provider.on('synced', report) provider.on('join-error', onJoinError) config.observe(report) config.observe(syncFrontmatter) + doc.on('update', restartSettle) report() if (provider.joinError) onJoinError(provider.joinError) return () => { + cancelSettle() provider.off('synced', report) provider.off('join-error', onJoinError) config.unobserve(report) config.unobserve(syncFrontmatter) + doc.off('update', restartSettle) // Report NOT ready on teardown — the safe direction. If this effect ever re-runs while mounted // (a future dep change), briefly gating autosave off is harmless; reporting `true` here could // ungate it while the doc is unready. diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/use-editable-file-content.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/use-editable-file-content.ts index f8dac76bbdd..db6206c43b1 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/use-editable-file-content.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/use-editable-file-content.ts @@ -178,7 +178,15 @@ export function useEditableFileContent({ file.id, file.key, GENERATED_SOURCE_FILE_TYPES.has(file.type), - { refetchInterval: reconcileRefetchInterval } + { + refetchInterval: reconcileRefetchInterval, + // `canAutosave: false` on this surface means a server-side owner holds durability — the + // collaborative relay, which projects the live document to markdown itself and merges + // external writes INTO that document. There is nothing a focus refetch of the durable bytes + // can teach the editor that the shared document does not already have; all it does is + // re-read a storage key the relay's last save has already rotated away from. + refetchOnWindowFocus: canAutosave, + } ) /** diff --git a/apps/sim/app/workspace/[workspaceId]/files/prefetch.ts b/apps/sim/app/workspace/[workspaceId]/files/prefetch.ts index 250a6e5f713..90e653dbd8c 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/prefetch.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/prefetch.ts @@ -1,33 +1,33 @@ import type { QueryClient } from '@tanstack/react-query' import { listWorkspaceFileFolders } from '@/lib/uploads/contexts/workspace/workspace-file-folder-manager' -import { listWorkspaceFilesWithShares } from '@/lib/workspace-files/queries' import { getWorkspaceHostContextForViewer } from '@/lib/workspaces/host-context' import { prefetchResourceListChrome } from '@/app/workspace/[workspaceId]/lib/prefetch-resource-list-chrome' import { WORKSPACE_FILE_FOLDERS_STALE_TIME, workspaceFileFolderKeys, } from '@/hooks/queries/workspace-file-folders' -import { - WORKSPACE_FILES_LIST_STALE_TIME, - workspaceFilesKeys, -} from '@/hooks/queries/workspace-files' /** - * Prefetches everything the Files browser needs to paint a complete, correctly-ordered - * first frame: workspace files, file folders, and (via {@link prefetchResourceListChrome}) - * the pinned ids that drive row order plus the members behind the Owner column — - * under the same query keys their client hooks (`useWorkspaceFiles`, - * `useWorkspaceFileFolders`) use (scope `active`), so the browser paints - * populated on first render. + * Prefetches what the Files browser needs on top of the workspace layout's own prefetch, so the + * first frame is complete and correctly ordered: file folders, and (via + * {@link prefetchResourceListChrome}) the pinned ids that drive row order plus the members behind + * the Owner column — under the same query keys their client hooks (`useWorkspaceFileFolders`) use + * (scope `active`), so the browser paints populated on first render. + * + * The FILE LIST itself is deliberately not here: the sidebar reads it on every workspace route, so + * it is prefetched by `prefetchWorkspaceSidebar` in the layout — the only boundary that renders + * before the sidebar registers the query. Prefetching it again here would re-read it per request + * and still not reach the server render (`HydrationBoundary` defers an already-seen query to an + * effect, which SSR never runs). See the note on that entry. * - * Files and folders read the data layer; both payloads are shaped to their route contract so - * a hydrated entry matches a client fetch. Everything else still goes through its route — - * see {@link prefetchInternalJson}. + * Folders read the data layer; the payload is shaped to its route contract so a hydrated entry + * matches a client fetch. Everything else still goes through its route — see + * {@link prefetchInternalJson}. * - * Those two reads carry no authorization of their own, so the viewer is proved first. This - * reuses the layout's `cache`d host-context lookup rather than re-deriving the permission, - * so it costs no additional queries; a viewer without access caches nothing and the client - * fetch reaches the route for the real 403. + * That read carries no authorization of its own, so the viewer is proved first. This reuses the + * layout's `cache`d host-context lookup rather than re-deriving the permission, so it costs no + * additional queries; a viewer without access caches nothing and the client fetch reaches the + * route for the real 403. */ export async function prefetchFilesBrowser( queryClient: QueryClient, @@ -38,11 +38,6 @@ export async function prefetchFilesBrowser( if (!hostContext) return await Promise.all([ - queryClient.prefetchQuery({ - queryKey: workspaceFilesKeys.list(workspaceId, 'active'), - queryFn: () => listWorkspaceFilesWithShares(workspaceId, 'active'), - staleTime: WORKSPACE_FILES_LIST_STALE_TIME, - }), queryClient.prefetchQuery({ queryKey: workspaceFileFolderKeys.list(workspaceId, 'active'), queryFn: () => listWorkspaceFileFolders(workspaceId, { scope: 'active' }), diff --git a/apps/sim/app/workspace/[workspaceId]/lib/prefetch.test.ts b/apps/sim/app/workspace/[workspaceId]/lib/prefetch.test.ts index 7d701d22066..4bb0510df1d 100644 --- a/apps/sim/app/workspace/[workspaceId]/lib/prefetch.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/lib/prefetch.test.ts @@ -92,23 +92,35 @@ describe('workspace list prefetches', () => { }) describe('prefetchFilesBrowser', () => { - it('primes both file + folder keys the client hooks read', async () => { - const files = [{ id: 'f-1' }] + it('primes the folder key the client hook reads', async () => { const folders = [{ id: 'folder-1' }] - mockListWorkspaceFilesWithShares.mockResolvedValue(files) mockListWorkspaceFileFolders.mockResolvedValue(folders) const client = makeClient() await prefetchFilesBrowser(client, WORKSPACE_ID, USER_ID) - expect(mockListWorkspaceFilesWithShares).toHaveBeenCalledWith(WORKSPACE_ID, 'active') expect(mockListWorkspaceFileFolders).toHaveBeenCalledWith(WORKSPACE_ID, { scope: 'active' }) - expect(client.getQueryData(workspaceFilesKeys.list(WORKSPACE_ID, 'active'))).toEqual(files) expect(client.getQueryData(workspaceFileFolderKeys.list(WORKSPACE_ID, 'active'))).toEqual( folders ) }) + /** + * The FILE LIST is deliberately not primed here — `prefetchWorkspaceSidebar` owns it, because the + * sidebar reads that query on every workspace route and therefore registers it before any page + * renders. `HydrationBoundary` hands an already-seen query to a `useEffect`, which SSR never runs, + * so a page-level prefetch of this key costs a request per render and still cannot reach the server + * render. Restoring it here would reintroduce exactly that. + */ + it('leaves the file list to the layout rather than re-reading it per page', async () => { + const client = makeClient() + + await prefetchFilesBrowser(client, WORKSPACE_ID, USER_ID) + + expect(mockListWorkspaceFilesWithShares).not.toHaveBeenCalled() + expect(client.getQueryData(workspaceFilesKeys.list(WORKSPACE_ID, 'active'))).toBeUndefined() + }) + /** * The reads bypass the route that used to authorize them, so a viewer without workspace * access must prime nothing and let the client fetch reach the route for the real 403. @@ -120,7 +132,7 @@ describe('workspace list prefetches', () => { await prefetchFilesBrowser(client, WORKSPACE_ID, USER_ID) expect(client.getQueryCache().getAll()).toHaveLength(0) - expect(mockListWorkspaceFilesWithShares).not.toHaveBeenCalled() + expect(mockListWorkspaceFileFolders).not.toHaveBeenCalled() }) }) diff --git a/apps/sim/app/workspace/[workspaceId]/prefetch.ts b/apps/sim/app/workspace/[workspaceId]/prefetch.ts index c15cdbb5bad..a356c96ab5b 100644 --- a/apps/sim/app/workspace/[workspaceId]/prefetch.ts +++ b/apps/sim/app/workspace/[workspaceId]/prefetch.ts @@ -7,6 +7,7 @@ import { isChatEnabled } from '@/lib/core/config/env-flags' import { listFoldersForWorkspace } from '@/lib/folders/queries' import { getUserProfile } from '@/lib/users/queries' import { listWorkflowsForUser } from '@/lib/workflows/queries' +import { listWorkspaceFilesWithShares } from '@/lib/workspace-files/queries' import { getWorkspaceHostContextForViewer } from '@/lib/workspaces/host-context' import { listWorkspacesForViewer } from '@/lib/workspaces/list' import { getWorkspacePermissionsForAuthorizedViewer } from '@/lib/workspaces/permissions/utils' @@ -25,6 +26,10 @@ import { workflowKeys } from '@/hooks/queries/utils/workflow-keys' import { mapWorkflow, WORKFLOW_LIST_STALE_TIME } from '@/hooks/queries/utils/workflow-list-query' import { normalizeWorkspacesResponse } from '@/hooks/queries/utils/workspace-list-query' import { WORKSPACE_PERMISSIONS_STALE_TIME, workspaceKeys } from '@/hooks/queries/workspace' +import { + WORKSPACE_FILES_LIST_STALE_TIME, + workspaceFilesKeys, +} from '@/hooks/queries/workspace-files' import { WORKSPACE_HOST_CONTEXT_STALE_TIME, workspaceHostKeys, @@ -160,6 +165,22 @@ export async function prefetchWorkspaceSidebar( }, staleTime: FOLDER_LIST_STALE_TIME, }), + /** + * The sidebar reads the workspace's files for its search modal, on EVERY workspace route — so this + * query is registered by sidebar chrome before any page renders. That ordering is why it has to be + * prefetched HERE and not only by the Files pages: `HydrationBoundary` hydrates a query the cache + * has already seen from a `useEffect`, which never runs during SSR, so a page-level boundary can + * only ever hand this entry to the client. Seeding it with the layout's own boundary — the first + * one to render — is what lets the server paint the Files browser and the open file's header + * populated instead of shipping a spinner and resolving it a beat later on the client. + * + * Same key + shape as {@link prefetchFilesBrowser}, so whichever runs is a no-op for the other. + */ + queryClient.prefetchQuery({ + queryKey: workspaceFilesKeys.list(workspaceId, 'active'), + queryFn: () => listWorkspaceFilesWithShares(workspaceId, 'active'), + staleTime: WORKSPACE_FILES_LIST_STALE_TIME, + }), queryClient.prefetchQuery({ queryKey: workspaceKeys.permissions(workspaceId), queryFn: () => diff --git a/apps/sim/hooks/queries/workspace-files.test.tsx b/apps/sim/hooks/queries/workspace-files.test.tsx index 64afac2dd03..d6e5a765774 100644 --- a/apps/sim/hooks/queries/workspace-files.test.tsx +++ b/apps/sim/hooks/queries/workspace-files.test.tsx @@ -9,7 +9,7 @@ */ import { act, type ReactNode } from 'react' import { sleep } from '@sim/utils/helpers' -import { QueryClient, QueryClientProvider } from '@tanstack/react-query' +import { focusManager, QueryClient, QueryClientProvider } from '@tanstack/react-query' import { createRoot, type Root } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { createWorkspaceFileContract } from '@/lib/api/contracts/workspace-files' @@ -44,7 +44,8 @@ afterEach(() => { function renderContentHook(options?: { refetchInterval?: number | false | (() => number | false) -}): { unmount: () => void } { + refetchOnWindowFocus?: boolean +}): { queryClient: QueryClient; unmount: () => void } { const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }) const container = document.createElement('div') const root: Root = createRoot(container) @@ -66,6 +67,7 @@ function renderContentHook(options?: { ) }) return { + queryClient, unmount: () => { act(() => root.unmount()) queryClient.clear() @@ -149,6 +151,97 @@ describe('useWorkspaceFileContent refetchInterval passthrough', () => { }) }) +/** + * A content update rewrites the file under a NEW storage key and deletes the old object, so the key + * held by an open tab goes dead — every few seconds while a collaborative document is being edited, + * since the relay persists it server-side. These pin the two halves of the answer: don't create the + * staleness where a server-side owner holds durability, and recover from it everywhere else. + */ +describe('useWorkspaceFileContent stale storage key', () => { + async function focusWindow() { + await act(async () => { + focusManager.setFocused(false) + focusManager.setFocused(true) + await sleep(50) + }) + } + + afterEach(() => { + focusManager.setFocused(undefined) + }) + + it('re-reads on focus by default, so an out-of-band edit reaches an open tab', async () => { + const { unmount } = renderContentHook() + await act(async () => { + await sleep(50) + }) + expect(fetchCount).toBe(1) + + await focusWindow() + + expect(fetchCount).toBe(2) + unmount() + }) + + it('does not re-read on focus when the collaborative relay owns durability', async () => { + const { unmount } = renderContentHook({ refetchOnWindowFocus: false }) + await act(async () => { + await sleep(50) + }) + expect(fetchCount).toBe(1) + + await focusWindow() + + expect(fetchCount).toBe(1) + unmount() + }) + + it('re-resolves the file record when the key it read has been superseded', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => { + fetchCount += 1 + return new Response('{"error":"FileNotFoundError"}', { status: 404 }) + }) + ) + const { queryClient, unmount } = renderContentHook() + const invalidateQueries = vi.spyOn(queryClient, 'invalidateQueries') + + await act(async () => { + await sleep(50) + }) + + expect(fetchCount).toBe(1) + expect(invalidateQueries).toHaveBeenCalledWith({ + queryKey: workspaceFilesKeys.workspaceLists('ws-1'), + }) + // The RECORD is re-resolved, never this query — re-driving the read against the same dead key + // is what would spin. + expect(invalidateQueries).toHaveBeenCalledTimes(1) + unmount() + }) + + it('leaves the record alone when the read fails for any other reason', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => { + fetchCount += 1 + return new Response('nope', { status: 500 }) + }) + ) + const { queryClient, unmount } = renderContentHook() + const invalidateQueries = vi.spyOn(queryClient, 'invalidateQueries') + + await act(async () => { + await sleep(50) + }) + + expect(fetchCount).toBe(1) + expect(invalidateQueries).not.toHaveBeenCalled() + unmount() + }) +}) + describe('useCreateWorkspaceFile', () => { it('uses the create contract and reconciles workspace file caches', async () => { const response = { success: true, file: { id: 'wf-created' } } diff --git a/apps/sim/hooks/queries/workspace-files.ts b/apps/sim/hooks/queries/workspace-files.ts index a6eaaf4ebe7..cc3092b5e70 100644 --- a/apps/sim/hooks/queries/workspace-files.ts +++ b/apps/sim/hooks/queries/workspace-files.ts @@ -1,4 +1,4 @@ -import { useMemo } from 'react' +import { useCallback, useMemo } from 'react' import { toast } from '@sim/emcn' import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' @@ -202,6 +202,49 @@ export function useWorkspaceImageDimensionsAdapter( }, [files, queryClient, workspaceId]) } +/** + * A read that addressed a storage object the file no longer points at. + * + * A workspace file's bytes are rewritten under a NEW storage key on every content update and the + * superseded object is deleted (`updateWorkspaceFileContent`), so a 404 from a content read means + * "the key you are holding has been replaced", not "the server is broken" — the serve route says as + * much and logs it at `info`. Distinguished from a transport failure so {@link useStaleKeyRecovery} + * can re-resolve the record instead of surfacing a dead end. + */ +class StaleStorageKeyError extends Error { + constructor() { + super('File content is no longer at the requested storage key') + this.name = 'StaleStorageKeyError' + } +} + +/** + * Re-resolve a workspace's file records after a read found its storage key superseded. + * + * The key a content read addresses comes from the cached file list, and nothing invalidates that + * list when a write rotates the key — the collaborative relay projects an open document back to + * markdown every few seconds, entirely server-side, so an open tab's key can be replaced many times + * over without a single client-visible event. Recovering on the 404 makes the rotation cost exactly + * one request instead of stranding the reader on a dead key until a full reload. + * + * Invalidates the RECORD, never the failed query: the refetched list either hands back a new key — + * which re-keys the read onto a fresh cache entry that fetches once — or the same one, in which case + * nothing refetches and the failure stands. That asymmetry is what makes this loop-proof, and it is + * why a genuinely deleted file settles instead of retrying: the list simply stops containing it. + */ +function useStaleKeyRecovery(workspaceId: string): (error: unknown) => void { + const queryClient = useQueryClient() + return useCallback( + (error: unknown) => { + if (!workspaceId || !(error instanceof StaleStorageKeyError)) return + void queryClient.invalidateQueries({ + queryKey: workspaceFilesKeys.workspaceLists(workspaceId), + }) + }, + [queryClient, workspaceId] + ) +} + /** * Fetch file content as text via a content-source URL */ @@ -209,6 +252,7 @@ async function fetchWorkspaceFileContent(url: string, signal?: AbortSignal): Pro // boundary-raw-fetch: binary/text download, response is not JSON const response = await fetch(url, { signal, cache: 'no-store' }) + if (response.status === 404) throw new StaleStorageKeyError() if (!response.ok) { throw new Error('Failed to fetch file content') } @@ -227,22 +271,38 @@ async function fetchWorkspaceFileContent(url: string, signal?: AbortSignal): Pro * its single refetch raced the agent's write. The function form is re-evaluated by react-query * after every fetch and options pass, so a condition read through a ref stops the polling as soon * as it flips — no re-render required. + * + * `refetchOnWindowFocus` is how an out-of-band edit reaches a tab that was left open, so it defaults + * on. Pass `false` where a server-side owner holds the file's durability — the collaborative relay + * projects the live document to markdown itself, and delivers external writes into that document as + * CRDT merges — because there the durable bytes are strictly behind what the editor already shows, + * and re-reading them only chases a storage key the relay's last save already replaced. */ export function useWorkspaceFileContent( workspaceId: string, fileId: string, key: string, raw?: boolean, - options?: { refetchInterval?: number | false | (() => number | false) } + options?: { + refetchInterval?: number | false | (() => number | false) + refetchOnWindowFocus?: boolean + } ) { const source = useFileContentSource() + const recoverStaleKey = useStaleKeyRecovery(workspaceId) return useQuery({ queryKey: workspaceFilesKeys.content(workspaceId, fileId, raw ? 'raw' : 'text', key), - queryFn: ({ signal }) => - fetchWorkspaceFileContent(source.buildUrl(key, { raw, bust: true }), signal), + queryFn: async ({ signal }) => { + try { + return await fetchWorkspaceFileContent(source.buildUrl(key, { raw, bust: true }), signal) + } catch (error) { + recoverStaleKey(error) + throw error + } + }, enabled: !!workspaceId && !!fileId && !!key, staleTime: WORKSPACE_FILE_CONTENT_STALE_TIME, - refetchOnWindowFocus: 'always', + refetchOnWindowFocus: options?.refetchOnWindowFocus === false ? false : 'always', refetchInterval: options?.refetchInterval ?? false, }) } @@ -280,6 +340,7 @@ async function fetchWorkspaceFileBinary( // boundary-raw-fetch: binary download consumed as ArrayBuffer const response = await fetch(url, init) if (response.status === 409) throw new DocNotReadyError() + if (response.status === 404) throw new StaleStorageKeyError() if (!response.ok) throw new Error('Failed to fetch file content') return response.arrayBuffer() } @@ -303,17 +364,24 @@ export function useWorkspaceFileBinary( options?: { enabled?: boolean; version?: string | number } ) { const source = useFileContentSource() + const recoverStaleKey = useStaleKeyRecovery(workspaceId) return useQuery({ queryKey: options?.version != null ? [...workspaceFilesKeys.content(workspaceId, fileId, 'binary', key), options.version] : workspaceFilesKeys.content(workspaceId, fileId, 'binary', key), - queryFn: ({ signal }) => - fetchWorkspaceFileBinary( - source.buildUrl(key, { version: options?.version, bust: true }), - options?.version, - signal - ), + queryFn: async ({ signal }) => { + try { + return await fetchWorkspaceFileBinary( + source.buildUrl(key, { version: options?.version, bust: true }), + options?.version, + signal + ) + } catch (error) { + recoverStaleKey(error) + throw error + } + }, // Callers gate this on a readiness signal (e.g. the file has committed // content) so we don't 409-poll the serve route for a generated doc whose // compiled artifact hasn't been written yet — the doc is fetched once, when diff --git a/apps/sim/lib/collab-doc/converter.test.ts b/apps/sim/lib/collab-doc/converter.test.ts index ba5606600b6..f31796d2a82 100644 --- a/apps/sim/lib/collab-doc/converter.test.ts +++ b/apps/sim/lib/collab-doc/converter.test.ts @@ -2,19 +2,31 @@ * @vitest-environment jsdom */ import { FILE_DOC_SEED } from '@sim/realtime-protocol/file-doc' -import { describe, expect, it } from 'vitest' +import { Editor, getSchema, type JSONContent } from '@tiptap/core' +import { prosemirrorJSONToYDoc, yDocToProsemirrorJSON } from '@tiptap/y-tiptap' +import { beforeAll, describe, expect, it } from 'vitest' +import { Awareness } from 'y-protocols/awareness' import * as Y from 'yjs' +import { createMarkdownEditorExtensions } from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/editor-extensions' +import { createMarkdownContentExtensions } from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/extensions' import { applyFrontmatter, postProcessSerializedMarkdown, + splitFrontmatter, } from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity' -import { serializeMarkdownBody } from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-parse' +import { + editorNormalForm, + parseMarkdownToDoc, + serializeMarkdownBody, +} from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-parse' import { applyMarkdownToYDoc, + canonicalizeYDoc, markdownToYDoc, yDocToFileMarkdown, yDocToMarkdown, } from './converter' +import { COLLAB_DOC_FIELD } from './field' /** Representative markdown covering the custom-fidelity constructs (tables, code, lists, marks). */ const SAMPLES = [ @@ -30,6 +42,13 @@ const SAMPLES = [ '- [ ] todo\n- [x] done', ] +beforeAll(() => { + if (!document.elementFromPoint) document.elementFromPoint = () => null +}) + +/** The schema the collab converter builds its docs on — mirrors `markdownSchema()` in converter.ts. */ +const markdownSchemaForTest = () => getSchema(createMarkdownContentExtensions()) + describe('collab-doc converter', () => { it('round-trips markdown through the Yjs doc identically to the client engine', () => { for (const md of SAMPLES) { @@ -43,6 +62,197 @@ describe('collab-doc converter', () => { expect(yDocToMarkdown(markdownToYDoc(''))).toBe(serializeMarkdownBody('')) }) + /** + * The Files editor paints a read-only placeholder built from the file's markdown, then swaps in the + * live collaborative doc once the CRDT syncs. Anything the CRDT holds that the markdown projection + * cannot round-trip shows up as the document reflowing its spacing a beat after the file appears. + * + * The case that matters is a state no markdown parse produced: the user presses Enter on an empty + * line, which puts a real empty paragraph in the CRDT. Projecting that to markdown and re-parsing it + * has to give the same blocks back — otherwise the blank line is both invisible on first paint and + * deleted for good once the room goes cold. + */ + describe('placeholder ⇄ live CRDT parity', () => { + const shapeOf = (blocks: JSONContent[] | undefined) => + (blocks ?? []) + .map((n) => (n.type === 'paragraph' && !n.content?.length ? '∅' : n.type)) + .join(',') + + /** + * The doc a peer renders (the CRDT) vs the doc the placeholder builds from the projected markdown. + * The placeholder is a real editor, so it is compared through {@link editorNormalForm} — the same + * normalization ProseMirror applies on mount. Comparing the bare parse instead would assert a shape + * neither side ever renders, and would let the CRDT drift back out of the editor's normal form. + */ + const parity = (live: Y.Doc) => ({ + crdt: shapeOf(yDocToProsemirrorJSON(live, COLLAB_DOC_FIELD).content), + placeholder: shapeOf(editorNormalForm(yDocToMarkdown(live)).content), + }) + + const paragraphs = (count: number) => + Array.from({ length: count }, () => new Y.XmlElement('paragraph')) + + /** Seed a doc from markdown, then apply an edit no markdown parse could have produced. */ + const typedInto = (md: string, edit: (fragment: Y.XmlFragment) => void) => { + const live = markdownToYDoc(md) + edit(live.getXmlFragment(COLLAB_DOC_FIELD)) + return live + } + + it('a blank line typed between two paragraphs round-trips as-is', () => { + const live = typedInto('a\n\nb', (f) => f.insert(1, paragraphs(2))) + const { crdt, placeholder } = parity(live) + expect(crdt).toBe('paragraph,∅,∅,paragraph') + expect(placeholder).toBe(crdt) + live.destroy() + }) + + /** + * Every CRDT state below is one markdown genuinely cannot describe, so the round-trip is NOT the + * identity on it — a run past the parse bound, trailing empties the serializer collapses, an empty + * paragraph in a document that must parse whole. Each one used to reach a room verbatim and reflow + * the doc a beat after it painted. `canonicalizeYDoc` is the single pass that resolves all of them, + * so assert the invariant it establishes rather than enumerating what markdown can hold. + */ + it.each([ + [ + 'a run past the per-gap bound', + () => typedInto('a\n\nb', (f) => f.insert(1, paragraphs(30))), + ], + ['trailing empties', () => typedInto('a\n\nb', (f) => f.insert(f.length, paragraphs(3)))], + ['leading empties', () => typedInto('a\n\nb', (f) => f.insert(0, paragraphs(2)))], + ['between two lists', () => typedInto('- a\n\n- b', (f) => f.insert(1, paragraphs(1)))], + ['between two quotes', () => typedInto('> a\n\n> b', (f) => f.insert(1, paragraphs(1)))], + [ + 'inside a raw-HTML document', + () => typedInto('# H\n\nbody\n\n
x
', (f) => f.insert(1, paragraphs(1))), + ], + [ + 'inside a reference-definition document', + () => + typedInto('# H\n\nsee [y][r]\n\n[r]: https://e.com', (f) => f.insert(1, paragraphs(1))), + ], + ])('canonicalizing restores parity: %s', (_label, build) => { + const live = build() + canonicalizeYDoc(live) + const { crdt, placeholder } = parity(live) + expect(placeholder).toBe(crdt) + // Idempotent: a canonical doc is already its own markdown projection, so a second pass is a no-op. + expect(canonicalizeYDoc(live)).toBe(false) + live.destroy() + }) + + /** + * The point of canonical is that the CRDT and the durable bytes describe the same document, so the + * invariant is stated against the bytes that get WRITTEN — `yDocToFileMarkdown`, post-process and + * all. Converging on the bare serializer output instead would leave that pass's fidelity fixes + * (empty list markers, escaped callout markers) outside the fixed point. + */ + it.each([ + ['a callout the serializer escapes', '# T\n\n> [!NOTE]\n> body\n\ntail'], + ['a list with an empty item', '# T\n\n- parent\n - \n- after'], + ['trailing blank run', '# T\n\nbody\n\n\n\n'], + ['ends on a list', '# T\n\nintro\n\n- a\n- b'], + ])('a canonical doc projects to exactly the file body: %s', (_label, md) => { + const live = markdownToYDoc(md) + canonicalizeYDoc(live) + + const body = splitFrontmatter(yDocToFileMarkdown(live)).body + // Re-reading the written bytes must rebuild the very doc the CRDT holds. + expect(shapeOf(editorNormalForm(body).content)).toBe( + shapeOf(yDocToProsemirrorJSON(live, COLLAB_DOC_FIELD).content) + ) + // And a second pass has nothing left to do. + expect(canonicalizeYDoc(live)).toBe(false) + live.destroy() + }) + + it('holds for every representative document', () => { + for (const md of [ + ...SAMPLES, + 'a\n\n\n\nb', + '# Heading\n\n\n\nbody\n\n\n\n\n\ntail', + '\n\n\n\nleading blank lines', + '- a\n- b\n\n\n\nafter the list', + '- a\n\n\n\n- b', + '> a\n\n\n\n> b', + ]) { + const live = markdownToYDoc(md) + const { crdt, placeholder } = parity(live) + expect(placeholder, `diverged for ${JSON.stringify(md)}`).toBe(crdt) + live.destroy() + } + }) + + /** + * `canonicalizeYDoc`'s return value gates whether both callers re-encode, so it has to be true + * whenever the doc actually moved. Deciding that from the markdown projection could not work: the + * repair is the trailing paragraph, which serializes to a blank line the post-process collapses, so + * every doc ending on a list/heading/table/rule was repaired and still reported unchanged — and the + * cached snapshot kept the unrepaired bytes, reopening the stacking-empties path. + */ + it.each([ + ['ends with a list', '# T\n\nbody\n\n- a\n- b'], + ['ends with a heading', '# T\n\nbody\n\n## Tail'], + ['ends with a table', '# T\n\n| a | b |\n| --- | --- |\n| 1 | 2 |'], + ['ends with a rule', '# T\n\nbody\n\n---'], + ])('reports the repair it actually made: %s', (_label, md) => { + // A doc built from the RAW parse — what a snapshot cached before this normalization looks like. + const doc = prosemirrorJSONToYDoc( + markdownSchemaForTest(), + parseMarkdownToDoc(md), + COLLAB_DOC_FIELD + ) + const before = shapeOf(yDocToProsemirrorJSON(doc, COLLAB_DOC_FIELD).content) + + const reported = canonicalizeYDoc(doc) + + const after = shapeOf(yDocToProsemirrorJSON(doc, COLLAB_DOC_FIELD).content) + expect(after).toBe(`${before},∅`) + expect(reported).toBe(true) + doc.destroy() + }) + + /** + * Opening a document must not CHANGE it. ProseMirror appends an empty paragraph to any doc that + * does not end in one, so a seed ending on a list, heading, table, or rule used to be rewritten by + * the first client that bound to it — into the SHARED doc, where a trailing blank line cannot + * serialize, so the file never recorded it and nothing ever reconciled the two. Every client that + * seeded without seeing another's contribution stacked one more; a real document accumulated 18 + * against the placeholder's 1, and the pane jumped by that much the moment the live editor took + * over. Seeding in the editor's own normal form is what makes the bind a no-op. + */ + it.each([ + ['ends with a list', '# T\n\nbody\n\n- a\n- b'], + ['ends with a heading', '# T\n\nbody\n\n## Tail'], + ['ends with a table', '# T\n\n| a | b |\n| --- | --- |\n| 1 | 2 |'], + ['ends with a rule', '# T\n\nbody\n\n---'], + ['ends with a paragraph', '# T\n\nbody'], + ['ends with a blank line', '# T\n\nbody\n\n\n\n'], + ])('binding an editor to the seed changes nothing: %s', (_label, md) => { + const doc = markdownToYDoc(md) + const before = doc.getXmlFragment(COLLAB_DOC_FIELD).length + const awareness = new Awareness(doc) + const editor = new Editor({ + extensions: createMarkdownEditorExtensions({ + placeholder: '', + embeds: true, + collaboration: { + doc, + awareness, + user: { name: 'U', color: '#fff', clientId: doc.clientID }, + }, + }), + }) + + expect(doc.getXmlFragment(COLLAB_DOC_FIELD).length).toBe(before) + + editor.destroy() + awareness.destroy() + doc.destroy() + }) + }) + it('applies new content into an existing doc (agent write)', () => { const ydoc = markdownToYDoc('# Hello\n\nWorld.') applyMarkdownToYDoc(ydoc, '# Hello\n\nWorld and then some more.') diff --git a/apps/sim/lib/collab-doc/converter.ts b/apps/sim/lib/collab-doc/converter.ts index 65ceb88cade..f90c6305c1a 100644 --- a/apps/sim/lib/collab-doc/converter.ts +++ b/apps/sim/lib/collab-doc/converter.ts @@ -14,10 +14,10 @@ import { postProcessSerializedMarkdown, } from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity' import { - parseMarkdownToDoc, + editorNormalForm, serializeDocToMarkdown, } from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-parse' -import { COLLAB_DOC_FIELD } from './normalize' +import { COLLAB_DOC_FIELD } from './field' /** * Server-side conversion between a file's markdown and its collaborative Yjs document. @@ -75,8 +75,7 @@ function ensureDomForTipTap(): void { /** Convert a file's markdown to a fresh collaborative {@link Y.Doc} (cold-start seed). */ export function markdownToYDoc(markdown: string): Y.Doc { ensureDomForTipTap() - const json = parseMarkdownToDoc(markdown) - return prosemirrorJSONToYDoc(markdownSchema(), json, COLLAB_DOC_FIELD) + return prosemirrorJSONToYDoc(markdownSchema(), editorNormalForm(markdown), COLLAB_DOC_FIELD) } /** Project a collaborative {@link Y.Doc}'s BODY back to markdown (no frontmatter). */ @@ -102,6 +101,49 @@ export function yDocToFileMarkdown(ydoc: Y.Doc): string { ) } +/** + * Converge a collaborative {@link Y.Doc} onto its own markdown projection — the document's CANONICAL + * form — and report whether anything changed. + * + * A ProseMirror document is strictly richer than markdown, so `parse ∘ serialize` is not the identity: + * trailing empty paragraphs are collapsed by `postProcessSerializedMarkdown`, a blank run past the parse + * bound is truncated, and a document that must parse whole (raw HTML, reference definitions) keeps no + * empty paragraphs at all. A CRDT holding any such state describes a document its own markdown cannot + * reproduce — so the file renders one way from the live doc and another from the durable bytes, and the + * difference surfaces as the editor reflowing a beat after it paints, then silently discarding the + * spacing once the room goes cold. + * + * The fix is to keep the CRDT inside the image of the parse. Defining canonical as "what the round-trip + * produces" rather than as a hand-written list of what markdown cannot hold is what makes this + * self-maintaining: every future gap between the two representations is absorbed here automatically, + * with no second place to update. Idempotent by construction — a canonical doc projects to markdown that + * parses back to itself, so a second call is a no-op — and it applies the difference through + * {@link applyMarkdownToYDoc}, so it is a minimal CRDT diff rather than a replacement. + * + * Call this on a DETACHED doc (a decoded snapshot), never on a live room: it is a correctness pass for + * durable artifacts, and converging a document somebody is typing into would move their caret. + * + * "Changed" is decided on the DOCUMENT, not on its markdown. Comparing projections looks equivalent and + * is not: the repairs this pass exists to make are precisely the ones markdown cannot express, so a + * markdown-equality check is blind to them. Concretely, appending the trailing paragraph + * {@link editorNormalForm} requires serializes to a trailing blank line that + * `postProcessSerializedMarkdown` collapses — so every doc ending on a list, heading, table, or rule was + * repaired here and still reported unchanged, and both callers key their re-encode off that flag. The + * cached snapshot then kept the UNREPAIRED bytes, which is the one path back into the stacking-empties + * bug this pass was written to close. + */ +export function canonicalizeYDoc(ydoc: Y.Doc): boolean { + ensureDomForTipTap() + const before = yDocToProsemirrorJSON(ydoc, COLLAB_DOC_FIELD) + // Converge on the body that will actually be WRITTEN, post-process included — the same pass + // `yDocToFileMarkdown` applies. Targeting the bare serializer output would define canonical against a + // string the file never contains, so the fidelity fixes that pass makes (empty list markers that + // re-parse wrong, backslash-escaped callout markers) would sit outside the fixed point this exists to + // establish, and the live doc could settle on a shape the durable bytes do not reproduce. + applyMarkdownToYDoc(ydoc, postProcessSerializedMarkdown(serializeDocToMarkdown(before))) + return JSON.stringify(yDocToProsemirrorJSON(ydoc, COLLAB_DOC_FIELD)) !== JSON.stringify(before) +} + /** * Apply new markdown content into an EXISTING collaborative {@link Y.Doc} as a minimal CRDT diff, * merging with any concurrent user edits rather than replacing the document. This is how the agent @@ -112,7 +154,7 @@ export function yDocToFileMarkdown(ydoc: Y.Doc): string { export function applyMarkdownToYDoc(ydoc: Y.Doc, markdown: string): void { ensureDomForTipTap() const schema = markdownSchema() - const target = ProseMirrorNode.fromJSON(schema, parseMarkdownToDoc(markdown)) + const target = ProseMirrorNode.fromJSON(schema, editorNormalForm(markdown)) const fragment = ydoc.getXmlFragment(COLLAB_DOC_FIELD) // `updateYFragment` diffs against the fragment's CURRENT content, so it needs the fragment↔PM // binding metadata (the element/mark mapping the live editor's ySyncPlugin normally maintains). diff --git a/apps/sim/lib/collab-doc/field.ts b/apps/sim/lib/collab-doc/field.ts new file mode 100644 index 00000000000..b67e7229f10 --- /dev/null +++ b/apps/sim/lib/collab-doc/field.ts @@ -0,0 +1,8 @@ +/** + * The Yjs `XmlFragment` name TipTap's Collaboration extension binds to (its default `field`). The + * client configures `Collaboration.configure({ document })` with no explicit `field`, so it uses + * TipTap's default, `'default'`. Server-side conversion, seeding, and persistence MUST target the same + * fragment or the client would sync an empty document — so this is the single canonical source consumed + * by both bundles (it has no imports, making it safe from client and server alike). + */ +export const COLLAB_DOC_FIELD = 'default' diff --git a/apps/sim/lib/collab-doc/normalize.test.ts b/apps/sim/lib/collab-doc/normalize.test.ts deleted file mode 100644 index f57bad15c72..00000000000 --- a/apps/sim/lib/collab-doc/normalize.test.ts +++ /dev/null @@ -1,127 +0,0 @@ -/** - * @vitest-environment node - */ -import { describe, expect, it } from 'vitest' -import * as Y from 'yjs' -import { COLLAB_DOC_FIELD, stripEmptyTopLevelParagraphs } from './normalize' - -/** Build a top-level element with the given tag and optional text content. */ -function element(tag: string, text?: string): Y.XmlElement { - const el = new Y.XmlElement(tag) - if (text !== undefined) el.insert(0, [new Y.XmlText(text)]) - return el -} - -/** Recursively concatenate the visible text of a Yjs XML node. */ -function textOf(node: Y.XmlElement | Y.XmlText | Y.XmlHook): string { - if (node instanceof Y.XmlText) return node.toString() - if (node instanceof Y.XmlElement) { - let text = '' - for (let i = 0; i < node.length; i++) text += textOf(node.get(i)) - return text - } - return '' -} - -/** The ordered list of top-level `[tag, text]` pairs currently in a doc's body fragment. */ -function structure(doc: Y.Doc): Array<[string, string]> { - const fragment = doc.getXmlFragment(COLLAB_DOC_FIELD) - const out: Array<[string, string]> = [] - for (let i = 0; i < fragment.length; i++) { - const node = fragment.get(i) - out.push([node instanceof Y.XmlElement ? node.nodeName! : 'text', textOf(node)]) - } - return out -} - -describe('stripEmptyTopLevelParagraphs', () => { - it('removes interior empty paragraphs while preserving content and order (production repro)', () => { - // Mirrors the persisted snapshot for random_data.md: a description paragraph, TWO consecutive empty - // paragraphs (the reported "two spaces"), then a bullet list, then another interior empty paragraph. - const doc = new Y.Doc() - const fragment = doc.getXmlFragment(COLLAB_DOC_FIELD) - fragment.insert(0, [ - element('paragraph', 'A small collection of sample data.'), - element('paragraph'), - element('paragraph'), - element('bulletList', 'list'), - element('paragraph'), - element('paragraph', 'trailing content'), - ]) - - expect(stripEmptyTopLevelParagraphs(doc)).toBe(true) - expect(structure(doc)).toEqual([ - ['paragraph', 'A small collection of sample data.'], - ['bulletList', 'list'], - ['paragraph', 'trailing content'], - ]) - doc.destroy() - }) - - it('is idempotent — a second pass finds nothing to remove', () => { - const doc = new Y.Doc() - doc - .getXmlFragment(COLLAB_DOC_FIELD) - .insert(0, [element('paragraph'), element('paragraph', 'body')]) - - expect(stripEmptyTopLevelParagraphs(doc)).toBe(true) - expect(stripEmptyTopLevelParagraphs(doc)).toBe(false) - expect(structure(doc)).toEqual([['paragraph', 'body']]) - doc.destroy() - }) - - it('returns false and mutates nothing when there are no top-level empty paragraphs', () => { - const doc = new Y.Doc() - doc - .getXmlFragment(COLLAB_DOC_FIELD) - .insert(0, [element('heading', 'Title'), element('paragraph', 'body')]) - - expect(stripEmptyTopLevelParagraphs(doc)).toBe(false) - expect(structure(doc)).toEqual([ - ['heading', 'Title'], - ['paragraph', 'body'], - ]) - doc.destroy() - }) - - it('leaves an empty paragraph nested inside another block untouched (only top-level is stripped)', () => { - const doc = new Y.Doc() - const listItem = new Y.XmlElement('listItem') - listItem.insert(0, [new Y.XmlElement('paragraph')]) // an empty paragraph BELOW the fragment root - const list = new Y.XmlElement('bulletList') - list.insert(0, [listItem]) - doc.getXmlFragment(COLLAB_DOC_FIELD).insert(0, [list]) - - expect(stripEmptyTopLevelParagraphs(doc)).toBe(false) - const nestedList = doc.getXmlFragment(COLLAB_DOC_FIELD).get(0) as Y.XmlElement - const nestedItem = nestedList.get(0) as Y.XmlElement - expect(nestedItem.get(0)).toBeInstanceOf(Y.XmlElement) - expect((nestedItem.get(0) as Y.XmlElement).nodeName).toBe('paragraph') - doc.destroy() - }) - - it('survives an encode/decode round-trip preserving CRDT ids and the config map (seed-repair path)', () => { - const original = new Y.Doc() - original - .getXmlFragment(COLLAB_DOC_FIELD) - .insert(0, [element('paragraph', 'kept'), element('paragraph')]) - original.getMap('config').set('initialContentLoaded', true) - original.getMap('config').set('frontmatter', 'title: x') - const before = Y.encodeStateAsUpdate(original) - original.destroy() - - // Repair exactly as normalizeSeedUpdate does: apply → strip → re-encode. - const repair = new Y.Doc() - Y.applyUpdate(repair, before) - expect(stripEmptyTopLevelParagraphs(repair)).toBe(true) - const after = Y.encodeStateAsUpdate(repair) - repair.destroy() - - const seeded = new Y.Doc() - Y.applyUpdate(seeded, after) - expect(structure(seeded)).toEqual([['paragraph', 'kept']]) - expect(seeded.getMap('config').get('initialContentLoaded')).toBe(true) - expect(seeded.getMap('config').get('frontmatter')).toBe('title: x') - seeded.destroy() - }) -}) diff --git a/apps/sim/lib/collab-doc/normalize.ts b/apps/sim/lib/collab-doc/normalize.ts deleted file mode 100644 index a82075f79be..00000000000 --- a/apps/sim/lib/collab-doc/normalize.ts +++ /dev/null @@ -1,43 +0,0 @@ -import * as Y from 'yjs' - -/** - * The Yjs `XmlFragment` name TipTap's Collaboration extension binds to (its default `field`). The - * client configures `Collaboration.configure({ document })` with no explicit `field`, so it uses - * TipTap's default, `'default'`. Server-side conversion, seeding, and persistence MUST target the same - * fragment or the client would sync an empty document — so this is the single canonical source consumed - * by both bundles (it imports only `yjs`, making it safe from client and server alike). - */ -export const COLLAB_DOC_FIELD = 'default' - -/** - * Remove every top-level empty paragraph (a `paragraph` element with no children) from a collaborative - * document's body fragment, returning whether it deleted any. - * - * The markdown parse pipeline strips these from EVERY parse target (see `stripEmptyParagraphs` in - * `markdown-parse.ts`): in markdown a run of blank lines between blocks is insignificant, so the static - * placeholder, the download, and every standard renderer show no interior blank. A cached Yjs snapshot, - * however, is a raw CRDT binary that bypasses that parse — so it can preserve an empty-paragraph node the - * re-parse would have dropped. When a warm room seeds from such a snapshot, the empty paragraph surfaces - * as a stray blank line appearing once the doc settles, diverging from the placeholder that was shown - * first. Enforcing the same no-top-level-empty-paragraph invariant on the Yjs side keeps the live - * collaborative doc rendering identically to the markdown re-parse. - * - * Idempotent, and only TOP-LEVEL paragraphs are touched — blank lines that carry meaning inside a - * construct (e.g. a loose list) live below the fragment root and are left alone. Runs its own Yjs - * transaction so the deletions commit atomically, iterating the fragment back-to-front so a deletion - * never shifts a not-yet-checked index. - */ -export function stripEmptyTopLevelParagraphs(doc: Y.Doc): boolean { - const fragment = doc.getXmlFragment(COLLAB_DOC_FIELD) - let removed = false - doc.transact(() => { - for (let i = fragment.length - 1; i >= 0; i--) { - const node = fragment.get(i) - if (node instanceof Y.XmlElement && node.nodeName === 'paragraph' && node.length === 0) { - fragment.delete(i, 1) - removed = true - } - } - }) - return removed -} diff --git a/apps/sim/lib/collab-doc/persist.test.ts b/apps/sim/lib/collab-doc/persist.test.ts new file mode 100644 index 00000000000..38cbe9ae6a8 --- /dev/null +++ b/apps/sim/lib/collab-doc/persist.test.ts @@ -0,0 +1,170 @@ +/** + * @vitest-environment jsdom + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import * as Y from 'yjs' + +const { mockGetWorkspaceFile, mockFetchBuffer, mockUpdateContent, mockSaveState } = vi.hoisted( + () => ({ + mockGetWorkspaceFile: vi.fn(), + mockFetchBuffer: vi.fn(), + mockUpdateContent: vi.fn(), + mockSaveState: vi.fn(), + }) +) + +vi.mock('@/lib/uploads/contexts/workspace', () => ({ + ContentVersionConflictError: class ContentVersionConflictError extends Error {}, + getWorkspaceFile: mockGetWorkspaceFile, + fetchWorkspaceFileBuffer: mockFetchBuffer, + updateWorkspaceFileContent: mockUpdateContent, +})) + +vi.mock('./collab-state', () => ({ + hashMarkdown: () => 'test-source-hash', + saveCollabDocState: mockSaveState, +})) + +import { markdownToYDoc, yDocToFileMarkdown } from './converter' +import { persistFileDoc } from './persist' + +const VERSION = new Date('2026-01-01T00:00:00.000Z') + +/** The exact bytes `persistFileDoc` would project from a doc seeded with `md`. */ +function projectionOf(md: string): Buffer { + const doc = markdownToYDoc(md) + try { + return Buffer.from(yDocToFileMarkdown(doc), 'utf-8') + } finally { + doc.destroy() + } +} + +function stateOf(md: string): Uint8Array { + const doc = markdownToYDoc(md) + try { + return Y.encodeStateAsUpdate(doc) + } finally { + doc.destroy() + } +} + +describe('persistFileDoc — no-op writes', () => { + beforeEach(() => { + vi.clearAllMocks() + mockSaveState.mockResolvedValue(undefined) + }) + + function stubFile(durable: Buffer) { + mockGetWorkspaceFile.mockResolvedValue({ + id: 'file-1', + name: 'note.md', + key: 'k', + size: durable.length, + updatedAt: VERSION, + contentUpdatedAt: VERSION, + }) + mockFetchBuffer.mockResolvedValue(durable) + } + + /** + * Opening a file emits a Yjs update of its own (y-tiptap normalizes node attributes on bind), which + * schedules a persist whose markdown is byte-identical to the file. Writing it would rewrite the file + * under a fresh storage key and delete the old object, 404ing every reader still holding it — the + * page's own first content read included. + */ + it('writes nothing when the projection already matches the durable bytes', async () => { + const md = '# Title\n\nbody\n\n- [ ] task' + stubFile(projectionOf(md)) + + const result = await persistFileDoc('ws-1', 'file-1', 'user-1', stateOf(md), VERSION.getTime()) + + expect(mockUpdateContent).not.toHaveBeenCalled() + expect(result).toEqual({ status: 'persisted', version: VERSION.getTime() }) + }) + + it('reports the CURRENT durable version on a no-op, resyncing a stale If-Match instead of conflicting', async () => { + const md = 'a\n\nb' + stubFile(projectionOf(md)) + + const result = await persistFileDoc( + 'ws-1', + 'file-1', + 'user-1', + stateOf(md), + VERSION.getTime() - 5000 + ) + + expect(mockUpdateContent).not.toHaveBeenCalled() + expect(result).toEqual({ status: 'persisted', version: VERSION.getTime() }) + }) + + it('still refreshes the cached snapshot on a no-op, so a cold open seeds from the canonical binary', async () => { + const md = 'a\n\nb' + stubFile(projectionOf(md)) + + await persistFileDoc('ws-1', 'file-1', 'user-1', stateOf(md), VERSION.getTime()) + + expect(mockSaveState).toHaveBeenCalledWith('file-1', expect.anything(), 'test-source-hash') + }) + + it('writes when the content actually changed', async () => { + stubFile(projectionOf('a\n\nb')) + mockUpdateContent.mockResolvedValue({ + contentUpdatedAt: new Date(VERSION.getTime() + 1000), + updatedAt: new Date(VERSION.getTime() + 1000), + }) + + const result = await persistFileDoc( + 'ws-1', + 'file-1', + 'user-1', + stateOf('a\n\nb\n\nc'), + VERSION.getTime() + ) + + expect(mockUpdateContent).toHaveBeenCalledTimes(1) + expect(result).toEqual({ status: 'persisted', version: VERSION.getTime() + 1000 }) + }) + + it('skips the compare read entirely when the byte count already differs', async () => { + stubFile(Buffer.from('a shorter file', 'utf-8')) + mockUpdateContent.mockResolvedValue({ + contentUpdatedAt: new Date(VERSION.getTime() + 1000), + updatedAt: new Date(VERSION.getTime() + 1000), + }) + + await persistFileDoc( + 'ws-1', + 'file-1', + 'user-1', + stateOf('# A much longer document\n\nbody'), + VERSION.getTime() + ) + + expect(mockFetchBuffer).not.toHaveBeenCalled() + expect(mockUpdateContent).toHaveBeenCalledTimes(1) + }) + + it('falls through to the write when the durable bytes cannot be read', async () => { + const md = 'a\n\nb' + const durable = projectionOf(md) + mockGetWorkspaceFile.mockResolvedValue({ + id: 'file-1', + name: 'note.md', + key: 'k', + size: durable.length, + updatedAt: VERSION, + contentUpdatedAt: VERSION, + }) + mockFetchBuffer.mockRejectedValue(new Error('storage unavailable')) + mockUpdateContent.mockResolvedValue({ + contentUpdatedAt: new Date(VERSION.getTime() + 1000), + updatedAt: new Date(VERSION.getTime() + 1000), + }) + + await persistFileDoc('ws-1', 'file-1', 'user-1', stateOf(md), VERSION.getTime()) + + expect(mockUpdateContent).toHaveBeenCalledTimes(1) + }) +}) diff --git a/apps/sim/lib/collab-doc/persist.ts b/apps/sim/lib/collab-doc/persist.ts index 357e3d20a47..64cb727c1d7 100644 --- a/apps/sim/lib/collab-doc/persist.ts +++ b/apps/sim/lib/collab-doc/persist.ts @@ -3,12 +3,12 @@ import { getErrorMessage } from '@sim/utils/errors' import * as Y from 'yjs' import { ContentVersionConflictError, + fetchWorkspaceFileBuffer, getWorkspaceFile, updateWorkspaceFileContent, } from '@/lib/uploads/contexts/workspace' import { hashMarkdown, saveCollabDocState } from './collab-state' -import { yDocToFileMarkdown } from './converter' -import { stripEmptyTopLevelParagraphs } from './normalize' +import { canonicalizeYDoc, yDocToFileMarkdown } from './converter' const logger = createLogger('FileDocPersist') @@ -69,19 +69,50 @@ export async function persistFileDoc( const ydoc = new Y.Doc() let markdownBuffer: Buffer - // The Yjs snapshot cached below (`saveCollabDocState`) seeds a later cold room open directly, so it - // must never carry structure the markdown re-parse would strip — a top-level empty paragraph left in - // the snapshot resurfaces as a stray blank line when that warm doc settles, diverging from the static - // placeholder. Normalize it out here so the cached binary matches the durable markdown by construction. + // The snapshot cached below seeds a later cold room directly, so it must describe the same document + // the durable markdown does — otherwise a warm open renders structure the markdown cannot reproduce + // and the doc reflows once it settles. `canonicalizeYDoc` converges this DETACHED copy onto its own + // markdown projection, which is exactly that guarantee, and leaves the live room untouched. let cachedDocState = docState try { Y.applyUpdate(ydoc, docState) - if (stripEmptyTopLevelParagraphs(ydoc)) cachedDocState = Y.encodeStateAsUpdate(ydoc) + if (canonicalizeYDoc(ydoc)) cachedDocState = Y.encodeStateAsUpdate(ydoc) markdownBuffer = Buffer.from(yDocToFileMarkdown(ydoc), 'utf-8') } finally { ydoc.destroy() } + // A persist that would write the bytes already on disk is skipped entirely. Binding an editor to a + // seeded document emits a Yjs update of its own — y-tiptap normalizes node attributes on bind — so + // simply OPENING a file schedules a save whose projection is byte-identical to the file. Writing it + // is not free: `updateWorkspaceFileContent` uploads under a FRESH storage key, repoints the row, and + // deletes the old object, so every reader still holding the previous key 404s. That is the stray + // not-found a page sees on open, racing its own first content read. + // + // Length is the free reject — a real edit almost never lands on the same byte count — so the compare + // read happens only when a no-op write is actually on the table. Unchanged content means there is + // nothing to clobber, so this reports the file's CURRENT durable version rather than conflicting on a + // stale `expectedVersion`: it resynchronizes the relay's If-Match token instead of stranding it. + if (record.size === markdownBuffer.length) { + const current = await fetchWorkspaceFileBuffer(record).catch(() => null) + if (current?.equals(markdownBuffer)) { + // Still refresh the cached snapshot: the markdown is unchanged (so its `sourceHash` tag stays + // valid) but the doc state may have just been canonicalized, and a cold open should seed from + // the repaired binary rather than the one that needed repairing. + try { + await saveCollabDocState(fileId, cachedDocState, hashMarkdown(markdownBuffer)) + } catch (error) { + logger.warn(`Failed to cache collab doc state for file ${fileId}`, { + error: getErrorMessage(error), + }) + } + return { + status: 'persisted', + version: (record.contentUpdatedAt ?? record.updatedAt).getTime(), + } + } + } + try { const updated = await updateWorkspaceFileContent( workspaceId, diff --git a/apps/sim/lib/collab-doc/seed.test.ts b/apps/sim/lib/collab-doc/seed.test.ts index 28e8562464a..a690b95fb8c 100644 --- a/apps/sim/lib/collab-doc/seed.test.ts +++ b/apps/sim/lib/collab-doc/seed.test.ts @@ -2,6 +2,8 @@ * @vitest-environment jsdom */ import { FILE_DOC_SEED } from '@sim/realtime-protocol/file-doc' +import { getSchema } from '@tiptap/core' +import { prosemirrorJSONToYDoc } from '@tiptap/y-tiptap' import { beforeEach, describe, expect, it, vi } from 'vitest' import * as Y from 'yjs' @@ -23,8 +25,13 @@ vi.mock('./collab-state', () => ({ loadFreshCollabDocState: mockLoadFresh, })) -import { serializeMarkdownBody } from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-parse' -import { yDocToMarkdown } from './converter' +import { createMarkdownContentExtensions } from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/extensions' +import { + parseMarkdownToDoc, + serializeMarkdownBody, +} from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-parse' +import { markdownToYDoc, yDocToMarkdown } from './converter' +import { COLLAB_DOC_FIELD } from './field' import { buildFileDocSeed } from './seed' describe('buildFileDocSeed', () => { @@ -54,8 +61,10 @@ describe('buildFileDocSeed', () => { it('cold-start fast path: returns the cached binary directly without re-converting when it is fresh', async () => { // A cached binary derived from the current markdown → seed returns it verbatim (no conversion), the - // Hocuspocus load-document path that preserves the CRDT's client ids across reopens. - const cachedDoc = new Y.Doc() + // Hocuspocus load-document path that preserves the CRDT's client ids across reopens. Built through + // `markdownToYDoc` so it is in the canonical form persist caches; a hand-rolled doc would be + // repaired on the way through and this would assert the fast path while never taking it. + const cachedDoc = markdownToYDoc('# Anything') cachedDoc.getText('marker').insert(0, 'cached') const cached = Y.encodeStateAsUpdate(cachedDoc) mockFetchBuffer.mockResolvedValue(Buffer.from('# Anything', 'utf-8')) @@ -67,6 +76,39 @@ describe('buildFileDocSeed', () => { const doc = new Y.Doc() Y.applyUpdate(doc, seed!.update) expect(doc.getText('marker').toString()).toBe('cached') + cachedDoc.destroy() + }) + + /** + * The freshness tag is a hash of the markdown alone, so a snapshot written under older parse rules + * still reads as fresh and would otherwise be replayed verbatim forever. Repairing it here is the only + * path that ever fixes one — and the repair has to be reported, or the caller hands back the bytes it + * just decided were wrong (which is how the cached path kept reseeding docs that were missing the + * editor's trailing paragraph, letting every binding client stack another). + */ + it('repairs a cached snapshot that is not in the editor normal form', async () => { + const stale = prosemirrorJSONToYDoc( + getSchema(createMarkdownContentExtensions()), + // A raw parse — no trailing paragraph, which is exactly what a pre-normalization snapshot holds. + parseMarkdownToDoc('# T\n\nbody\n\n- a\n- b'), + COLLAB_DOC_FIELD + ) + const cached = Y.encodeStateAsUpdate(stale) + mockFetchBuffer.mockResolvedValue(Buffer.from('# T\n\nbody\n\n- a\n- b', 'utf-8')) + mockLoadFresh.mockResolvedValue(cached) + + const seed = await buildFileDocSeed('ws-1', 'file-1') + + expect(seed?.update).not.toBe(cached) + const doc = new Y.Doc() + Y.applyUpdate(doc, seed!.update) + const fragment = doc.getXmlFragment(COLLAB_DOC_FIELD) + const last = fragment.get(fragment.length - 1) + expect(last instanceof Y.XmlElement && last.nodeName === 'paragraph' && last.length === 0).toBe( + true + ) + stale.destroy() + doc.destroy() }) it('falls through to conversion when the cache read fails (never blocks a cold open)', async () => { diff --git a/apps/sim/lib/collab-doc/seed.ts b/apps/sim/lib/collab-doc/seed.ts index d0c7fa73f4d..8c25d533017 100644 --- a/apps/sim/lib/collab-doc/seed.ts +++ b/apps/sim/lib/collab-doc/seed.ts @@ -5,8 +5,7 @@ import * as Y from 'yjs' import { fetchWorkspaceFileBuffer, getWorkspaceFile } from '@/lib/uploads/contexts/workspace' import { splitFrontmatter } from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity' import { hashMarkdown, loadFreshCollabDocState } from './collab-state' -import { markdownToYDoc } from './converter' -import { stripEmptyTopLevelParagraphs } from './normalize' +import { canonicalizeYDoc, markdownToYDoc } from './converter' const logger = createLogger('FileDocSeed') @@ -28,18 +27,22 @@ export interface FileDocSeed { } /** - * Repair a cached Yjs snapshot before it seeds a room: strip any top-level empty paragraphs the markdown - * re-parse would drop (see {@link stripEmptyTopLevelParagraphs}), so a warm seed renders identically to - * the static placeholder and never surfaces a stray blank line once the doc settles. Returns the original - * bytes untouched when the snapshot is already clean (the common case) — no re-encode cost — and a fresh - * encode (preserving the CRDT's client ids, only adding tombstones for the removed empties) when it - * repaired a legacy snapshot baked before this normalization existed. + * Bring a cached Yjs snapshot into canonical form before it seeds a room, so a warm open and a cold open + * render the same document as the static placeholder (see {@link canonicalizeYDoc}). + * + * The freshness tag is a hash of the markdown alone, carrying no parser version — so a snapshot written + * under older parse rules still reads as fresh and would otherwise be replayed verbatim, with no path + * that ever repairs it. Running the round-trip here is that repair, and it doubles as the bound on this + * branch: the cached path never calls `parseMarkdownToDoc`, so it is the one way into a room that the + * parse-side limits do not cover. Returns the original bytes untouched when the snapshot is already + * canonical (the common case) — no re-encode — and a fresh encode, preserving the CRDT's client ids, when + * it repaired one. */ -function normalizeSeedUpdate(cached: Uint8Array): Uint8Array { +function canonicalSeedUpdate(cached: Uint8Array): Uint8Array { const doc = new Y.Doc() try { Y.applyUpdate(doc, cached) - return stripEmptyTopLevelParagraphs(doc) ? Y.encodeStateAsUpdate(doc) : cached + return canonicalizeYDoc(doc) ? Y.encodeStateAsUpdate(doc) : cached } finally { doc.destroy() } @@ -82,7 +85,7 @@ export async function buildFileDocSeed( // block the cold open — symmetric with persist's best-effort cache write. try { const cached = await loadFreshCollabDocState(fileId, hashMarkdown(buffer)) - if (cached) return { update: normalizeSeedUpdate(cached), version } + if (cached) return { update: canonicalSeedUpdate(cached), version } } catch (error) { logger.warn(`Failed to read cached collab doc state for file ${fileId}`, { error: getErrorMessage(error), From 35f159701de1fea35557ab91ca665b6eda6b428f Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Wed, 12 Aug 2026 22:37:35 -0700 Subject: [PATCH 2/3] fix(files): serve a whole document on join, and stop persistence deadlocking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening a file right after an edit replayed it — the block you had just moved moved again, in front of you. A room rebuilds itself from the file's Redis stream one entry at a time, into the same Y.Doc that fans every update out to its room, and the join attached the socket before that finished and before the server seed. So a client was never sent the document; it was sent the document's history, and it watched that replay. The new join-readiness test reproduces it exactly — ['AAA', 'BBBAAA'] where one state was due — and fails without the fix. Underneath it, persistence had deadlocked. The If-Match token is a REMEMBERED timestamp: held in the room, which dies with it, and in a cluster key written fire-and-forget. A relay that exits in the moments after a successful write comes back holding a version older than the file's, every persist then fails the CAS, and because a conflict neither writes nor advances the token, that document can never be persisted again. Measured on a live file: token 1786594348911 against a content version of 1786594418409, with 103 unpersisted entries still in the stream. The durable markdown froze there — and the editor's placeholder is built from it, so every reload painted the pre-edit document and the live one corrected it on screen. - Assemble a room before attaching a client to it. The join awaits the stream catch-up and the seed, so the first sync IS the finished document, in one message. The seed is memoized on the room, so concurrent joins wait for the same one instead of the second being served an empty doc; a task that loses the seed lock PULLS the winner's seed from the stream rather than waiting for the tailer to push it, which is what left a freshly uploaded file read-only until its readiness deadline lapsed. - Drop the client-side quiet-frame gate that was standing in for this. It was unsound in both directions: it delayed a document that was already correct, and it opened mid-flight anyway whenever updates arrived more than a frame apart, which is what a cross-region Redis and a long room history produce. - Await the cluster version write after a successful persist. It is the only record that survives a teardown, and one round trip after a blob write is not a cost worth a wedged document. - On a version conflict, ask the CONTENT, not the clock. If the file still holds the bytes this document last projected — the tag written with every persist — then nothing out-of-band exists to protect, so re-sync the token and write. Any other bytes and the conflict stands exactly as before. - Actually run the stale-storage-key recovery. It was requested from inside the failing read's own queryFn, where react-query drops it, so nothing re-resolved the record and the reader sat on a dead key showing "Failed to load file content" until something unrelated refetched. It now runs off that cycle and cancels a read already in flight, which could only hand back the dead key. While the record is re-resolving the surface reports loading, not failure. - One document per file, for its whole life. A document rebuilt from markdown is a DIFFERENT document to Yjs — its items carry new client ids — so a client still holding the old one merges the file into itself, twice, on both sides. The seed now stores what it builds (until that row existed, a file opened but never edited was rebuilt on every open) and resumes it with a CRDT diff when the markdown moved on out-of-band. A document also carries an identity the join ack names, so a tab that outlived its room is refused rather than merged. - Let a browser keep an embedded image. The inline route sent no-cache with no validator, so every open re-downloaded the whole image — measured at ~1 MB per open of a real document, with the image area blank until it landed. A `key` names one storage object and a content write never rewrites one, so those bytes are immutable; a `fileId` names the file, whose bytes move, so that form still revalidates. --- apps/realtime/src/handlers/file-doc-store.ts | 52 ++- .../handlers/file-doc.join-readiness.test.ts | 305 ++++++++++++++ apps/realtime/src/handlers/file-doc.test.ts | 85 ++-- apps/realtime/src/handlers/file-doc.ts | 382 +++++++++++------- .../[id]/files/inline/route.test.ts | 20 + .../api/workspaces/[id]/files/inline/route.ts | 23 +- .../collaboration/file-doc-provider.test.ts | 44 ++ .../collaboration/file-doc-provider.ts | 73 ++-- .../rich-markdown-editor.tsx | 56 +-- .../hooks/queries/workspace-files.test.tsx | 129 +++++- apps/sim/hooks/queries/workspace-files.ts | 80 +++- apps/sim/lib/collab-doc/collab-state.ts | 43 +- apps/sim/lib/collab-doc/persist.test.ts | 123 +++++- apps/sim/lib/collab-doc/persist.ts | 59 ++- apps/sim/lib/collab-doc/seed.test.ts | 131 +++++- apps/sim/lib/collab-doc/seed.ts | 84 +++- .../application/read-workspace-inline-file.ts | 17 +- packages/realtime-protocol/src/file-doc.ts | 17 + 18 files changed, 1401 insertions(+), 322 deletions(-) create mode 100644 apps/realtime/src/handlers/file-doc.join-readiness.test.ts diff --git a/apps/realtime/src/handlers/file-doc-store.ts b/apps/realtime/src/handlers/file-doc-store.ts index f7b8fc5180a..c806da5e6be 100644 --- a/apps/realtime/src/handlers/file-doc-store.ts +++ b/apps/realtime/src/handlers/file-doc-store.ts @@ -18,10 +18,12 @@ * client receives each update exactly once, from its own task's local broadcast — no adapter * amplification, and every task's doc stays converged. (Awareness/presence stay on the adapter: they * are ephemeral and need no convergence or replay.) - * - {@link attachRoom} does a synchronous catch-up read from the head of the stream when a task first - * opens a file, so a late-joining task (the normal case under autoscaling) loads the current shared - * state before its first client syncs. Catch-up + tail are seamless: the tailer resumes from the - * exact id catch-up stopped at. + * - {@link attachRoom} reads the stream from the head when a task first opens a file, and the relay + * AWAITS it before attaching a client, so a late-joining task (the normal case under autoscaling) + * holds the current shared state before its first client syncs — a client must never watch the + * catch-up land entry by entry, which is the document's edit history replaying on screen. Catch-up + + * tail are seamless: the tailer resumes from the exact id catch-up stopped at, and {@link catchUp} + * can re-run at any time for a caller that must converge without waiting on the tailer. * - The one-time seed is written via the atomic {@link seedIfEmpty} (append-iff-empty in one Redis * step), so exactly one task ever writes the seed cluster-wide (the fix for split-brain) — even if two * tasks race. {@link shouldSeed} is a Redis lock + empty-stream check layered on top ONLY as an @@ -185,6 +187,18 @@ function applyEntryToDoc( } } +/** + * Whether stream id `id` sorts after `than`. A Redis stream id is `-`, so a lexicographic + * compare is wrong the moment the millisecond part changes digit length (`'9999-0' > '10000-0'`); + * compare the two parts numerically instead. The initial `'0'` (nothing applied) has no `-seq` part, + * which reads as sequence 0 — before every real entry. + */ +function isAfterStreamId(id: string, than: string): boolean { + const [ms, seq = '0'] = id.split('-') + const [thanMs, thanSeq = '0'] = than.split('-') + return Number(ms) === Number(thanMs) ? Number(seq) > Number(thanSeq) : Number(ms) > Number(thanMs) +} + /** Whether a doc carries the seed flag (mirrors the relay's `isDocSeeded`), so the store can tell the * one-time seed transition from a real post-seed edit without re-implementing the check divergently. */ function isDocSeeded(doc: Y.Doc): boolean { @@ -260,10 +274,9 @@ export class FileDocStore { } /** - * Register a locally-opened room and load the shared state into its doc: read the whole stream from - * the head, apply every entry (origin {@link REDIS_ORIGIN}), and remember the last id so the tailer - * resumes exactly after it. A brand-new file has an empty stream and loads nothing (it is seeded - * shortly after, via {@link shouldSeed}). No-op when disabled. + * Register a locally-opened room and load the shared state into its doc ({@link catchUp}). A + * brand-new file has an empty stream and loads nothing (it is seeded shortly after, via + * {@link shouldSeed}). No-op when disabled. */ async attachRoom(name: string, doc: Y.Doc): Promise { if (!this.enabled || !this.write) return @@ -277,12 +290,31 @@ export class FileDocStore { realEdited: false, } this.rooms.set(name, room) + await this.catchUp(name) + } + + /** + * PULL the shared state into a registered room: read the stream and apply every entry the doc has + * not integrated yet (origin {@link REDIS_ORIGIN}), advancing `lastId` so the tailer resumes exactly + * after it. This is the ONLY way a room loads shared state, so a caller that must not depend on the + * tailer's asynchronous push — the join, which may not serve a client a half-assembled document — + * can converge on demand. Idempotent and safe to call repeatedly; no-op when disabled or the room is + * not registered (a fast open→close detached it). Never throws. + */ + async catchUp(name: string): Promise { + if (!this.enabled || !this.write) return + const room = this.rooms.get(name) + if (!room) return try { const entries = await this.write.xRange(streamKey(name), '-', '+') for (const entry of entries) { - // The room can be detached + its doc destroyed while catch-up is in flight (a fast open→close); - // stop touching it the moment that happens. + // The room can be detached + its doc destroyed while the read is in flight (a fast + // open→close); stop touching it the moment that happens. if (this.rooms.get(name) !== room) return + // Applying a Yjs update twice is a no-op, but `applyEntry`'s bookkeeping is not: re-applying + // the SEED after `seededObserved` latched would count it as a post-seed edit and let a + // compaction snapshot claim content no user ever typed. Skip what this room already holds. + if (!isAfterStreamId(entry.id, room.lastId)) continue this.applyEntry(room, entry.id, entry.message) } await this.write.expire(streamKey(name), STREAM_TTL_SEC) diff --git a/apps/realtime/src/handlers/file-doc.join-readiness.test.ts b/apps/realtime/src/handlers/file-doc.join-readiness.test.ts new file mode 100644 index 00000000000..9b7b6a1c7ec --- /dev/null +++ b/apps/realtime/src/handlers/file-doc.join-readiness.test.ts @@ -0,0 +1,305 @@ +/** + * @vitest-environment node + * + * The join's readiness contract, with the shared store ENABLED (`file-doc.test.ts` runs it disabled). + * + * A room loads its document from the file's Redis stream one entry at a time, into the same `Y.Doc` + * that fans every update out to the room. So a client attached while that is happening is not sent the + * document — it is sent the document's history, and it watches the history replay on screen (reload + * right after moving a block and the block moves again in front of you). These tests pin the fix: the + * join waits for the room to hold the whole document, so the client's first sync is authoritative. + */ +import { + FILE_DOC_EVENTS, + FILE_DOC_MESSAGE_TYPE, + FILE_DOC_SEED, +} from '@sim/realtime-protocol/file-doc' +import * as decoding from 'lib0/decoding' +import * as encoding from 'lib0/encoding' +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' +import * as syncProtocol from 'y-protocols/sync' +import * as Y from 'yjs' +import type { IRoomManager } from '@/rooms' + +const { mockAuthorizeRoom, mockFetchFileDocSeed } = vi.hoisted(() => ({ + mockAuthorizeRoom: vi.fn(), + mockFetchFileDocSeed: vi.fn(), +})) + +vi.mock('@sim/platform-authz/rooms', () => ({ authorizeRoom: mockAuthorizeRoom })) + +vi.mock('@/handlers/file-doc-app', () => ({ + fetchFileDocSeed: mockFetchFileDocSeed, + fetchFileDocMerge: vi.fn(), + fetchFileDocPersist: vi.fn().mockResolvedValue({ status: 'persisted', version: 1 }), +})) + +/** One in-memory Redis backing per test — only the stream/lock ops the store actually uses. */ +const backing = vi.hoisted(() => ({ + streams: new Map }[]>(), + kv: new Map(), + seq: 0, + /** Ticks of event-loop delay each xRange takes, modelling a remote (cross-region) Redis. */ + readDelayTicks: 0, +})) + +const seqOf = (id: string) => Number(id.split('-')[0]) + +vi.mock('redis', () => { + const makeClient = (): Record => { + const client: Record = { + connect: async () => {}, + quit: async () => {}, + on: () => client, + duplicate: () => makeClient(), + xAdd: async (key: string, _star: string, fields: Record) => { + const id = `${++backing.seq}-0` + const arr = backing.streams.get(key) ?? [] + arr.push({ id, message: { ...fields } }) + backing.streams.set(key, arr) + return id + }, + xRange: async (key: string) => { + for (let i = 0; i < backing.readDelayTicks; i++) await Promise.resolve() + return (backing.streams.get(key) ?? []).map((e) => ({ ...e })) + }, + xLen: async (key: string) => (backing.streams.get(key) ?? []).length, + xRead: async (streams: { key: string; id: string }[]) => { + const res: { name: string; messages: { id: string; message: Record }[] }[] = + [] + for (const { key, id } of streams) { + const after = (backing.streams.get(key) ?? []).filter((e) => seqOf(e.id) > seqOf(id)) + if (after.length) res.push({ name: key, messages: after.map((e) => ({ ...e })) }) + } + if (res.length) return res + await new Promise((r) => setTimeout(r, 5)) + return null + }, + set: async (key: string, val: string, opts?: { NX?: boolean }) => { + if (opts?.NX && backing.kv.has(key)) return null + backing.kv.set(key, val) + return 'OK' + }, + eval: async (script: string, opts: { keys: string[]; arguments: string[] }) => { + const [key] = opts.keys + if (script.includes('xlen')) { + const [field, value] = opts.arguments + const arr = backing.streams.get(key) ?? [] + if (arr.length > 0) return 0 + arr.push({ id: `${++backing.seq}-0`, message: { [field]: value } }) + backing.streams.set(key, arr) + return 1 + } + const [token] = opts.arguments + if (backing.kv.get(key) === token) { + backing.kv.delete(key) + return 1 + } + return 0 + }, + expire: async () => 1, + get: async (key: string) => backing.kv.get(key) ?? null, + exists: async (key: string) => (backing.kv.has(key) ? 1 : 0), + } + return client + } + return { createClient: () => makeClient() } +}) + +import { cleanupFileDocForSocket, setupWorkspaceFileDocHandlers } from '@/handlers/file-doc' +import { getFileDocStore, initFileDocStore } from '@/handlers/file-doc-store' + +const FILE_ID = 'file-1' +const ROOM_NAME = `workspace-file-doc:${FILE_ID}` +const STREAM_KEY = `filedoc:stream:${ROOM_NAME}` +const FIELD = 'default' + +type Handler = (payload?: unknown) => Promise | void + +interface FakeSocket { + id: string + emit: (event: string, payload: unknown) => void + rooms: Set +} + +/** + * An `io` that actually DELIVERS: a room emit reaches every socket that joined that room, so a frame + * the relay fans out mid-assembly lands on the joiner's `emit` exactly as it would in the browser. + * Recording the emits without routing them would hide the very thing these tests are about. + */ +function createIo(sockets: FakeSocket[]) { + const emitTo = (target: string, except: string | null, event: string, payload: unknown) => { + for (const socket of sockets) { + if (socket.id === except || !socket.rooms.has(target)) continue + socket.emit(event, payload) + } + } + const to = vi.fn((target: string) => ({ + except: (exclude: string) => ({ + emit: (event: string, payload: unknown) => emitTo(target, exclude, event, payload), + }), + emit: (event: string, payload: unknown) => emitTo(target, null, event, payload), + })) + return { + to, + in: vi.fn(() => ({ socketsLeave: () => {} })), + local: { to }, + } as unknown as IRoomManager['io'] +} + +function setup(id: string, sockets: FakeSocket[]) { + const handlers: Record = {} + const rooms = new Set() + const socket = { + id, + userId: 'user-1', + userName: 'Test User', + userImage: 'avatar.png', + disconnected: false, + rooms, + on: vi.fn((event: string, handler: Handler) => { + handlers[event] = handler + }), + emit: vi.fn(), + join: vi.fn((name: string) => rooms.add(name)), + leave: vi.fn((name: string) => rooms.delete(name)), + } + sockets.push(socket as unknown as FakeSocket) + setupWorkspaceFileDocHandlers( + socket as unknown as Parameters[0], + { isReady: () => true, io: createIo(sockets) } as unknown as IRoomManager + ) + return { socket, handlers } +} + +/** Append a Yjs update to the file's stream, exactly as `publish`/`seedIfEmpty` would. */ +function appendToStream(update: Uint8Array): void { + const arr = backing.streams.get(STREAM_KEY) ?? [] + arr.push({ id: `${++backing.seq}-0`, message: { u: Buffer.from(update).toString('base64') } }) + backing.streams.set(STREAM_KEY, arr) +} + +/** + * A warm room's history: the seed, then a later edit — the "I moved a block, then reloaded" case. + * Returns the markdown-equivalent text of each state. + */ +function seedWarmStreamHistory(): { intermediate: string; final: string } { + const doc = new Y.Doc() + doc.getText(FIELD).insert(0, 'AAA') + doc.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.flag, true) + appendToStream(Y.encodeStateAsUpdate(doc)) + const afterSeed = Y.encodeStateVector(doc) + doc.getText(FIELD).insert(0, 'BBB') + appendToStream(Y.encodeStateAsUpdate(doc, afterSeed)) + doc.destroy() + return { intermediate: 'AAA', final: 'BBBAAA' } +} + +/** Every document state this socket was ever shown, in order. */ +function statesDeliveredTo(socket: { emit: ReturnType }): string[] { + const clientDoc = new Y.Doc() + const states: string[] = [] + for (const [event, payload] of socket.emit.mock.calls) { + if (event !== FILE_DOC_EVENTS.MESSAGE || !(payload instanceof Uint8Array)) continue + const decoder = decoding.createDecoder(payload) + if (decoding.readVarUint(decoder) !== FILE_DOC_MESSAGE_TYPE.SYNC) continue + syncProtocol.readSyncMessage(decoder, encoding.createEncoder(), clientDoc, null) + const text = clientDoc.getText(FIELD).toString() + if (text !== (states.at(-1) ?? '')) states.push(text) + } + clientDoc.destroy() + return states +} + +/** Let anything the join left running (a catch-up, a seed) settle, so a frame it fans out afterwards + * is counted — that late delivery IS the replay these tests exist to rule out. */ +async function flushPendingWork(): Promise { + for (let i = 0; i < 20; i++) await Promise.resolve() +} + +/** Ask the server for its state the way a client does after the join ack. */ +function requestSyncStep2(handlers: Record): void { + const encoder = encoding.createEncoder() + encoding.writeVarUint(encoder, FILE_DOC_MESSAGE_TYPE.SYNC) + syncProtocol.writeSyncStep1(encoder, new Y.Doc()) + handlers[FILE_DOC_EVENTS.MESSAGE](encoding.toUint8Array(encoder)) +} + +describe('file-doc join readiness (shared store enabled)', () => { + /** Every socket the test created, so a room emit can be routed to its members. */ + const sockets: FakeSocket[] = [] + + // One store for the whole file: `initFileDocStore` is idempotent once enabled, so a per-test + // shutdown would leave every later test running against a store with closed clients. + beforeAll(async () => { + await initFileDocStore('redis://fake') + }) + + afterAll(async () => { + await getFileDocStore().shutdown() + }) + + beforeEach(() => { + vi.clearAllMocks() + backing.streams.clear() + backing.kv.clear() + backing.seq = 0 + backing.readDelayTicks = 0 + mockAuthorizeRoom.mockResolvedValue({ + allowed: true, + status: 200, + workspaceId: 'ws-1', + workspacePermission: 'write', + }) + mockFetchFileDocSeed.mockResolvedValue(null) + }) + + afterEach(() => { + cleanupFileDocForSocket('socket-1', createIo(sockets), true) + sockets.length = 0 + }) + + it('hands a joiner the final document, never the room history it was rebuilt from', async () => { + const { intermediate, final } = seedWarmStreamHistory() + // The catch-up read is not instantaneous — the case that made this visible is a cross-region Redis. + backing.readDelayTicks = 6 + const { socket, handlers } = setup('socket-1', sockets) + + await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: FILE_ID, clientId: 1 }) + requestSyncStep2(handlers) + await flushPendingWork() + + // One state, and it is the final one: the client never saw the pre-move document. + expect(statesDeliveredTo(socket)).toEqual([final]) + expect(statesDeliveredTo(socket)).not.toContain(intermediate) + }) + + it('does not fetch a seed for a room the stream can already reconstruct', async () => { + seedWarmStreamHistory() + const { handlers } = setup('socket-1', sockets) + + await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: FILE_ID, clientId: 1 }) + + expect(mockFetchFileDocSeed).not.toHaveBeenCalled() + }) + + it('pulls a seed another writer put in the stream instead of waiting for the tailer to push it', async () => { + // The seed lock is held by a writer whose room has since been dropped (a fast open→close on a + // freshly created file), and its seed lands in the stream. Waiting to be told about it is what + // left a new file un-editable until the client's readiness deadline lapsed; the join reads it. + backing.kv.set(`filedoc:seedlock:${ROOM_NAME}`, 'held-by-a-writer-that-is-gone') + const doc = new Y.Doc() + doc.getText(FIELD).insert(0, 'seeded by the writer that held the lock') + doc.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.flag, true) + appendToStream(Y.encodeStateAsUpdate(doc)) + doc.destroy() + + const { socket, handlers } = setup('socket-1', sockets) + await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: FILE_ID, clientId: 1 }) + requestSyncStep2(handlers) + await flushPendingWork() + + expect(mockFetchFileDocSeed).not.toHaveBeenCalled() + expect(statesDeliveredTo(socket)).toEqual(['seeded by the writer that held the lock']) + }) +}) diff --git a/apps/realtime/src/handlers/file-doc.test.ts b/apps/realtime/src/handlers/file-doc.test.ts index 938092d4484..4ba34c83de1 100644 --- a/apps/realtime/src/handlers/file-doc.test.ts +++ b/apps/realtime/src/handlers/file-doc.test.ts @@ -570,22 +570,42 @@ describe('setupWorkspaceFileDocHandlers', () => { expect(clientDoc.getText(FILE_DOC_FIELD).toString()).toBe('# From server') }) - it('seeds the document only once from the server across concurrent joiners of the same file', async () => { + it('seeds once across concurrent joiners, and every one of them waits for that seed', async () => { // Keep the first seed fetch IN FLIGHT so the doc is still unseeded when the second socket joins: - // that forces the dedup onto `serverSeedStarted` (the in-flight guard) rather than `isDocSeeded`. + // that forces the dedup onto the in-flight seed rather than `isDocSeeded`. Both joins must WAIT + // for it — a joiner answered before the seed would be handed an empty document and would then + // watch the content arrive as a live update. let resolveSeed: (v: { update: Uint8Array; version: number } | null) => void = () => {} mockFetchFileDocSeed.mockReturnValueOnce(new Promise((resolve) => (resolveSeed = resolve))) const { io } = createIo() const a = setup('socket-a', io) const b = setup('socket-b', io) - await a.handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 1 }) - await b.handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 2 }) - // Second join happened with the fetch still pending; only after this does the seed land. + const joinA = a.handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 1 }) + const joinB = b.handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 2 }) + await flushMicrotasks() + + // The second join found the seed already in flight, so it does not start another one — and + // neither join has been answered yet. expect(mockFetchFileDocSeed).toHaveBeenCalledTimes(1) + expect(joinSuccessFileId(a.socket)).toBeUndefined() + expect(joinSuccessFileId(b.socket)).toBeUndefined() + resolveSeed(seedResult('# From server')) - await flushMicrotasks() + await Promise.all([joinA, joinB]) expect(mockFetchFileDocSeed).toHaveBeenCalledTimes(1) + + // The joiner that never triggered the fetch is served the seeded document all the same. + b.socket.emit.mockClear() + b.handlers[FILE_DOC_EVENTS.MESSAGE]( + frame(FILE_DOC_MESSAGE_TYPE.SYNC, (e) => syncProtocol.writeSyncStep1(e, new Y.Doc())) + ) + const reply = b.socket.emit.mock.calls.find( + ([event, payload]) => event === FILE_DOC_EVENTS.MESSAGE && payload instanceof Uint8Array + ) + const clientDoc = new Y.Doc() + applySyncReply(reply?.[1] as Uint8Array, clientDoc) + expect(clientDoc.getText(FILE_DOC_FIELD).toString()).toBe('# From server') }) it('marks an empty/absent-file doc seeded so clients still reach readiness', async () => { @@ -640,43 +660,58 @@ describe('setupWorkspaceFileDocHandlers', () => { expect(clientDoc.getText(FILE_DOC_FIELD).toString()).toBe('# Recovered') }) - it('does not seed a room that was dropped while the seed fetch was in flight', async () => { + it('does not seed a room the joiner abandoned while the seed fetch was in flight', async () => { let resolveSeed: (v: { update: Uint8Array; version: number } | null) => void = () => {} mockFetchFileDocSeed.mockReturnValueOnce(new Promise((resolve) => (resolveSeed = resolve))) const { io } = createIo() - const { handlers } = setup('socket-1', io) + const { socket, handlers } = setup('socket-1', io) - await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 1 }) - // The only owner leaves → the room (and its doc) is destroyed while the fetch is still pending. - cleanupFileDocForSocket('socket-1', io, true) + const joining = handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 1 }) + await flushMicrotasks() + // The client leaves before the room finished assembling → the join aborts and drops the room it + // was preparing (nothing else owns it). + handlers[FILE_DOC_EVENTS.LEAVE]({ fileId: 'file-1' }) // Resolving now must not touch the destroyed doc or throw (liveness re-check after the await). resolveSeed(seedResult('# Too late')) - await expect(flushMicrotasks()).resolves.toBeUndefined() + await expect(joining).resolves.toBeUndefined() + expect(joinSuccessFileId(socket)).toBeUndefined() + expect(socket.join).not.toHaveBeenCalled() }) - it('still seeds when content was synced into the doc before the seed returned', async () => { - // Defensive: the guard is `isDocSeeded`, NOT doc-emptiness. In practice a fresh client never - // writes ahead of the seed (@tiptap/y-tiptap suppresses the empty-paragraph placeholder and real - // edits are readiness-gated), but even if some update landed content in the doc before the seed - // fetch resolved, the seed must still apply and set the flag — or the client's - // `synced && initialContentLoaded` gate would never open. + it('attaches a client only once the document is whole — no empty sync, no frames before it', async () => { + // The room assembles itself into the same doc that fans updates out to its room, so a socket + // attached mid-assembly receives the document's history rather than the document. Nothing about + // the client exists in the room until the seed has landed: no membership, no sync, and any frame + // it sends meanwhile is not applied. let resolveSeed: (v: { update: Uint8Array; version: number } | null) => void = () => {} mockFetchFileDocSeed.mockReturnValueOnce(new Promise((resolve) => (resolveSeed = resolve))) const { io } = createIo() const { socket, handlers } = setup('socket-1', io) - await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 1 }) + const joining = handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 1 }) + await flushMicrotasks() - // The client syncs a placeholder update — content in the doc, but no seed flag. - const placeholder = new Y.Doc() - placeholder.getText(FILE_DOC_FIELD).insert(0, 'x') + expect(socket.join).not.toHaveBeenCalled() + expect(joinSuccessFileId(socket)).toBeUndefined() + expect( + socket.emit.mock.calls.some( + ([event, payload]) => event === FILE_DOC_EVENTS.MESSAGE && payload instanceof Uint8Array + ) + ).toBe(false) + + // A document frame sent before the join was answered reaches an unbound socket and is dropped. + const early = new Y.Doc() + early.getText(FILE_DOC_FIELD).insert(0, 'too early') handlers[FILE_DOC_EVENTS.MESSAGE]( frame(FILE_DOC_MESSAGE_TYPE.SYNC, (e) => - syncProtocol.writeUpdate(e, Y.encodeStateAsUpdate(placeholder)) + syncProtocol.writeUpdate(e, Y.encodeStateAsUpdate(early)) ) ) + resolveSeed(seedResult('# Seeded')) - await flushMicrotasks() + await joining + expect(joinSuccessFileId(socket)).toBe('file-1') + // The first thing the client is served is the finished document — content and seed flag together. socket.emit.mockClear() handlers[FILE_DOC_EVENTS.MESSAGE]( frame(FILE_DOC_MESSAGE_TYPE.SYNC, (e) => syncProtocol.writeSyncStep1(e, new Y.Doc())) @@ -687,7 +722,7 @@ describe('setupWorkspaceFileDocHandlers', () => { const clientDoc = new Y.Doc() applySyncReply(reply?.[1] as Uint8Array, clientDoc) expect(clientDoc.getMap(FILE_DOC_SEED.configMap).get(FILE_DOC_SEED.flag)).toBe(true) - expect(clientDoc.getText(FILE_DOC_FIELD).toString()).toContain('# Seeded') + expect(clientDoc.getText(FILE_DOC_FIELD).toString()).toBe('# Seeded') }) it('merges a copilot edit into a seeded live room and relays it to editors', async () => { diff --git a/apps/realtime/src/handlers/file-doc.ts b/apps/realtime/src/handlers/file-doc.ts index a0152fd85f6..1e29f476de9 100644 --- a/apps/realtime/src/handlers/file-doc.ts +++ b/apps/realtime/src/handlers/file-doc.ts @@ -131,9 +131,13 @@ interface FileDocRoom { /** socketId → (clientId → its presence ownership). A socket owns one entry per collaborative provider * it mounted for this file (see {@link FileDocOwner}); an empty inner map is never kept. */ owners: Map> - /** True once the server-side seed fetch has started, so concurrent joins don't each fetch. - * Reset on a fetch FAILURE so a later join can retry (a genuinely empty file stays empty). */ - serverSeedStarted: boolean + /** + * The in-flight server seed for this room, or `null`. Concurrent joins await THIS promise rather + * than each starting a fetch — and, unlike a "started" boolean, awaiting it is what lets a second + * joiner be served a document that is already seeded instead of an empty one. Cleared when it + * settles, so a failed seed is re-attempted by a later join (a genuinely empty file stays empty). + */ + seeding: Promise | null /** The workspace this file belongs to, captured at join — needed to persist back to markdown. */ workspaceId: string | null /** The last collaborator to edit here, for persist attribution (blob metadata) only. */ @@ -170,6 +174,17 @@ interface FileDocRoom { * {@link FileDocStore.isAgentStreaming} flag. `0` when no agent stream is active. */ agentStreamingUntil: number + /** + * Resolves once this room's doc reflects the file's shared stream (see {@link FileDocStore.catchUp}). + * Never rejects — the catch-up logs and gives up — so awaiting it can never fail a join. + */ + hydrated: Promise + /** + * How many joins are currently preparing this room. A room is created by the first join and has no + * owner until that join commits, so without this a concurrent last-leave would tear down the very + * document being assembled. A room with a join in flight is not idle. + */ + pendingJoins: number } /** Live documents keyed by Socket.IO room name. Module-global: one Y.Doc per file. */ @@ -360,7 +375,12 @@ async function flushPersist(name: string, room: FileDocRoom, final: boolean): Pr } if (result.status === 'persisted') { room.syncedVersion = Math.max(room.syncedVersion ?? 0, result.version) - void store.setSyncedVersion(name, result.version) + // AWAITED, unlike every other version write: the room's own copy dies with the room, so this + // cluster key is the only record that survives a teardown or a process restart. Fire-and-forget + // here means a task that exits in the moments after a write comes back holding a version older + // than the file's, and — since a conflict neither writes nor advances the token — never persists + // that document again. One round trip after a blob write is not a cost worth that. + await store.setSyncedVersion(name, result.version) return } // status === 'conflict': the durable file advanced out-of-band since our If-Match token. We do NOT @@ -412,6 +432,12 @@ function isDocSeeded(doc: Y.Doc): boolean { return doc.getMap(FILE_DOC_SEED.configMap).get(FILE_DOC_SEED.flag) === true } +/** The identity of the document this doc holds ({@link FILE_DOC_SEED.docIdKey}), if it carries one. */ +function docIdOf(doc: Y.Doc): string | undefined { + const docId = doc.getMap(FILE_DOC_SEED.configMap).get(FILE_DOC_SEED.docIdKey) + return typeof docId === 'string' ? docId : undefined +} + /** * Decode the client IDs an awareness update carries, without applying it, to * check a frame only touches its sender's own presence. Mirrors the wire format @@ -435,10 +461,13 @@ function awarenessUpdateClientIds(update: Uint8Array): number[] { * memory. Before dropping, flush the converged doc back to durable markdown (the last collaborator on * this task leaving) and detach from the shared stream. A later joiner re-creates it — catching up * from the stream if the doc is still live on another task, or re-seeding from markdown otherwise. + * + * A room being PREPARED for a join is not idle even though it has no owners yet: tearing it down there + * would drop the hydration/seed that join is waiting on, and the join would have to start over. */ function destroyRoomIfIdle(name: string) { const room = fileDocRooms.get(name) - if (!room || room.owners.size > 0) return + if (!room || room.owners.size > 0 || room.pendingJoins > 0) return room.persistDeadline = null if (room.persistTimer) { clearTimeout(room.persistTimer) @@ -469,40 +498,83 @@ export async function flushAllFileDocRooms(): Promise { } /** - * Seed a room's document server-side, once, on the first join: ask the app to build the seed (the - * file's current markdown → Yjs, through the exact editor engine) and apply it, which relays the - * content to every connected client via `doc.on('update')`. No client is elected to import content. + * Bring a room's document to its AUTHORITATIVE state — reflecting the file's shared stream and + * carrying its seed — so the join can attach a client to a document that is already whole. Never + * rejects: a room that cannot be seeded is served unseeded, which the client's readiness deadline + * turns into its read-only fallback, exactly as an unreachable relay does. + */ +async function ensureRoomReady( + name: string, + room: FileDocRoom, + workspaceId: string | null +): Promise { + await room.hydrated + // The room can be dropped and re-created while the catch-up is in flight (a fast open→close); the + // join re-checks identity after this and abandons a stale room rather than serving from it. + if (fileDocRooms.get(name) !== room || !workspaceId) return + await ensureServerSeed(name, room, workspaceId) +} + +/** + * Seed a room's document server-side, once: ask the app to build the seed (the file's current markdown + * → Yjs, through the exact editor engine) and apply it. No client is elected to import content. + * + * MEMOIZED on the room, so concurrent joins await the same seed instead of the second one being served + * an empty document while the first one's fetch is still in flight. Cleared when it settles: a failed + * seed is re-attempted by the next join (a genuinely empty file stays empty and needs no retry). * * `isDocSeeded` is the sufficient guard: content only ever reaches the doc alongside the seed flag * (this seed, or a client's offline fallback), so an unseeded doc is genuinely empty and safe to seed. * A genuinely empty/missing file returns `null` (a read error throws instead), so still set the flag — - * an empty doc must reach readiness, not wait forever. After the fetch, re-check the room is still - * live and unseeded (an owner may have left, or a client seeded it, while the fetch was in flight). - * - * Recovery on failure is deliberately simple — no in-room retry loop: a single attempt bounded by a - * timeout shorter than the client's readiness deadline, then release the guard. A transient failure - * is re-attempted by the next join/reconnect; a persistent one lets the connected client's readiness - * deadline lapse into its read-only fallback. (An in-room backoff retry can outlast that client - * deadline, so it would keep trying a doc the client has already given up on — worse, not better.) + * an empty doc must reach readiness, not wait forever. */ -async function ensureServerSeed( +function ensureServerSeed(name: string, room: FileDocRoom, workspaceId: string): Promise { + if (isDocSeeded(room.doc)) return Promise.resolve() + room.seeding ??= runServerSeed(name, room, workspaceId).finally(() => { + room.seeding = null + }) + return room.seeding +} + +/** + * Whichever task wins the seed lock writes the seed; the others must end up holding the SAME seed + * before they serve anyone. They pull it, on this cadence, rather than waiting for the tailer to push + * it: a join's readiness may not depend on an asynchronous subscriber, because when that delivery is + * late or lost the client sits on an empty document until its readiness deadline lapses and the file + * opens read-only. Bounded by the longest a legitimate seed can take (the winner's own fetch bound), + * which stays inside the client's readiness deadline — see {@link FILE_DOC_TIMEOUTS}. + */ +const SEED_WAIT_RETRY_MS = 150 + +async function runServerSeed(name: string, room: FileDocRoom, workspaceId: string): Promise { + const store = getFileDocStore() + const deadline = Date.now() + FILE_DOC_TIMEOUTS.seedRequestMs + while (fileDocRooms.get(name) === room && !isDocSeeded(room.doc)) { + // Exactly one task across the cluster builds the seed; the others receive it via the stream (the + // fix for split-brain seeding). Returns a lock token here (single-pod: a sentinel token). + const token = await store.shouldSeed(name) + if (token) { + await seedUnderLock(name, room, workspaceId, token) + return + } + // No token: a peer holds the lock with its fetch in flight, or the stream is already seeded (which + // includes a PRIOR room for this same file whose seed landed after we read the stream). Either way + // the seed can only appear in the stream, so read it rather than wait to be told. + await store.catchUp(name) + if (isDocSeeded(room.doc) || Date.now() >= deadline) return + await sleep(SEED_WAIT_RETRY_MS) + } +} + +/** Fetch, publish, and apply the seed while holding the cluster's seed lock for this file. */ +async function seedUnderLock( name: string, room: FileDocRoom, - workspaceId: string + workspaceId: string, + token: string ): Promise { - if (room.serverSeedStarted || isDocSeeded(room.doc)) return - room.serverSeedStarted = true const store = getFileDocStore() - // Exactly one task across the cluster builds the seed; the others receive it via the stream (the fix - // for split-brain seeding). Returns a lock token here (single-pod: a sentinel token). - const token = await store.shouldSeed(name) - if (!token) { - // A peer is seeding (or already did). Release our guard so a later join can retry if the seed never - // arrives (e.g. the seeder died); the stream / this doc being seeded makes a retry safe. - room.serverSeedStarted = false - return - } - // We hold the seed lock — release it on EVERY exit from here (one `finally`, impossible to leak). + // Release the lock on EVERY exit from here (one `finally`, impossible to leak). try { const seed = await fetchFileDocSeed(workspaceId, room.fileId) if (fileDocRooms.get(name) !== room || isDocSeeded(room.doc)) return @@ -533,15 +605,13 @@ async function ensureServerSeed( if (didSeed) { Y.applyUpdate(room.doc, seedUpdate, SEED_ORIGIN) } else { - // A peer seeded first: its seed arrives via the tailer, so we must NOT apply our own — a second, - // different-client-id seed IS the split-brain. Clear the guard so a later join can retry if that - // peer seed somehow never lands (e.g. a fail-closed `xLen` error made `shouldSeed` skip a genuinely - // empty stream); a real peer-seed makes the retry a no-op. - room.serverSeedStarted = false + // A peer won the atomic append: we must NOT apply our own — a second, different-client-id seed IS + // the split-brain. Read THEIRS out of the stream instead of waiting for the tailer to deliver it, + // so this room is seeded by the time the caller is told it is ready. + await store.catchUp(name) } } catch (error) { logger.warn(`Server seed failed for file ${room.fileId} (workspace ${workspaceId})`, error) - room.serverSeedStarted = false } finally { await store.releaseSeedLock(name, token) } @@ -725,12 +795,14 @@ function getOrCreateRoom(io: Server, ref: RoomRef): FileDocRoom { // The server holds no cursor of its own; it only relays clients' awareness. awareness.setLocalState(null) + // Started BEFORE the room is registered so no join can observe a room without its hydration handle. + const hydrated = getFileDocStore().attachRoom(name, doc) const room: FileDocRoom = { fileId: ref.id, doc, awareness, owners: new Map(), - serverSeedStarted: false, + seeding: null, workspaceId: null, lastEditorUserId: null, edited: false, @@ -739,6 +811,8 @@ function getOrCreateRoom(io: Server, ref: RoomRef): FileDocRoom { persistDeadline: null, syncedVersion: null, agentStreamingUntil: 0, + hydrated, + pendingJoins: 0, } // Register synchronously BEFORE the async catch-up so a concurrent join sees this room, not a second. fileDocRooms.set(name, room) @@ -818,10 +892,6 @@ function getOrCreateRoom(io: Server, ref: RoomRef): FileDocRoom { broadcast(io, name, encoding.toUint8Array(encoder), originSocketId(origin)) }) - // Load the shared state into the doc and start tailing the stream (fire-and-forget: content streams - // in via `doc.on('update')` as it lands, mirroring the fire-and-forget seed below). Disabled → no-op. - void getFileDocStore().attachRoom(name, doc) - return room } @@ -1091,117 +1161,143 @@ export function setupWorkspaceFileDocHandlers( // awareness). Resolved here so the generation guard below also covers this await. const avatarUrl = await resolveAvatarUrl(socket, userId) - // Re-check access immediately before registering, mirroring the workflow join: the - // access re-validation sweep records a revocation BEFORE it evicts, so a join that - // authorized just before the revocation must not complete afterwards and re-bind - // the socket to the document. This RE-RESOLVES rather than peeking the cache — a - // peek treats an expired entry as unknown and fails open, which a join stalled - // longer than the cache TTL would slip straight through. Normally a cache hit (this - // join's own authorize just warmed it), so it costs no extra query. - const currentPermission = await resolveCurrentRoomPermission(userId, room, FILE_DOC_ACTION) - if (!satisfiesRoomMembership(currentPermission, ROOM_TYPES.WORKSPACE_FILE_DOC)) { - logger.warn(`User ${userId} lost write access to file ${fileId} before the join completed`) - emitJoinError(socket, fileId, 'Access denied to file', 'ACCESS_DENIED', false) - return - } - - // Abort a JOIN superseded during authorization/identity resolution: the socket - // disconnected, or a newer JOIN (a document switch) bumped the generation. Registering - // here would leak a dead socket's room or bind the socket to the wrong document. - // Last await before the commit, so nothing can interleave between the access - // re-check above and the registration below. - if (socket.disconnected || joinGeneration.get(socket.id) !== generation) return - const entry = getOrCreateRoom(io, room) + // The workspace the server-side persist writes back to — and what the seed is built from, so it + // must be captured BEFORE the room is prepared below. + if (authorized.workspaceId) entry.workspaceId = authorized.workspaceId - // A client id must be owned by at most one user, or a peer could bind an active - // collaborator's id and pass the per-frame ownership check to spoof/clear its caret. - // Distinguish a reconnect from a spoof by the owning user: the same user reclaiming its - // own client id (a dropped socket reconnecting reuses the Yjs client id, and its prior - // socket may not be cleaned up yet) takes over the stale binding; a DIFFERENT user is - // rejected. This runs BEFORE any teardown of the socket's current binding below, so a - // rejected rebind — even during a document switch — leaves the socket's existing document - // and caret untouched. - for (const [otherSid, clientMap] of entry.owners) { - if (otherSid === socket.id) continue - const owner = clientMap.get(clientId) - if (owner === undefined) continue - if (owner.userId !== userId) { - emitJoinError(socket, fileId, 'Client id already in use', 'CLIENT_ID_IN_USE', false) + // Hold the room open across the awaits below: it has no owner until this join commits, so a + // concurrent last-leave would otherwise tear down the very document being prepared. + entry.pendingJoins += 1 + try { + // A client is attached to a WHOLE document or to nothing. A room assembles itself from the + // shared stream and the server seed, and both land in the same Y.Doc that fans every update out + // to its room — so a socket attached mid-assembly is not sent the document, it is sent the + // document's history, and it watches that replay on screen (reload right after moving a block + // and the block moves again in front of you). Waiting here is what makes the handshake below + // authoritative: the client's first sync IS the finished document, in one message. + await ensureRoomReady(name, entry, entry.workspaceId) + + // Re-check access immediately before registering, mirroring the workflow join: the + // access re-validation sweep records a revocation BEFORE it evicts, so a join that + // authorized just before the revocation must not complete afterwards and re-bind + // the socket to the document. This RE-RESOLVES rather than peeking the cache — a + // peek treats an expired entry as unknown and fails open, which a join stalled + // longer than the cache TTL would slip straight through. Normally a cache hit (this + // join's own authorize just warmed it), so it costs no extra query. + const currentPermission = await resolveCurrentRoomPermission(userId, room, FILE_DOC_ACTION) + if (!satisfiesRoomMembership(currentPermission, ROOM_TYPES.WORKSPACE_FILE_DOC)) { + logger.warn( + `User ${userId} lost write access to file ${fileId} before the join completed` + ) + emitJoinError(socket, fileId, 'Access denied to file', 'ACCESS_DENIED', false) return } - // Same user reclaiming its client id on a stale prior socket: evict just THAT clientID's binding - // + caret from the old socket. If that leaves the old socket with no providers, also drop its - // room mapping + Socket.IO membership so it can no longer send document (sync) frames - // (handleMessage's SYNC path gates on socketToRoomName, not owners); an old socket that still - // hosts OTHER providers keeps them. Done inline rather than via cleanupFileDocForSocket, which - // could destroyRoomIfIdle the room we're joining. - clientMap.delete(clientId) - awarenessProtocol.removeAwarenessStates(entry.awareness, [clientId], null) - if (clientMap.size === 0) { - entry.owners.delete(otherSid) - socketToRoomName.delete(otherSid) - io.in(otherSid).socketsLeave(name) - } - } - // Only now that the rebind is guaranteed to succeed, leave a previously-joined document if - // switching (a socket edits at most one). A duplicate join of the SAME room falls through - // and simply re-runs the sync handshake, idempotently. - const currentName = socketToRoomName.get(socket.id) - if (currentName && currentName !== name) { - socket.leave(currentName) - cleanupFileDocForSocket(socket.id, io) - } + // Abort a JOIN superseded while the room was being prepared: the socket disconnected, a newer + // JOIN (a document switch) bumped the generation, or the room was dropped and re-created. + // Registering here would leak a dead socket's room, bind the socket to the wrong document, or + // attach it to a doc no longer registered. Last await before the commit, so nothing can + // interleave between the access re-check above and the registration below. + if ( + socket.disconnected || + joinGeneration.get(socket.id) !== generation || + fileDocRooms.get(name) !== entry + ) + return - // ADD this provider's clientID to the socket's ownership set (do NOT overwrite a sibling provider - // on the same socket — that lone-owner overwrite is exactly what dropped the chat preview's - // awareness when the Files editor co-mounted). A re-JOIN of the same clientID is idempotent. A - // single provider that later unmounts clears its own caret via its awareness removal; the whole - // set is dropped on the socket's LEAVE/disconnect (client emits LEAVE only after its LAST provider - // for the file tears down). - let clientMap = entry.owners.get(socket.id) - if (clientMap === undefined) { - clientMap = new Map() - entry.owners.set(socket.id, clientMap) - } - clientMap.set(clientId, { clientId, userId, userName, avatarUrl }) - socketToRoomName.set(socket.id, name) - socket.join(name) + // A client id must be owned by at most one user, or a peer could bind an active + // collaborator's id and pass the per-frame ownership check to spoof/clear its caret. + // Distinguish a reconnect from a spoof by the owning user: the same user reclaiming its + // own client id (a dropped socket reconnecting reuses the Yjs client id, and its prior + // socket may not be cleaned up yet) takes over the stale binding; a DIFFERENT user is + // rejected. This runs BEFORE any teardown of the socket's current binding below, so a + // rejected rebind — even during a document switch — leaves the socket's existing document + // and caret untouched. + for (const [otherSid, clientMap] of entry.owners) { + if (otherSid === socket.id) continue + const owner = clientMap.get(clientId) + if (owner === undefined) continue + if (owner.userId !== userId) { + emitJoinError(socket, fileId, 'Client id already in use', 'CLIENT_ID_IN_USE', false) + return + } + // Same user reclaiming its client id on a stale prior socket: evict just THAT clientID's + // binding + caret from the old socket. If that leaves the old socket with no providers, also + // drop its room mapping + Socket.IO membership so it can no longer send document (sync) frames + // (handleMessage's SYNC path gates on socketToRoomName, not owners); an old socket that still + // hosts OTHER providers keeps them. Done inline rather than via cleanupFileDocForSocket, which + // could destroyRoomIfIdle the room we're joining. + clientMap.delete(clientId) + awarenessProtocol.removeAwarenessStates(entry.awareness, [clientId], null) + if (clientMap.size === 0) { + entry.owners.delete(otherSid) + socketToRoomName.delete(otherSid) + io.in(otherSid).socketsLeave(name) + } + } - // Capture what the server-side persist needs: the workspace to write back to, and the current - // user for attribution (refreshed to the actual editor on each edit in `handleMessage`). - if (authorized.workspaceId) entry.workspaceId = authorized.workspaceId - entry.lastEditorUserId = userId - - socket.emit(FILE_DOC_EVENTS.JOIN_SUCCESS, { fileId }) - // Server-authenticated roster → everyone in the room, including this joiner. - broadcastFileDocPresence(io, name, entry) - - // Begin the sync handshake: send the server's state (sync step 1). The - // client replies with its updates and requests the server's in return. - const syncEncoder = encoding.createEncoder() - encoding.writeVarUint(syncEncoder, FILE_DOC_MESSAGE_TYPE.SYNC) - syncProtocol.writeSyncStep1(syncEncoder, entry.doc) - socket.emit(FILE_DOC_EVENTS.MESSAGE, encoding.toUint8Array(syncEncoder)) - - // Send existing awareness so the new client immediately sees others' carets. - const states = entry.awareness.getStates() - if (states.size > 0) { - const awarenessEncoder = encoding.createEncoder() - encoding.writeVarUint(awarenessEncoder, FILE_DOC_MESSAGE_TYPE.AWARENESS) - encoding.writeVarUint8Array( - awarenessEncoder, - awarenessProtocol.encodeAwarenessUpdate(entry.awareness, Array.from(states.keys())) - ) - socket.emit(FILE_DOC_EVENTS.MESSAGE, encoding.toUint8Array(awarenessEncoder)) - } + // Only now that the rebind is guaranteed to succeed, leave a previously-joined document if + // switching (a socket edits at most one). A duplicate join of the SAME room falls through + // and simply re-runs the sync handshake, idempotently. + const currentName = socketToRoomName.get(socket.id) + if (currentName && currentName !== name) { + socket.leave(currentName) + cleanupFileDocForSocket(socket.id, io) + } - // Seed the document server-side (once). Fire-and-forget: the join completes immediately and - // the seed relays to this socket via `doc.on('update')` the moment it lands. - if (authorized.workspaceId) void ensureServerSeed(name, entry, authorized.workspaceId) + // ADD this provider's clientID to the socket's ownership set (do NOT overwrite a sibling + // provider on the same socket — that lone-owner overwrite is exactly what dropped the chat + // preview's awareness when the Files editor co-mounted). A re-JOIN of the same clientID is + // idempotent. A single provider that later unmounts clears its own caret via its awareness + // removal; the whole set is dropped on the socket's LEAVE/disconnect (the client emits LEAVE + // only after its LAST provider for the file tears down). + let clientMap = entry.owners.get(socket.id) + if (clientMap === undefined) { + clientMap = new Map() + entry.owners.set(socket.id, clientMap) + } + clientMap.set(clientId, { clientId, userId, userName, avatarUrl }) + socketToRoomName.set(socket.id, name) + socket.join(name) + + // Attribution for the server-side persist, refreshed to the actual editor on each edit in + // `handleMessage`. + entry.lastEditorUserId = userId + + // Name the document this room holds, so a client that still carries a DIFFERENT one (its room + // outlived by a document rebuilt in its place) can refuse to merge instead of unioning two + // documents into the file twice over. Read after readiness — before it, the room has no doc yet. + socket.emit(FILE_DOC_EVENTS.JOIN_SUCCESS, { fileId, docId: docIdOf(entry.doc) }) + // Server-authenticated roster → everyone in the room, including this joiner. + broadcastFileDocPresence(io, name, entry) + + // Begin the sync handshake: send the server's state (sync step 1). The + // client replies with its updates and requests the server's in return. + const syncEncoder = encoding.createEncoder() + encoding.writeVarUint(syncEncoder, FILE_DOC_MESSAGE_TYPE.SYNC) + syncProtocol.writeSyncStep1(syncEncoder, entry.doc) + socket.emit(FILE_DOC_EVENTS.MESSAGE, encoding.toUint8Array(syncEncoder)) + + // Send existing awareness so the new client immediately sees others' carets. + const states = entry.awareness.getStates() + if (states.size > 0) { + const awarenessEncoder = encoding.createEncoder() + encoding.writeVarUint(awarenessEncoder, FILE_DOC_MESSAGE_TYPE.AWARENESS) + encoding.writeVarUint8Array( + awarenessEncoder, + awarenessProtocol.encodeAwarenessUpdate(entry.awareness, Array.from(states.keys())) + ) + socket.emit(FILE_DOC_EVENTS.MESSAGE, encoding.toUint8Array(awarenessEncoder)) + } - logger.info(`User ${userId} joined file-doc room ${fileId}`) + logger.info(`User ${userId} joined file-doc room ${fileId}`) + } finally { + entry.pendingJoins -= 1 + // A join that returned without registering may have left behind the room it created; drop it + // if nothing else claimed it. A no-op once this join committed (the room then has an owner). + destroyRoomIfIdle(name) + } } catch (error) { logger.error('Error joining file-doc room:', error) try { diff --git a/apps/sim/app/api/workspaces/[id]/files/inline/route.test.ts b/apps/sim/app/api/workspaces/[id]/files/inline/route.test.ts index 495044427bb..793f3f9f245 100644 --- a/apps/sim/app/api/workspaces/[id]/files/inline/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/files/inline/route.test.ts @@ -30,6 +30,7 @@ describe('GET /api/workspaces/[id]/files/inline', () => { mockReadInline.mockResolvedValue({ file: { name: 'photo.png', type: 'image/png', size: PNG.length }, stream: new Blob([new Uint8Array(PNG)]).stream(), + addressedBy: 'fileId', }) }) @@ -43,6 +44,7 @@ describe('GET /api/workspaces/[id]/files/inline', () => { input: { workspaceId: 'ws-1', fileId: 'wf_abc' }, }) ) + // A file id names the FILE, whose bytes move under it on every edit — so it must revalidate. expect(res.headers.get('Cache-Control')).toBe('private, no-cache, must-revalidate') expect(res.headers.get('Content-Disposition')).toBe('inline; filename="photo.png"') }) @@ -58,6 +60,24 @@ describe('GET /api/workspaces/[id]/files/inline', () => { }) }) + /** + * A storage key names one object and a content write never rewrites one, so these bytes can never + * change. Revalidating them meant re-downloading every embedded image on every open — a document is + * rendered by two editors (the read-only placeholder, then the live one) and each renders the image + * twice, so the image was fetched again on every one of those passes. + */ + it('lets the browser keep an image addressed by storage key', async () => { + mockReadInline.mockResolvedValue({ + file: { name: 'photo.png', type: 'image/png', size: PNG.length }, + stream: new Blob([new Uint8Array(PNG)]).stream(), + addressedBy: 'key', + }) + + const res = await GET(req('key=workspace%2Fws-1%2Fphoto.png'), params) + + expect(res.headers.get('Cache-Control')).toBe('private, max-age=31536000, immutable') + }) + it('returns the concealed 404 response for an unauthorized or missing file', async () => { mockReadInline.mockRejectedValue( new OrchestrationError('forbidden', 'Insufficient permissions') diff --git a/apps/sim/app/api/workspaces/[id]/files/inline/route.ts b/apps/sim/app/api/workspaces/[id]/files/inline/route.ts index ad3780eb4be..2ede7737d24 100644 --- a/apps/sim/app/api/workspaces/[id]/files/inline/route.ts +++ b/apps/sim/app/api/workspaces/[id]/files/inline/route.ts @@ -10,6 +10,25 @@ import { encodeFilenameForHeader, getSecureFileHeaders } from '@/app/api/files/u export const dynamic = 'force-dynamic' +/** + * How long the browser may reuse an embedded image, by how the request addressed it (see + * {@link ReadWorkspaceInlineFileResult.addressedBy}). + * + * A `key` names one storage object and a content write never rewrites one, so those bytes are + * immutable and the browser needs no round trip — which is the difference between an embedded image + * reappearing instantly and it being downloaded again. Every document render asks for the same image + * at least twice (ProseMirror's own DOM, then the React node view) and every editor mounts twice (the + * read-only placeholder, then the live editor), so revalidating each time meant re-fetching the whole + * image on every open and reload — measured at ~1 MB per open on a real document, with the image area + * blank until it landed. `private` keeps it out of shared caches: the bytes are authorized per user. + * + * A `fileId` names the FILE, and its bytes move under it on every edit, so that form keeps revalidating. + */ +const INLINE_CACHE_CONTROL = { + key: 'private, max-age=31536000, immutable', + fileId: 'private, no-cache, must-revalidate', +} as const + /** * GET /api/workspaces/[id]/files/inline?key=|fileId= * @@ -29,12 +48,12 @@ export const GET = defineInternalBinaryRoute({ fileId: query.fileId, }), useCase: readWorkspaceInlineFile, - present: ({ file, stream }) => { + present: ({ file, stream, addressedBy }) => { const secure = getSecureFileHeaders(file.name, file.type) const headers = new Headers({ 'Content-Type': secure.contentType, 'Content-Disposition': `${secure.disposition}; ${encodeFilenameForHeader(file.name)}`, - 'Cache-Control': 'private, no-cache, must-revalidate', + 'Cache-Control': INLINE_CACHE_CONTROL[addressedBy], 'X-Content-Type-Options': 'nosniff', }) if (secure.contentType === 'image/svg+xml') { diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.test.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.test.ts index 689c2ee6c77..baa4d97b592 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.test.ts @@ -98,6 +98,50 @@ describe('FileDocProvider', () => { expect(emittedMessages(emit)).toHaveLength(0) }) + /** + * A tab that outlived its room can be offered a DIFFERENT document for the same file. Yjs would union + * the two — the file twice, on both sides, and the relay persists it — and there is no un-merge. So + * the sync must not happen at all; the fatal path leaves the editor read-only on what it already + * shows, and a reload binds a fresh document. + */ + it('refuses to sync into a document it does not recognize', () => { + const { provider, doc, emit, fire } = createProvider(true) + doc.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.docIdKey, 'doc-original') + const joinError = vi.fn() + provider.on('join-error', joinError) + emit.mockClear() + + fire(FILE_DOC_EVENTS.JOIN_SUCCESS, { fileId: 'file-1', docId: 'doc-rebuilt' }) + + expect(emittedMessages(emit)).toHaveLength(0) + expect(provider.synced).toBe(false) + expect(provider.joinError).toMatchObject({ code: 'DOCUMENT_REPLACED', retryable: false }) + expect(joinError).toHaveBeenCalledTimes(1) + }) + + it('syncs when the room holds the document it already has', () => { + const { doc, emit, fire } = createProvider(true) + doc.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.docIdKey, 'doc-original') + emit.mockClear() + + fire(FILE_DOC_EVENTS.JOIN_SUCCESS, { fileId: 'file-1', docId: 'doc-original' }) + + expect(emittedMessages(emit).length).toBeGreaterThan(0) + }) + + it('syncs when either side carries no identity (a fresh doc, or a room seeded before identities)', () => { + const fresh = createProvider(true) + fresh.emit.mockClear() + fresh.fire(FILE_DOC_EVENTS.JOIN_SUCCESS, { fileId: 'file-1', docId: 'doc-rebuilt' }) + expect(emittedMessages(fresh.emit).length).toBeGreaterThan(0) + + const unnamedRoom = createProvider(true) + unnamedRoom.doc.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.docIdKey, 'doc-original') + unnamedRoom.emit.mockClear() + unnamedRoom.fire(FILE_DOC_EVENTS.JOIN_SUCCESS, { fileId: 'file-1' }) + expect(emittedMessages(unnamedRoom.emit).length).toBeGreaterThan(0) + }) + it('applies a server sync step 2 and becomes synced', () => { const { provider, doc, fire } = createProvider(true) const synced = vi.fn() diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.ts index 84bab0733bc..79884c1e81c 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.ts @@ -174,19 +174,11 @@ export class FileDocProvider extends ObservableV2 { */ private handleReadinessDeadline = () => { this.readinessTimer = null - if ((this.synced && this.isSeeded()) || this.fatal || this.disposed) return - const error: JoinFileDocError = { - fileId: this.fileId, - error: 'Realtime document was not ready in time', - code: 'READINESS_TIMEOUT', - retryable: false, - } - this.fatal = true - this.joinError = error - // Drop `synced` so the editor's `synced && seeded` gate stays closed → the fallback renders the - // stored content read-only rather than becoming editable on a doc the server never seeded. - this.setSynced(false) - this.emit('join-error', [error]) + if (this.synced && this.isSeeded()) return + // Dropping `synced` (see {@link failFatally}) is what keeps the editor's `synced && seeded` gate + // closed, so the fallback renders the stored content read-only rather than becoming editable on a + // document the server never seeded. + this.failFatally('Realtime document was not ready in time', 'READINESS_TIMEOUT') } private clearReadinessTimer() { @@ -215,13 +207,55 @@ export class FileDocProvider extends ObservableV2 { /** * Handle the join ack. The server registers the room before acking, so an earlier * send could be dropped — the initial sync + local awareness exchange begins here. + * + * Unless the room holds a DIFFERENT document than ours. Two documents built from the same markdown + * are not the same document to Yjs — their items carry different client ids — so syncing one into the + * other appends the file to itself, on both sides, and the server persists the result. A document is + * rebuilt only when the room AND the shared stream are both gone (a tab that slept through it), which + * is precisely when a stale tab reconnects. There is no way to un-merge afterwards, so the sync never + * happens: take the fatal path, which leaves the editor read-only on the content it already shows. + * A reload binds a fresh document and recovers. */ private handleJoinSuccess = (data: JoinFileDocSuccess) => { if (data.fileId !== this.fileId) return + const local = this.docId() + if (local !== undefined && data.docId !== undefined && data.docId !== local) { + this.failFatally( + 'This document was reloaded on the server; refresh to continue editing', + 'DOCUMENT_REPLACED' + ) + return + } this.sendSyncStep1() this.sendLocalAwareness() } + /** The identity of the document we hold, once the server seed has named one. */ + private docId(): string | undefined { + const docId = this.doc.getMap(FILE_DOC_SEED.configMap).get(FILE_DOC_SEED.docIdKey) + return typeof docId === 'string' ? docId : undefined + } + + /** + * Give up on this document, non-retryably: latch fatal so nothing more is applied or relayed, drop + * `synced` so the editor's gate closes, and surface the rejection to the owner (which falls back to a + * read-only view of the stored content). + */ + private failFatally(message: string, code: string) { + if (this.fatal || this.disposed) return + const error: JoinFileDocError = { + fileId: this.fileId, + error: message, + code, + retryable: false, + } + this.fatal = true + this.joinError = error + this.clearReadinessTimer() + this.setSynced(false) + this.emit('join-error', [error]) + } + /** * Handle a join rejection. A non-retryable rejection (access denied, invalid) * won't succeed on retry, so latch {@link fatal} to stop (re)joining and let the @@ -247,18 +281,7 @@ export class FileDocProvider extends ObservableV2 { */ private handleAccessRevoked = (data: RoomAccessRevokedBroadcast) => { if (data.room?.type !== ROOM_TYPES.WORKSPACE_FILE_DOC || data.room.id !== this.fileId) return - if (this.fatal || this.disposed) return - const error: JoinFileDocError = { - fileId: this.fileId, - error: data.message, - code: 'ACCESS_REVOKED', - retryable: false, - } - this.fatal = true - this.joinError = error - this.clearReadinessTimer() - this.setSynced(false) - this.emit('join-error', [error]) + this.failFatally(data.message, 'ACCESS_REVOKED') } private handleMessage = (data: unknown) => { diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx index 0eb5f3d72c0..0a53b21cce5 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx @@ -722,38 +722,15 @@ export function LoadedRichMarkdownEditor({ */ useEffect(() => { /** - * Reveal the live document only once it has stopped moving. - * - * `synced` fires when the sync handshake completes, but the room's remaining updates can still be - * in flight — reconnecting to a room that was edited moments ago, the handshake lands on the base - * state and the edit arrives a few milliseconds later. Flipping readiness on `synced` alone - * therefore un-hides the live editor onto an INTERMEDIATE state and lets the document correct - * itself in view: measured on a reload right after a block move, the editor was revealed showing - * the pre-move order and re-ordered 7ms later. The placeholder was showing the right thing the - * whole time, so the swap is the only reason anything moved. - * - * So hold the reveal until one animation frame passes with no update to the shared document; any - * update restarts the wait. Deferred readiness costs at most a frame of the placeholder — which is - * already rendering the correct content — while going NOT-ready stays immediate, so nothing can - * keep a fatal or unsynced document editable a moment longer than before. + * Readiness is a protocol fact, never a timing guess: the relay attaches a client only once its + * room holds the whole document (it awaits the shared-stream catch-up and the server seed before + * answering a join), so a completed sync IS the finished document and revealing on it cannot show + * an intermediate state. This deliberately does NOT wait for the document to "stop moving" — a + * quiet-frame gate was tried and it is unsound in both directions: it delays the reveal of a + * document that was already correct, and it opens mid-flight anyway whenever the updates arrive + * more than a frame apart (which is what a remote Redis and a long room history produce). */ - let settleFrame: number | null = null - const cancelSettle = () => { - if (settleFrame !== null) cancelAnimationFrame(settleFrame) - settleFrame = null - } const setReady = (ready: boolean) => { - cancelSettle() - if (ready) { - settleFrame = requestAnimationFrame(() => { - settleFrame = null - applyReady(true) - }) - return - } - applyReady(false) - } - const applyReady = (ready: boolean) => { // Child-local: gates editability (a user must never type into an unsynced/unseeded doc). setCollabReady(ready) // Parent: gates CLIENT autosave. In a collaborative session the relay persists the doc to @@ -762,16 +739,14 @@ export function LoadedRichMarkdownEditor({ // Only the non-collaborative (solo) path client-autosaves. onCollabReadyChange(collaboration ? false : ready) } - // No shared document to settle on the solo path, so apply directly — deferring a frame would only - // delay autosave, and this branch returns before the cleanup that cancels a pending frame. if (!collaboration) { - applyReady(true) + setReady(true) return } const { provider, doc } = collaboration if (!editor) { setReady(false) - return cancelSettle + return } const config = doc.getMap(FILE_DOC_SEED.configMap) @@ -835,31 +810,18 @@ export function LoadedRichMarkdownEditor({ } } - /** An update while the reveal is pending means the document is still catching up — wait again. */ - const restartSettle = () => { - if (settleFrame === null) return - cancelSettle() - settleFrame = requestAnimationFrame(() => { - settleFrame = null - applyReady(true) - }) - } - provider.on('synced', report) provider.on('join-error', onJoinError) config.observe(report) config.observe(syncFrontmatter) - doc.on('update', restartSettle) report() if (provider.joinError) onJoinError(provider.joinError) return () => { - cancelSettle() provider.off('synced', report) provider.off('join-error', onJoinError) config.unobserve(report) config.unobserve(syncFrontmatter) - doc.off('update', restartSettle) // Report NOT ready on teardown — the safe direction. If this effect ever re-runs while mounted // (a future dep change), briefly gating autosave off is harmless; reporting `true` here could // ungate it while the doc is unready. diff --git a/apps/sim/hooks/queries/workspace-files.test.tsx b/apps/sim/hooks/queries/workspace-files.test.tsx index d6e5a765774..6dea419e9a4 100644 --- a/apps/sim/hooks/queries/workspace-files.test.tsx +++ b/apps/sim/hooks/queries/workspace-files.test.tsx @@ -16,6 +16,8 @@ import { createWorkspaceFileContract } from '@/lib/api/contracts/workspace-files import { useCreateWorkspaceFile, useWorkspaceFileContent, + useWorkspaceFiles, + type WorkspaceFileContentResult, workspaceFilesKeys, } from '@/hooks/queries/workspace-files' @@ -205,19 +207,20 @@ describe('useWorkspaceFileContent stale storage key', () => { }) ) const { queryClient, unmount } = renderContentHook() - const invalidateQueries = vi.spyOn(queryClient, 'invalidateQueries') + const refetchQueries = vi.spyOn(queryClient, 'refetchQueries') await act(async () => { await sleep(50) }) expect(fetchCount).toBe(1) - expect(invalidateQueries).toHaveBeenCalledWith({ - queryKey: workspaceFilesKeys.workspaceLists('ws-1'), - }) + expect(refetchQueries).toHaveBeenCalledWith( + { queryKey: workspaceFilesKeys.workspaceLists('ws-1') }, + { cancelRefetch: true } + ) // The RECORD is re-resolved, never this query — re-driving the read against the same dead key // is what would spin. - expect(invalidateQueries).toHaveBeenCalledTimes(1) + expect(refetchQueries).toHaveBeenCalledTimes(1) unmount() }) @@ -230,14 +233,14 @@ describe('useWorkspaceFileContent stale storage key', () => { }) ) const { queryClient, unmount } = renderContentHook() - const invalidateQueries = vi.spyOn(queryClient, 'invalidateQueries') + const refetchQueries = vi.spyOn(queryClient, 'refetchQueries') await act(async () => { await sleep(50) }) expect(fetchCount).toBe(1) - expect(invalidateQueries).not.toHaveBeenCalled() + expect(refetchQueries).not.toHaveBeenCalled() unmount() }) }) @@ -272,3 +275,115 @@ describe('useCreateWorkspaceFile', () => { unmount() }) }) + +/** + * The rotation costs one request; it must not cost a lie. Between the 404 and the record re-resolving, + * the surface is mid-recovery — reporting a failure there paints "Failed to load file content" over a + * document that lands a few hundred milliseconds later, gone before the reader can act on it. + */ +describe('useWorkspaceFileContent while a superseded key is being re-resolved', () => { + /** Mounts the content read alongside the record query the recovery re-resolves, so the second list + * fetch can be held open and the recovery window observed from the outside. */ + function renderDuringRecovery(): { + getResult: () => WorkspaceFileContentResult + resolveRecord: () => void + unmount: () => void + } { + let releaseRecord: () => void = () => {} + let call = 0 + mockRequestJson.mockImplementation(async () => { + // First call is the initial record load; the one the 404 triggers is held open. + if (++call > 1) await new Promise((resolve) => (releaseRecord = resolve)) + return { success: true, files: [] } + }) + + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }) + const root: Root = createRoot(document.createElement('div')) + let result: WorkspaceFileContentResult | undefined + + function Probe() { + useWorkspaceFiles('ws-1') + result = useWorkspaceFileContent('ws-1', 'file-1', 'workspace/ws-1/123-abc-doc.md', false) + return null + } + + act(() => { + root.render( + + + + ) + }) + + return { + getResult: () => { + if (!result) throw new Error('Content hook did not render') + return result + }, + resolveRecord: () => releaseRecord(), + unmount: () => { + act(() => root.unmount()) + queryClient.clear() + }, + } + } + + function serve404() { + vi.stubGlobal( + 'fetch', + vi.fn(async () => { + fetchCount += 1 + return new Response('{"error":"FileNotFoundError"}', { status: 404 }) + }) + ) + } + + it('reports still-loading, not failed, while the record is being re-resolved', async () => { + serve404() + const { getResult, resolveRecord, unmount } = renderDuringRecovery() + await act(async () => { + await sleep(50) + }) + + expect(getResult().error).toBeNull() + expect(getResult().isLoading).toBe(true) + + resolveRecord() + unmount() + }) + + it('surfaces the failure once the record comes back unchanged — the object is really gone', async () => { + serve404() + const { getResult, resolveRecord, unmount } = renderDuringRecovery() + await act(async () => { + await sleep(50) + }) + + await act(async () => { + resolveRecord() + await sleep(50) + }) + + expect(getResult().error).not.toBeNull() + expect(getResult().isLoading).toBe(false) + unmount() + }) + + it('never masks a failure that is not a superseded key', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(async () => { + fetchCount += 1 + return new Response('nope', { status: 500 }) + }) + ) + const { getResult, resolveRecord, unmount } = renderDuringRecovery() + await act(async () => { + await sleep(50) + }) + + expect(getResult().error).not.toBeNull() + resolveRecord() + unmount() + }) +}) diff --git a/apps/sim/hooks/queries/workspace-files.ts b/apps/sim/hooks/queries/workspace-files.ts index cc3092b5e70..a236a623f2f 100644 --- a/apps/sim/hooks/queries/workspace-files.ts +++ b/apps/sim/hooks/queries/workspace-files.ts @@ -3,7 +3,13 @@ import { toast } from '@sim/emcn' import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' import { backoffWithJitter } from '@sim/utils/retry' -import { keepPreviousData, useMutation, useQuery, useQueryClient } from '@tanstack/react-query' +import { + keepPreviousData, + useIsFetching, + useMutation, + useQuery, + useQueryClient, +} from '@tanstack/react-query' import { isApiClientError } from '@/lib/api/client/errors' import { requestJson } from '@/lib/api/client/request' import { fileStorageStatusContract } from '@/lib/api/contracts/storage-transfer' @@ -227,19 +233,33 @@ class StaleStorageKeyError extends Error { * over without a single client-visible event. Recovering on the 404 makes the rotation cost exactly * one request instead of stranding the reader on a dead key until a full reload. * - * Invalidates the RECORD, never the failed query: the refetched list either hands back a new key — - * which re-keys the read onto a fresh cache entry that fetches once — or the same one, in which case - * nothing refetches and the failure stands. That asymmetry is what makes this loop-proof, and it is - * why a genuinely deleted file settles instead of retrying: the list simply stops containing it. + * Re-reads the RECORD, never the failed query: the refetched list either hands back a new key — which + * re-keys the read onto a fresh cache entry that fetches once — or the same one, in which case nothing + * refetches and the failure stands. That asymmetry is what makes this loop-proof, and it is why a + * genuinely deleted file settles instead of retrying: the list simply stops containing it. + * + * Both details below exist because this runs from inside the failing read's own `queryFn`, and each was + * measured: without them the recovery is requested and no fetch happens at all, so the reader is left + * on the dead key showing a failure until something unrelated (a window focus, another consumer) + * happens to re-resolve the record. */ function useStaleKeyRecovery(workspaceId: string): (error: unknown) => void { const queryClient = useQueryClient() return useCallback( (error: unknown) => { if (!workspaceId || !(error instanceof StaleStorageKeyError)) return - void queryClient.invalidateQueries({ - queryKey: workspaceFilesKeys.workspaceLists(workspaceId), - }) + // Off this fetch's own cycle (one microtask): a refetch asked for from inside a `queryFn` — where + // this catch sits — is dropped by react-query, silently. This is what turned "one extra request" + // into "no recovery at all". + // + // `cancelRefetch` because a re-resolution has to OBSERVE the rotation: a record read already in + // flight was started before it, so it can only hand back the key we already know is dead. + void Promise.resolve().then(() => + queryClient.refetchQueries( + { queryKey: workspaceFilesKeys.workspaceLists(workspaceId) }, + { cancelRefetch: true } + ) + ) }, [queryClient, workspaceId] ) @@ -287,10 +307,10 @@ export function useWorkspaceFileContent( refetchInterval?: number | false | (() => number | false) refetchOnWindowFocus?: boolean } -) { +): WorkspaceFileContentResult { const source = useFileContentSource() const recoverStaleKey = useStaleKeyRecovery(workspaceId) - return useQuery({ + const query = useQuery({ queryKey: workspaceFilesKeys.content(workspaceId, fileId, raw ? 'raw' : 'text', key), queryFn: async ({ signal }) => { try { @@ -305,6 +325,46 @@ export function useWorkspaceFileContent( refetchOnWindowFocus: options?.refetchOnWindowFocus === false ? false : 'always', refetchInterval: options?.refetchInterval ?? false, }) + return { + data: query.data, + ...useStaleKeyRecoveryState(workspaceId, query.isLoading, query.error), + } +} + +export interface WorkspaceFileContentResult { + data: string | undefined + /** True while there is nothing to show yet — including the re-resolution window below. */ + isLoading: boolean + /** The failure worth showing the reader, or `null` while the address is still being re-resolved. */ + error: Error | null +} + +/** + * Present a superseded storage key as STILL LOADING rather than as a failure. + * + * A stale key is not a dead end and is not the reader's problem: the object moved, and the 404 has + * already triggered {@link useStaleKeyRecovery}, which re-resolves the record — the moment a new key + * arrives the read is re-keyed onto a fresh cache entry and fetches again. Painting "Failed to load + * file content" in that window reports a failure the surface is actively recovering from, and the + * content lands a few hundred milliseconds later, so the message is gone before it can be acted on. + * + * The window is bounded by a FACT, never a timer: the re-resolution is in flight. If the refetched + * record hands back the same key — the object is genuinely gone, not moved — the recovery ends, the + * error surfaces, and the reader sees a real failure. + */ +function useStaleKeyRecoveryState( + workspaceId: string, + isLoading: boolean, + error: unknown +): { isLoading: boolean; error: Error | null } { + const resolvingRecord = useIsFetching({ + queryKey: workspaceFilesKeys.workspaceLists(workspaceId), + }) + const recovering = error instanceof StaleStorageKeyError && resolvingRecord > 0 + return { + isLoading: isLoading || recovering, + error: recovering ? null : ((error as Error) ?? null), + } } /** diff --git a/apps/sim/lib/collab-doc/collab-state.ts b/apps/sim/lib/collab-doc/collab-state.ts index 97828bece51..115ab356191 100644 --- a/apps/sim/lib/collab-doc/collab-state.ts +++ b/apps/sim/lib/collab-doc/collab-state.ts @@ -14,17 +14,24 @@ export function hashMarkdown(markdown: Buffer): string { return createHash('sha256').update(markdown).digest('hex') } +/** A file's stored collaborative document, with the markdown it was last derived from. */ +export interface CachedCollabDocState { + docState: Uint8Array + /** Hash of the markdown this binary projects to — `null`s out nothing; compare to decide freshness. */ + sourceHash: string +} + /** - * Load a file's cached Yjs binary IF it is still fresh — i.e. was derived from markdown whose hash - * matches `sourceHash` (the current file's markdown). Returns the binary to apply directly, or `null` - * when there is no cache or it is stale (the markdown changed externally since it was saved), in which - * case the caller re-converts from markdown. Applying the stored binary — rather than rebuilding a Y.Doc - * from markdown — is what preserves the CRDT's client ids and prevents duplicated content on reconnect. + * Load a file's stored Yjs binary, fresh or not. + * + * FRESH (its `sourceHash` matches the file's current markdown) means it can seed a room verbatim. + * STALE means the markdown moved on out-of-band, and the caller must bring it up to date — but it must + * do so by UPDATING this document, never by building a second one: the stored binary carries the + * document's identity, and a rebuilt document's items carry different client ids, so any client still + * holding the old one would merge the two into duplicated content. Either way this row is the file's + * collaborative document; there is only ever one. */ -export async function loadFreshCollabDocState( - fileId: string, - sourceHash: string -): Promise { +export async function loadCollabDocState(fileId: string): Promise { const [row] = await db .select({ docState: workspaceFileCollabState.docState, @@ -34,8 +41,22 @@ export async function loadFreshCollabDocState( .where(eq(workspaceFileCollabState.fileId, fileId)) .limit(1) - if (!row || row.sourceHash !== sourceHash) return null - return new Uint8Array(row.docState) + if (!row) return null + return { docState: new Uint8Array(row.docState), sourceHash: row.sourceHash } +} + +/** + * The markdown hash this file's cached doc state was derived from — i.e. the exact bytes the live + * document last projected onto the file — or `null` when nothing is cached. Selects only the tag, so a + * caller asking "is what's on disk still our own last write?" never loads the binary to find out. + */ +export async function collabDocStateSourceHash(fileId: string): Promise { + const [row] = await db + .select({ sourceHash: workspaceFileCollabState.sourceHash }) + .from(workspaceFileCollabState) + .where(eq(workspaceFileCollabState.fileId, fileId)) + .limit(1) + return row?.sourceHash ?? null } /** diff --git a/apps/sim/lib/collab-doc/persist.test.ts b/apps/sim/lib/collab-doc/persist.test.ts index 38cbe9ae6a8..96f1f4af931 100644 --- a/apps/sim/lib/collab-doc/persist.test.ts +++ b/apps/sim/lib/collab-doc/persist.test.ts @@ -4,25 +4,33 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import * as Y from 'yjs' -const { mockGetWorkspaceFile, mockFetchBuffer, mockUpdateContent, mockSaveState } = vi.hoisted( - () => ({ - mockGetWorkspaceFile: vi.fn(), - mockFetchBuffer: vi.fn(), - mockUpdateContent: vi.fn(), - mockSaveState: vi.fn(), - }) -) +const { + mockGetWorkspaceFile, + mockFetchBuffer, + mockUpdateContent, + mockSaveState, + mockStateSourceHash, + ContentVersionConflictError, +} = vi.hoisted(() => ({ + mockGetWorkspaceFile: vi.fn(), + mockFetchBuffer: vi.fn(), + mockUpdateContent: vi.fn(), + mockSaveState: vi.fn(), + mockStateSourceHash: vi.fn(), + ContentVersionConflictError: class ContentVersionConflictError extends Error {}, +})) vi.mock('@/lib/uploads/contexts/workspace', () => ({ - ContentVersionConflictError: class ContentVersionConflictError extends Error {}, + ContentVersionConflictError, getWorkspaceFile: mockGetWorkspaceFile, fetchWorkspaceFileBuffer: mockFetchBuffer, updateWorkspaceFileContent: mockUpdateContent, })) vi.mock('./collab-state', () => ({ - hashMarkdown: () => 'test-source-hash', + hashMarkdown: (buffer: Buffer) => `hash:${buffer.toString('utf-8')}`, saveCollabDocState: mockSaveState, + collabDocStateSourceHash: mockStateSourceHash, })) import { markdownToYDoc, yDocToFileMarkdown } from './converter' @@ -53,6 +61,7 @@ describe('persistFileDoc — no-op writes', () => { beforeEach(() => { vi.clearAllMocks() mockSaveState.mockResolvedValue(undefined) + mockStateSourceHash.mockResolvedValue(null) }) function stubFile(durable: Buffer) { @@ -105,7 +114,11 @@ describe('persistFileDoc — no-op writes', () => { await persistFileDoc('ws-1', 'file-1', 'user-1', stateOf(md), VERSION.getTime()) - expect(mockSaveState).toHaveBeenCalledWith('file-1', expect.anything(), 'test-source-hash') + expect(mockSaveState).toHaveBeenCalledWith( + 'file-1', + expect.anything(), + `hash:${projectionOf(md).toString('utf-8')}` + ) }) it('writes when the content actually changed', async () => { @@ -168,3 +181,91 @@ describe('persistFileDoc — no-op writes', () => { expect(mockUpdateContent).toHaveBeenCalledTimes(1) }) }) + +/** + * The If-Match token is a REMEMBERED timestamp — held in the relay's room (lost with the room) and in a + * cluster key written best-effort — so a relay that exits just after a successful write comes back with + * a version older than the file's. Every persist then fails the CAS, and because a conflict neither + * writes nor advances the token, the document can never be persisted again: the durable markdown + * freezes, and every reload paints that stale markdown before the live document corrects it on screen. + */ +describe('persistFileDoc — a stale token is not an out-of-band write', () => { + const NEWER = new Date(VERSION.getTime() + 60_000) + + beforeEach(() => { + vi.clearAllMocks() + mockSaveState.mockResolvedValue(undefined) + }) + + /** The file is at `NEWER` (durable = `durableMd`), while the caller still believes `VERSION`. */ + function stubConflict(durableMd: string) { + const durable = projectionOf(durableMd) + mockGetWorkspaceFile.mockResolvedValue({ + id: 'file-1', + name: 'note.md', + key: 'k', + // A different size than the projection under test, so the no-op compare never short-circuits. + size: durable.length + 999, + updatedAt: NEWER, + contentUpdatedAt: NEWER, + }) + mockFetchBuffer.mockResolvedValue(durable) + mockUpdateContent.mockImplementation(async (..._args: unknown[]) => { + const options = _args[5] as { expectedUpdatedAt?: Date } + if (options?.expectedUpdatedAt?.getTime() !== NEWER.getTime()) { + throw new ContentVersionConflictError('stale') + } + return { contentUpdatedAt: new Date(NEWER.getTime() + 1), updatedAt: NEWER } + }) + return durable + } + + it('writes anyway when the file still holds the bytes this document last projected', async () => { + const durable = stubConflict('a\n\nb') + // The cached doc state was tagged with exactly these bytes: nobody else has written since. + mockStateSourceHash.mockResolvedValue(`hash:${durable.toString('utf-8')}`) + + const result = await persistFileDoc( + 'ws-1', + 'file-1', + 'user-1', + stateOf('a\n\nb\n\nmoved'), + VERSION.getTime() + ) + + expect(result).toEqual({ status: 'persisted', version: NEWER.getTime() + 1 }) + // Once with the stale token (rejected), once with the file's real version. + expect(mockUpdateContent).toHaveBeenCalledTimes(2) + }) + + it('still refuses when the file holds someone else’s content', async () => { + stubConflict('a\n\nb') + mockStateSourceHash.mockResolvedValue('hash:something this document never wrote') + + const result = await persistFileDoc( + 'ws-1', + 'file-1', + 'user-1', + stateOf('a\n\nb\n\nmoved'), + VERSION.getTime() + ) + + expect(result).toEqual({ status: 'conflict' }) + expect(mockUpdateContent).toHaveBeenCalledTimes(1) + }) + + it('refuses when nothing was ever cached, so there is no proof of authorship', async () => { + stubConflict('a\n\nb') + mockStateSourceHash.mockResolvedValue(null) + + const result = await persistFileDoc( + 'ws-1', + 'file-1', + 'user-1', + stateOf('a\n\nb\n\nmoved'), + VERSION.getTime() + ) + + expect(result).toEqual({ status: 'conflict' }) + }) +}) diff --git a/apps/sim/lib/collab-doc/persist.ts b/apps/sim/lib/collab-doc/persist.ts index 64cb727c1d7..2e8e9692ff4 100644 --- a/apps/sim/lib/collab-doc/persist.ts +++ b/apps/sim/lib/collab-doc/persist.ts @@ -7,7 +7,7 @@ import { getWorkspaceFile, updateWorkspaceFileContent, } from '@/lib/uploads/contexts/workspace' -import { hashMarkdown, saveCollabDocState } from './collab-state' +import { collabDocStateSourceHash, hashMarkdown, saveCollabDocState } from './collab-state' import { canonicalizeYDoc, yDocToFileMarkdown } from './converter' const logger = createLogger('FileDocPersist') @@ -113,7 +113,7 @@ export async function persistFileDoc( } } - try { + const write = async (ifMatch: number): Promise => { const updated = await updateWorkspaceFileContent( workspaceId, fileId, @@ -124,7 +124,7 @@ export async function persistFileDoc( // This write IS the projection of the live doc, so re-merging it into that same doc would loop. syncLiveDoc: false, // If-Match: only if the durable file is still at the version the live doc synced from. - expectedUpdatedAt: expectedVersion !== undefined ? new Date(expectedVersion) : undefined, + expectedUpdatedAt: new Date(ifMatch), secretProvenancePolicy: { mode: 'preserve' }, } ) @@ -150,15 +150,60 @@ export async function persistFileDoc( status: 'persisted', version: (updated.contentUpdatedAt ?? updated.updatedAt).getTime(), } + } + + try { + return await write(expectedVersion) } catch (error) { if (!(error instanceof ContentVersionConflictError)) throw error - // Out-of-band content change since the live doc last synced — DON'T clobber. The relay leaves the - // durable content authoritative and does NOT re-persist or advance its synced version here (a later - // flush reconciles once the chokepoint merge lands), so it reads nothing off this result beyond the - // `conflict` status — no durable version re-read is needed. + return recoverFromVersionConflict(workspaceId, fileId, markdownBuffer, write) + } +} + +/** + * A stale If-Match does not prove someone else wrote the file — so ask the CONTENT, not the clock. + * + * The relay's token is a remembered timestamp: it lives in the room (lost when the room is dropped) and + * in a cluster key written best-effort, so a process that dies in the moments after a successful write + * comes back holding a version older than the file's. Every later persist then fails the CAS, and + * because a conflict deliberately neither writes nor advances the token, the room can never persist + * again: the session's edits stay in the stream, the durable markdown freezes at the last write, and + * every reload renders that stale markdown before the live document corrects it on screen. + * + * The guard exists to protect content the live document has never seen. The file's own bytes settle + * that directly: if they hash to what this document last projected ({@link collabDocStateSourceHash}, + * written with every successful persist), then nothing out-of-band exists and the write is safe — retry + * it once against the file's current version. If they hash to anything else, the change is real, the + * conflict stands, and the durable content stays authoritative exactly as before. + */ +async function recoverFromVersionConflict( + workspaceId: string, + fileId: string, + markdownBuffer: Buffer, + write: (ifMatch: number) => Promise +): Promise { + const conflict = (): PersistFileDocResult => { logger.warn( `Persist conflict for file ${fileId}; durable content changed out-of-band since sync` ) return { status: 'conflict' } } + try { + const current = await getWorkspaceFile(workspaceId, fileId, { throwOnError: true }) + if (!current) return { status: 'missing' } + const durable = await fetchWorkspaceFileBuffer(current) + if (hashMarkdown(durable) !== (await collabDocStateSourceHash(fileId))) return conflict() + logger.info( + `Persist token for file ${fileId} was stale, not the file; re-syncing and writing the projection` + ) + return await write((current.contentUpdatedAt ?? current.updatedAt).getTime()) + } catch (error) { + // Including a SECOND conflict: something wrote the file during the recovery, which is the very + // change the guard exists for. + if (error instanceof ContentVersionConflictError) return conflict() + logger.warn(`Persist conflict recovery failed for file ${fileId}`, { + error: getErrorMessage(error), + }) + return conflict() + } } diff --git a/apps/sim/lib/collab-doc/seed.test.ts b/apps/sim/lib/collab-doc/seed.test.ts index a690b95fb8c..29e83a2e2df 100644 --- a/apps/sim/lib/collab-doc/seed.test.ts +++ b/apps/sim/lib/collab-doc/seed.test.ts @@ -7,10 +7,11 @@ import { prosemirrorJSONToYDoc } from '@tiptap/y-tiptap' import { beforeEach, describe, expect, it, vi } from 'vitest' import * as Y from 'yjs' -const { mockGetWorkspaceFile, mockFetchBuffer, mockLoadFresh } = vi.hoisted(() => ({ +const { mockGetWorkspaceFile, mockFetchBuffer, mockLoadState, mockSaveState } = vi.hoisted(() => ({ mockGetWorkspaceFile: vi.fn(), mockFetchBuffer: vi.fn(), - mockLoadFresh: vi.fn(), + mockLoadState: vi.fn(), + mockSaveState: vi.fn(), })) vi.mock('@/lib/uploads/contexts/workspace', () => ({ @@ -22,7 +23,8 @@ vi.mock('@/lib/uploads/contexts/workspace', () => ({ // tests cover the markdown → Yjs conversion path (the cache-hit fast path has its own test below). vi.mock('./collab-state', () => ({ hashMarkdown: () => 'test-source-hash', - loadFreshCollabDocState: mockLoadFresh, + loadCollabDocState: mockLoadState, + saveCollabDocState: mockSaveState, })) import { createMarkdownContentExtensions } from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/extensions' @@ -44,8 +46,9 @@ describe('buildFileDocSeed', () => { context: 'workspace', updatedAt: new Date('2026-01-01T00:00:00.000Z'), }) - // Default: no cached binary → the conversion path runs (the case these tests cover). - mockLoadFresh.mockResolvedValue(null) + // Default: nothing stored → the conversion path runs (the case these tests cover). + mockLoadState.mockResolvedValue(null) + mockSaveState.mockResolvedValue(undefined) }) it('builds a seed whose applied update reproduces the file body (through the client engine)', async () => { @@ -68,7 +71,7 @@ describe('buildFileDocSeed', () => { cachedDoc.getText('marker').insert(0, 'cached') const cached = Y.encodeStateAsUpdate(cachedDoc) mockFetchBuffer.mockResolvedValue(Buffer.from('# Anything', 'utf-8')) - mockLoadFresh.mockResolvedValue(cached) + mockLoadState.mockResolvedValue({ docState: cached, sourceHash: 'test-source-hash' }) const seed = await buildFileDocSeed('ws-1', 'file-1') @@ -95,7 +98,7 @@ describe('buildFileDocSeed', () => { ) const cached = Y.encodeStateAsUpdate(stale) mockFetchBuffer.mockResolvedValue(Buffer.from('# T\n\nbody\n\n- a\n- b', 'utf-8')) - mockLoadFresh.mockResolvedValue(cached) + mockLoadState.mockResolvedValue({ docState: cached, sourceHash: 'test-source-hash' }) const seed = await buildFileDocSeed('ws-1', 'file-1') @@ -115,7 +118,7 @@ describe('buildFileDocSeed', () => { // The cache is a best-effort optimization over the durable markdown we already hold; a transient DB // error or a not-yet-migrated cache table must convert, not abort the seed. mockFetchBuffer.mockResolvedValue(Buffer.from('# Title\n\ntext.', 'utf-8')) - mockLoadFresh.mockRejectedValue(new Error('cache table missing')) + mockLoadState.mockRejectedValue(new Error('cache table missing')) const seed = await buildFileDocSeed('ws-1', 'file-1') expect(seed).not.toBeNull() @@ -168,3 +171,115 @@ describe('buildFileDocSeed', () => { expect(mockGetWorkspaceFile).toHaveBeenCalledWith('ws-1', 'file-1', { throwOnError: true }) }) }) + +/** + * One file, ONE collaborative document, for its whole life. + * + * Two documents built from the same markdown are not the same document to Yjs — their items carry + * different client ids — so a client still holding the first merges the two into the file twice over, + * and the relay persists that. The guard is never to build a second one: every load resumes the stored + * document, and a client checks the identity it is offered before it syncs. + */ +describe('buildFileDocSeed — document identity', () => { + beforeEach(() => { + vi.clearAllMocks() + mockGetWorkspaceFile.mockResolvedValue({ + id: 'file-1', + name: 'note.md', + key: 'k', + context: 'workspace', + updatedAt: new Date('2026-01-01T00:00:00.000Z'), + }) + mockLoadState.mockResolvedValue(null) + mockSaveState.mockResolvedValue(undefined) + }) + + const docIdOf = (update: Uint8Array): unknown => { + const doc = new Y.Doc() + try { + Y.applyUpdate(doc, update) + return doc.getMap(FILE_DOC_SEED.configMap).get(FILE_DOC_SEED.docIdKey) + } finally { + doc.destroy() + } + } + + it('stores the document it builds, so the next open resumes it rather than building another', async () => { + // Until this row exists, a file that is opened but never edited gets a NEW document on every open. + mockFetchBuffer.mockResolvedValue(Buffer.from('# Title\n\nbody', 'utf-8')) + + const seed = await buildFileDocSeed('ws-1', 'file-1') + + expect(mockSaveState).toHaveBeenCalledWith('file-1', seed!.update, 'test-source-hash') + expect(typeof docIdOf(seed!.update)).toBe('string') + }) + + it('keeps the stored document’s identity when the markdown changed out-of-band', async () => { + // A copilot write (or the content API) moved the markdown on, so the stored binary is stale. It must + // be UPDATED, not replaced: the identity — and the client ids under it — have to survive. + const stored = markdownToYDoc('# Title\n\nbody') + stored.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.docIdKey, 'doc-original') + mockLoadState.mockResolvedValue({ + docState: Y.encodeStateAsUpdate(stored), + sourceHash: 'a-hash-from-before-the-external-write', + }) + mockFetchBuffer.mockResolvedValue(Buffer.from('# Title\n\nbody\n\nadded externally', 'utf-8')) + + const seed = await buildFileDocSeed('ws-1', 'file-1') + + expect(docIdOf(seed!.update)).toBe('doc-original') + const doc = new Y.Doc() + Y.applyUpdate(doc, seed!.update) + expect(yDocToMarkdown(doc)).toBe(serializeMarkdownBody('# Title\n\nbody\n\nadded externally')) + doc.destroy() + stored.destroy() + }) + + it('a client holding the resumed document merges it back without duplicating the file', async () => { + // The end-to-end property, stated the way it fails: a tab that outlived its room reconnects and + // syncs. Building a second document here appends the whole file to itself, on both sides. + const original = markdownToYDoc('# Title\n\nfirst\n\nsecond') + original.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.docIdKey, 'doc-original') + const client = new Y.Doc() + Y.applyUpdate(client, Y.encodeStateAsUpdate(original)) + + mockLoadState.mockResolvedValue({ + docState: Y.encodeStateAsUpdate(original), + sourceHash: 'stale-after-an-external-write', + }) + mockFetchBuffer.mockResolvedValue(Buffer.from('# Title\n\nfirst\n\nsecond', 'utf-8')) + + const seed = await buildFileDocSeed('ws-1', 'file-1') + Y.applyUpdate(client, seed!.update) + + expect(yDocToMarkdown(client)).toBe(serializeMarkdownBody('# Title\n\nfirst\n\nsecond')) + client.destroy() + original.destroy() + }) + + it('rebuilds from markdown when the stored document is unusable, rather than failing the seed', async () => { + mockLoadState.mockResolvedValue({ + docState: new Uint8Array([9, 9, 9, 9]), + sourceHash: 'stale', + }) + mockFetchBuffer.mockResolvedValue(Buffer.from('# Title\n\nbody', 'utf-8')) + + const seed = await buildFileDocSeed('ws-1', 'file-1') + + expect(seed).not.toBeNull() + const doc = new Y.Doc() + Y.applyUpdate(doc, seed!.update) + expect(yDocToMarkdown(doc)).toBe(serializeMarkdownBody('# Title\n\nbody')) + doc.destroy() + }) + + it('still seeds when the document cannot be stored (the write is best-effort)', async () => { + mockFetchBuffer.mockResolvedValue(Buffer.from('# Title', 'utf-8')) + mockSaveState.mockRejectedValue(new Error('db down')) + + const seed = await buildFileDocSeed('ws-1', 'file-1') + + expect(seed).not.toBeNull() + expect(typeof docIdOf(seed!.update)).toBe('string') + }) +}) diff --git a/apps/sim/lib/collab-doc/seed.ts b/apps/sim/lib/collab-doc/seed.ts index 8c25d533017..21a506a4b9b 100644 --- a/apps/sim/lib/collab-doc/seed.ts +++ b/apps/sim/lib/collab-doc/seed.ts @@ -1,11 +1,17 @@ import { createLogger } from '@sim/logger' import { FILE_DOC_SEED } from '@sim/realtime-protocol/file-doc' import { getErrorMessage } from '@sim/utils/errors' +import { generateId } from '@sim/utils/id' import * as Y from 'yjs' import { fetchWorkspaceFileBuffer, getWorkspaceFile } from '@/lib/uploads/contexts/workspace' import { splitFrontmatter } from '@/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity' -import { hashMarkdown, loadFreshCollabDocState } from './collab-state' -import { canonicalizeYDoc, markdownToYDoc } from './converter' +import { + type CachedCollabDocState, + hashMarkdown, + loadCollabDocState, + saveCollabDocState, +} from './collab-state' +import { applyMarkdownToYDoc, canonicalizeYDoc, markdownToYDoc } from './converter' const logger = createLogger('FileDocSeed') @@ -74,28 +80,30 @@ export async function buildFileDocSeed( const version = (record.contentUpdatedAt ?? record.updatedAt).getTime() const buffer = await fetchWorkspaceFileBuffer(record, { maxBytes: MAX_SEED_BYTES }) - // Cold-start fast path: if we hold a cached Yjs binary derived from THIS exact markdown, apply it - // directly (the Hocuspocus load-document pattern) instead of re-converting. This preserves the CRDT's - // client ids across reopens — no duplicated content, no split-brain — and skips the server-side - // headless conversion. A stale/absent cache (markdown edited externally, or first ever open) falls - // through to the conversion below, and the next persist refreshes the cache. - // + const sourceHash = hashMarkdown(buffer) + // Best-effort read: the cache is an optimization over the durable markdown we already hold, so a // transient DB error (or a not-yet-migrated cache table) must fall through to conversion rather than - // block the cold open — symmetric with persist's best-effort cache write. + // block the cold open — symmetric with the best-effort write below. + let stored: CachedCollabDocState | null = null try { - const cached = await loadFreshCollabDocState(fileId, hashMarkdown(buffer)) - if (cached) return { update: canonicalSeedUpdate(cached), version } + stored = await loadCollabDocState(fileId) } catch (error) { logger.warn(`Failed to read cached collab doc state for file ${fileId}`, { error: getErrorMessage(error), }) } - const markdown = buffer.toString('utf-8') - const { frontmatter, body } = splitFrontmatter(markdown) + // Cold-start fast path: the stored document already projects to THIS markdown, so apply it verbatim + // (the Hocuspocus load-document pattern) instead of re-converting. + if (stored?.sourceHash === sourceHash) { + return { update: canonicalSeedUpdate(stored.docState), version } + } - const ydoc = markdownToYDoc(body) + const { frontmatter, body } = splitFrontmatter(buffer.toString('utf-8')) + // The markdown moved on out-of-band (a copilot write, the content API, a file tool) — bring the + // STORED document up to it rather than building a second one. See {@link resumeDocument}. + const ydoc = resumeDocument(fileId, stored?.docState, body) try { const config = ydoc.getMap(FILE_DOC_SEED.configMap) // Mark the document seeded IN the same doc, so the client's readiness gate @@ -105,8 +113,54 @@ export async function buildFileDocSeed( // Carry the frontmatter in the doc (not the body) so it merges across clients and a later // server-side edit can update it — the editor re-attaches this on autosave. config.set(FILE_DOC_SEED.frontmatterKey, frontmatter) - return { update: Y.encodeStateAsUpdate(ydoc), version } + // Mint an identity only for a document that has none; a resumed one keeps the identity its clients + // already know it by. + if (typeof config.get(FILE_DOC_SEED.docIdKey) !== 'string') { + config.set(FILE_DOC_SEED.docIdKey, generateId()) + } + const update = Y.encodeStateAsUpdate(ydoc) + // Store it NOW, not at the next persist. Until this row exists every cold open builds the document + // again from markdown, minting a new identity each time — so a file that is opened but never edited + // has a different document on every open, and any client that outlives a room (a laptop that slept + // past the shared stream's TTL) reconnects into one and merges its content in twice. + try { + await saveCollabDocState(fileId, update, sourceHash) + } catch (error) { + logger.warn(`Failed to store the collaborative document for file ${fileId}`, { + error: getErrorMessage(error), + }) + } + return { update, version } } finally { ydoc.destroy() } } + +/** + * The file's collaborative document, brought up to `body`. + * + * Two Yjs documents built from the same markdown are NOT the same document: their items carry + * different client ids, so merging them appends one to the other — the file, twice. Anything that + * rebuilds a document from markdown therefore mints a new identity, and any client still holding the + * previous one corrupts the file the moment it reconnects. So a document is built exactly once and + * every later change is applied INTO it as a CRDT diff (the same path a copilot edit takes), which is + * what keeps one file to one document for its whole life. + */ +function resumeDocument(fileId: string, stored: Uint8Array | undefined, body: string): Y.Doc { + if (!stored) return markdownToYDoc(body) + const ydoc = new Y.Doc() + try { + Y.applyUpdate(ydoc, stored) + applyMarkdownToYDoc(ydoc, body) + return ydoc + } catch (error) { + // The stored document is the file's identity, but it is still a CACHE: an undecodable one must not + // take the file's markdown down with it. Build a new document — which mints a new identity, so a + // client still holding the old one is refused rather than merged (see FILE_DOC_SEED.docIdKey). + logger.warn(`Stored collaborative document for file ${fileId} is unusable; rebuilding it`, { + error: getErrorMessage(error), + }) + ydoc.destroy() + return markdownToYDoc(body) + } +} diff --git a/apps/sim/lib/workspace-files/application/read-workspace-inline-file.ts b/apps/sim/lib/workspace-files/application/read-workspace-inline-file.ts index 1dba3799607..e6742b2d4fb 100644 --- a/apps/sim/lib/workspace-files/application/read-workspace-inline-file.ts +++ b/apps/sim/lib/workspace-files/application/read-workspace-inline-file.ts @@ -21,9 +21,20 @@ export interface ReadWorkspaceInlineFileInput { export interface ReadWorkspaceInlineFileResult { file: WorkspaceFileRecord stream: ReadableStream + /** + * How the caller addressed the file, which decides whether the response may be cached. + * + * `key` names a storage object, and a content write never rewrites one — it uploads under a fresh + * key and repoints the row, so the lookup that resolved this request (an exact match on the CURRENT + * key) either found bytes that can never change or found nothing at all. The response is therefore + * immutable. `fileId` names the FILE, whose bytes move as it is edited, so those responses must keep + * revalidating. + */ + addressedBy: 'key' | 'fileId' } async function executeReadWorkspaceInlineFile({ + input, context, }: AuthorizedWorkspaceUseCaseContext< typeof fileOperations.readContent, @@ -36,7 +47,11 @@ async function executeReadWorkspaceInlineFile({ if (!file) throw new OrchestrationError('not_found', 'Not found') const stream = await downloadFileStream({ key: file.key, context: 'workspace' }) - return { file, stream: nodeReadableToWebStream(stream) } + return { + file, + stream: nodeReadableToWebStream(stream), + addressedBy: input.key ? 'key' : 'fileId', + } } export const readWorkspaceInlineFile = defineAuthorizedWorkspaceFileUseCase({ diff --git a/packages/realtime-protocol/src/file-doc.ts b/packages/realtime-protocol/src/file-doc.ts index 3e50cf097df..6fd6ae25c36 100644 --- a/packages/realtime-protocol/src/file-doc.ts +++ b/packages/realtime-protocol/src/file-doc.ts @@ -86,6 +86,16 @@ export const FILE_DOC_SEED = { * be reflected instead of reverted by an open editor's autosave. */ frontmatterKey: 'frontmatter', + /** + * The document's IDENTITY: minted when a file's collaborative document is first built and carried in + * the CRDT from then on, so every later load resumes the same document rather than minting a second. + * + * It matters because two documents built from the same markdown are not the same document to Yjs — + * their items carry different client ids, so merging them yields the content TWICE. A client that + * still holds one identity must therefore never sync against another: the server sends this in the + * join ack and the client refuses to merge a document it does not recognize. + */ + docIdKey: 'docId', } as const /** @@ -129,6 +139,13 @@ export interface JoinFileDocPayload { /** Server → client acceptance of a {@link FILE_DOC_EVENTS.JOIN}. */ export interface JoinFileDocSuccess { fileId: string + /** + * The identity of the document this room holds ({@link FILE_DOC_SEED.docIdKey}), so a client can tell + * "the room I left" from "a document built in its place" BEFORE it syncs. Absent for a room whose doc + * carries no identity (an empty/missing file, or one seeded before identities existed), which is + * exactly the case where there is nothing to compare and the client proceeds. + */ + docId?: string } /** Server → client rejection of a {@link FILE_DOC_EVENTS.JOIN}. */ From cd413c59dabda0c895593d85ffeb882f3d88f3c1 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Wed, 12 Aug 2026 22:53:04 -0700 Subject: [PATCH 3/3] fix(files): name every collaborative document, and only cache what the URL names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from review, both real. A document stored before identities existed is returned by the seed's fast path on every open, and that path never named one — so those files could never acquire an identity, and the join-ack guard could never fire for them. That is the population most likely to have a tab that outlived its room, which is the case the guard exists for. The fast path now names an unnamed document and stores it, once: minting without storing would name it differently on every open and the guard would start refusing clients that hold the very same document. The inline route marked a response immutable whenever the caller passed a key, but a key is resolved to a FILE and the file's current key is what gets streamed. A content write landing between those two reads would serve the new bytes under a URL naming the old object — and cached for a year, that is wrong forever. The flag is now what it always meant: the URL names the exact object that was streamed. --- .../[id]/files/inline/route.test.ts | 6 +- .../api/workspaces/[id]/files/inline/route.ts | 25 ++++--- apps/sim/lib/collab-doc/seed.test.ts | 32 +++++++++ apps/sim/lib/collab-doc/seed.ts | 66 ++++++++++++++----- .../read-workspace-inline-file.test.ts | 33 ++++++++++ .../application/read-workspace-inline-file.ts | 17 ++--- 6 files changed, 138 insertions(+), 41 deletions(-) diff --git a/apps/sim/app/api/workspaces/[id]/files/inline/route.test.ts b/apps/sim/app/api/workspaces/[id]/files/inline/route.test.ts index 793f3f9f245..2b95cd7e007 100644 --- a/apps/sim/app/api/workspaces/[id]/files/inline/route.test.ts +++ b/apps/sim/app/api/workspaces/[id]/files/inline/route.test.ts @@ -30,7 +30,7 @@ describe('GET /api/workspaces/[id]/files/inline', () => { mockReadInline.mockResolvedValue({ file: { name: 'photo.png', type: 'image/png', size: PNG.length }, stream: new Blob([new Uint8Array(PNG)]).stream(), - addressedBy: 'fileId', + contentAddressed: false, }) }) @@ -66,11 +66,11 @@ describe('GET /api/workspaces/[id]/files/inline', () => { * rendered by two editors (the read-only placeholder, then the live one) and each renders the image * twice, so the image was fetched again on every one of those passes. */ - it('lets the browser keep an image addressed by storage key', async () => { + it('lets the browser keep an image whose URL names the object that was streamed', async () => { mockReadInline.mockResolvedValue({ file: { name: 'photo.png', type: 'image/png', size: PNG.length }, stream: new Blob([new Uint8Array(PNG)]).stream(), - addressedBy: 'key', + contentAddressed: true, }) const res = await GET(req('key=workspace%2Fws-1%2Fphoto.png'), params) diff --git a/apps/sim/app/api/workspaces/[id]/files/inline/route.ts b/apps/sim/app/api/workspaces/[id]/files/inline/route.ts index 2ede7737d24..0a3f20bf4ec 100644 --- a/apps/sim/app/api/workspaces/[id]/files/inline/route.ts +++ b/apps/sim/app/api/workspaces/[id]/files/inline/route.ts @@ -11,23 +11,22 @@ import { encodeFilenameForHeader, getSecureFileHeaders } from '@/app/api/files/u export const dynamic = 'force-dynamic' /** - * How long the browser may reuse an embedded image, by how the request addressed it (see - * {@link ReadWorkspaceInlineFileResult.addressedBy}). + * How long the browser may reuse an embedded image, decided by whether the URL names the exact object + * that was streamed (see {@link ReadWorkspaceInlineFileResult.contentAddressed}). * - * A `key` names one storage object and a content write never rewrites one, so those bytes are - * immutable and the browser needs no round trip — which is the difference between an embedded image - * reappearing instantly and it being downloaded again. Every document render asks for the same image - * at least twice (ProseMirror's own DOM, then the React node view) and every editor mounts twice (the + * A content write never rewrites a storage object, so a URL that names one addresses bytes that can + * never change and the browser needs no round trip — which is the difference between an embedded image + * reappearing instantly and being downloaded again. Every document render asks for the same image at + * least twice (ProseMirror's own DOM, then the React node view) and every editor mounts twice (the * read-only placeholder, then the live editor), so revalidating each time meant re-fetching the whole * image on every open and reload — measured at ~1 MB per open on a real document, with the image area * blank until it landed. `private` keeps it out of shared caches: the bytes are authorized per user. * - * A `fileId` names the FILE, and its bytes move under it on every edit, so that form keeps revalidating. + * Anything else — a request that names the FILE, whose bytes move under it, or one whose object was + * rotated away mid-request — keeps revalidating. */ -const INLINE_CACHE_CONTROL = { - key: 'private, max-age=31536000, immutable', - fileId: 'private, no-cache, must-revalidate', -} as const +const IMMUTABLE_CACHE_CONTROL = 'private, max-age=31536000, immutable' +const REVALIDATE_CACHE_CONTROL = 'private, no-cache, must-revalidate' /** * GET /api/workspaces/[id]/files/inline?key=|fileId= @@ -48,12 +47,12 @@ export const GET = defineInternalBinaryRoute({ fileId: query.fileId, }), useCase: readWorkspaceInlineFile, - present: ({ file, stream, addressedBy }) => { + present: ({ file, stream, contentAddressed }) => { const secure = getSecureFileHeaders(file.name, file.type) const headers = new Headers({ 'Content-Type': secure.contentType, 'Content-Disposition': `${secure.disposition}; ${encodeFilenameForHeader(file.name)}`, - 'Cache-Control': INLINE_CACHE_CONTROL[addressedBy], + 'Cache-Control': contentAddressed ? IMMUTABLE_CACHE_CONTROL : REVALIDATE_CACHE_CONTROL, 'X-Content-Type-Options': 'nosniff', }) if (secure.contentType === 'image/svg+xml') { diff --git a/apps/sim/lib/collab-doc/seed.test.ts b/apps/sim/lib/collab-doc/seed.test.ts index 29e83a2e2df..be67bab3f73 100644 --- a/apps/sim/lib/collab-doc/seed.test.ts +++ b/apps/sim/lib/collab-doc/seed.test.ts @@ -69,6 +69,9 @@ describe('buildFileDocSeed', () => { // repaired on the way through and this would assert the fast path while never taking it. const cachedDoc = markdownToYDoc('# Anything') cachedDoc.getText('marker').insert(0, 'cached') + // Named, as anything the current seed stored would be — an unnamed document is rewritten once to + // give it an identity, which has its own test below. + cachedDoc.getMap(FILE_DOC_SEED.configMap).set(FILE_DOC_SEED.docIdKey, 'doc-already-named') const cached = Y.encodeStateAsUpdate(cachedDoc) mockFetchBuffer.mockResolvedValue(Buffer.from('# Anything', 'utf-8')) mockLoadState.mockResolvedValue({ docState: cached, sourceHash: 'test-source-hash' }) @@ -273,6 +276,35 @@ describe('buildFileDocSeed — document identity', () => { doc.destroy() }) + /** + * A document stored before identities existed is returned by the fast path on every open, so if it + * were named only where documents are BUILT those files would never acquire one — and the join-ack + * guard could never fire for them, which is the population most likely to have a tab that outlived + * its room. Naming it must also be stored, or every open would name it differently and the guard + * would refuse a client holding the very same document. + */ + it('names a stored document that predates identities, once, and keeps that name', async () => { + const legacy = markdownToYDoc('# Legacy') + mockFetchBuffer.mockResolvedValue(Buffer.from('# Legacy', 'utf-8')) + mockLoadState.mockResolvedValue({ + docState: Y.encodeStateAsUpdate(legacy), + sourceHash: 'test-source-hash', + }) + + const first = await buildFileDocSeed('ws-1', 'file-1') + const docId = docIdOf(first!.update) + expect(typeof docId).toBe('string') + expect(mockSaveState).toHaveBeenCalledWith('file-1', first!.update, 'test-source-hash') + + // The next open finds it named and hands back the stored bytes untouched. + mockSaveState.mockClear() + mockLoadState.mockResolvedValue({ docState: first!.update, sourceHash: 'test-source-hash' }) + const second = await buildFileDocSeed('ws-1', 'file-1') + expect(docIdOf(second!.update)).toBe(docId) + expect(mockSaveState).not.toHaveBeenCalled() + legacy.destroy() + }) + it('still seeds when the document cannot be stored (the write is best-effort)', async () => { mockFetchBuffer.mockResolvedValue(Buffer.from('# Title', 'utf-8')) mockSaveState.mockRejectedValue(new Error('db down')) diff --git a/apps/sim/lib/collab-doc/seed.ts b/apps/sim/lib/collab-doc/seed.ts index 21a506a4b9b..adc10b1aa4a 100644 --- a/apps/sim/lib/collab-doc/seed.ts +++ b/apps/sim/lib/collab-doc/seed.ts @@ -41,19 +41,57 @@ export interface FileDocSeed { * that ever repairs it. Running the round-trip here is that repair, and it doubles as the bound on this * branch: the cached path never calls `parseMarkdownToDoc`, so it is the one way into a room that the * parse-side limits do not cover. Returns the original bytes untouched when the snapshot is already - * canonical (the common case) — no re-encode — and a fresh encode, preserving the CRDT's client ids, when - * it repaired one. + * canonical AND already named (the common case) — no re-encode — and a fresh encode, preserving the + * CRDT's client ids, when it had to repair or name one. + * + * Naming happens here too, not only where a document is built: a document stored before identities + * existed is returned by this path on every open, so if it were skipped here those files would never + * acquire one and the join-ack guard could never fire for them — which is the population most likely to + * have a tab that outlived its room. `changed` tells the caller to store what it got back, so the + * identity is minted ONCE and every later open agrees with it (a re-minted one would make the guard + * refuse a client holding the very same document). */ -function canonicalSeedUpdate(cached: Uint8Array): Uint8Array { +function prepareCachedSeed(cached: Uint8Array): { update: Uint8Array; changed: boolean } { const doc = new Y.Doc() try { Y.applyUpdate(doc, cached) - return canonicalizeYDoc(doc) ? Y.encodeStateAsUpdate(doc) : cached + const repaired = canonicalizeYDoc(doc) + const named = ensureDocumentIdentity(doc) + return repaired || named + ? { update: Y.encodeStateAsUpdate(doc), changed: true } + : { update: cached, changed: false } } finally { doc.destroy() } } +/** + * Give a document an identity if it has none, and report whether it needed one. A resumed document + * keeps the identity its clients already know it by — re-minting would make the join-ack guard refuse + * a client that holds this exact document. + */ +function ensureDocumentIdentity(ydoc: Y.Doc): boolean { + const config = ydoc.getMap(FILE_DOC_SEED.configMap) + if (typeof config.get(FILE_DOC_SEED.docIdKey) === 'string') return false + config.set(FILE_DOC_SEED.docIdKey, generateId()) + return true +} + +/** Store the file's collaborative document. Best-effort: the durable markdown is the source of truth. */ +async function storeDocument( + fileId: string, + update: Uint8Array, + sourceHash: string +): Promise { + try { + await saveCollabDocState(fileId, update, sourceHash) + } catch (error) { + logger.warn(`Failed to store the collaborative document for file ${fileId}`, { + error: getErrorMessage(error), + }) + } +} + /** * Build the server-side seed for a file's collaborative document: load the file's current markdown * and convert it — through the exact client engine (see {@link markdownToYDoc}) — into a Yjs update. @@ -97,7 +135,11 @@ export async function buildFileDocSeed( // Cold-start fast path: the stored document already projects to THIS markdown, so apply it verbatim // (the Hocuspocus load-document pattern) instead of re-converting. if (stored?.sourceHash === sourceHash) { - return { update: canonicalSeedUpdate(stored.docState), version } + const prepared = prepareCachedSeed(stored.docState) + // Store a repair or a freshly-minted identity so the next open finds it done — and, for the + // identity, so every open names the SAME document. + if (prepared.changed) await storeDocument(fileId, prepared.update, sourceHash) + return { update: prepared.update, version } } const { frontmatter, body } = splitFrontmatter(buffer.toString('utf-8')) @@ -113,23 +155,13 @@ export async function buildFileDocSeed( // Carry the frontmatter in the doc (not the body) so it merges across clients and a later // server-side edit can update it — the editor re-attaches this on autosave. config.set(FILE_DOC_SEED.frontmatterKey, frontmatter) - // Mint an identity only for a document that has none; a resumed one keeps the identity its clients - // already know it by. - if (typeof config.get(FILE_DOC_SEED.docIdKey) !== 'string') { - config.set(FILE_DOC_SEED.docIdKey, generateId()) - } + ensureDocumentIdentity(ydoc) const update = Y.encodeStateAsUpdate(ydoc) // Store it NOW, not at the next persist. Until this row exists every cold open builds the document // again from markdown, minting a new identity each time — so a file that is opened but never edited // has a different document on every open, and any client that outlives a room (a laptop that slept // past the shared stream's TTL) reconnects into one and merges its content in twice. - try { - await saveCollabDocState(fileId, update, sourceHash) - } catch (error) { - logger.warn(`Failed to store the collaborative document for file ${fileId}`, { - error: getErrorMessage(error), - }) - } + await storeDocument(fileId, update, sourceHash) return { update, version } } finally { ydoc.destroy() diff --git a/apps/sim/lib/workspace-files/application/read-workspace-inline-file.test.ts b/apps/sim/lib/workspace-files/application/read-workspace-inline-file.test.ts index f1012bc3c12..64f786f051b 100644 --- a/apps/sim/lib/workspace-files/application/read-workspace-inline-file.test.ts +++ b/apps/sim/lib/workspace-files/application/read-workspace-inline-file.test.ts @@ -76,6 +76,39 @@ describe('readWorkspaceInlineFile', () => { expect(mockLoadContext).toHaveBeenCalledWith('f1') }) + /** + * The response is marked immutable only when the URL names the object that was streamed. The key is + * resolved to a file and the file's CURRENT key is what gets downloaded, so a rotation landing + * between those two reads would serve new bytes under a URL naming the old object — cached, that + * would be wrong forever. + */ + it('is content-addressed only when the requested key is the one it streamed', async () => { + mockGetMetadataByKey.mockResolvedValue({ id: 'f1', workspaceId: 'ws-1' }) + + const match = await readWorkspaceInlineFile.execute({ + principal, + input: { workspaceId: 'ws-1', key: file.key }, + }) + expect(match.contentAddressed).toBe(true) + + // The file's content was replaced between resolving the key and reading the row. + mockGetWorkspaceFile.mockResolvedValue({ ...file, key: 'workspace/ws-1/rotated-photo.png' }) + const rotated = await readWorkspaceInlineFile.execute({ + principal, + input: { workspaceId: 'ws-1', key: file.key }, + }) + expect(rotated.contentAddressed).toBe(false) + }) + + it('is never content-addressed when the caller named the file rather than an object', async () => { + const result = await readWorkspaceInlineFile.execute({ + principal, + input: { workspaceId: 'ws-1', fileId: 'f1' }, + }) + + expect(result.contentAddressed).toBe(false) + }) + it('conceals a key belonging to another workspace before authorization', async () => { mockGetMetadataByKey.mockResolvedValue({ id: 'other', workspaceId: 'ws-other' }) diff --git a/apps/sim/lib/workspace-files/application/read-workspace-inline-file.ts b/apps/sim/lib/workspace-files/application/read-workspace-inline-file.ts index e6742b2d4fb..cff1cc494c9 100644 --- a/apps/sim/lib/workspace-files/application/read-workspace-inline-file.ts +++ b/apps/sim/lib/workspace-files/application/read-workspace-inline-file.ts @@ -22,15 +22,16 @@ export interface ReadWorkspaceInlineFileResult { file: WorkspaceFileRecord stream: ReadableStream /** - * How the caller addressed the file, which decides whether the response may be cached. + * Whether the URL that produced this response names the exact storage object it streamed — the only + * condition under which the bytes may be cached, since a content write never rewrites an object (it + * uploads under a fresh key and repoints the row). * - * `key` names a storage object, and a content write never rewrites one — it uploads under a fresh - * key and repoints the row, so the lookup that resolved this request (an exact match on the CURRENT - * key) either found bytes that can never change or found nothing at all. The response is therefore - * immutable. `fileId` names the FILE, whose bytes move as it is edited, so those responses must keep - * revalidating. + * It is deliberately not "the caller passed a key": the key is resolved to a file and the file's + * CURRENT key is what gets streamed, so a rotation landing between those two reads would serve the + * new bytes under a URL naming the old object — cached, that would be wrong forever. A `fileId` + * request names the FILE, whose bytes move as it is edited, and is never content-addressed. */ - addressedBy: 'key' | 'fileId' + contentAddressed: boolean } async function executeReadWorkspaceInlineFile({ @@ -50,7 +51,7 @@ async function executeReadWorkspaceInlineFile({ return { file, stream: nodeReadableToWebStream(stream), - addressedBy: input.key ? 'key' : 'fileId', + contentAddressed: input.key !== undefined && input.key === file.key, } }