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/_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/api/workspaces/[id]/files/inline/route.test.ts b/apps/sim/app/api/workspaces/[id]/files/inline/route.test.ts index 495044427bb..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,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(), + contentAddressed: false, }) }) @@ -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 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(), + contentAddressed: true, + }) + + 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..0a3f20bf4ec 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,24 @@ import { encodeFilenameForHeader, getSecureFileHeaders } from '@/app/api/files/u export const dynamic = 'force-dynamic' +/** + * 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 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. + * + * 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 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= * @@ -29,12 +47,12 @@ export const GET = defineInternalBinaryRoute({ fileId: query.fileId, }), useCase: readWorkspaceInlineFile, - present: ({ file, stream }) => { + 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': 'private, no-cache, must-revalidate', + '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/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/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/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..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 @@ -721,6 +721,15 @@ export function LoadedRichMarkdownEditor({ * is latched, so a fatal rejection that fired before this subscription is not missed. */ useEffect(() => { + /** + * 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). + */ const setReady = (ready: boolean) => { // Child-local: gates editability (a user must never type into an unsynced/unseeded doc). setCollabReady(ready) @@ -766,12 +775,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 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..6dea419e9a4 100644 --- a/apps/sim/hooks/queries/workspace-files.test.tsx +++ b/apps/sim/hooks/queries/workspace-files.test.tsx @@ -9,13 +9,15 @@ */ 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' import { useCreateWorkspaceFile, useWorkspaceFileContent, + useWorkspaceFiles, + type WorkspaceFileContentResult, workspaceFilesKeys, } from '@/hooks/queries/workspace-files' @@ -44,7 +46,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 +69,7 @@ function renderContentHook(options?: { ) }) return { + queryClient, unmount: () => { act(() => root.unmount()) queryClient.clear() @@ -149,6 +153,98 @@ 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 refetchQueries = vi.spyOn(queryClient, 'refetchQueries') + + await act(async () => { + await sleep(50) + }) + + expect(fetchCount).toBe(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(refetchQueries).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 refetchQueries = vi.spyOn(queryClient, 'refetchQueries') + + await act(async () => { + await sleep(50) + }) + + expect(fetchCount).toBe(1) + expect(refetchQueries).not.toHaveBeenCalled() + unmount() + }) +}) + describe('useCreateWorkspaceFile', () => { it('uses the create contract and reconciles workspace file caches', async () => { const response = { success: true, file: { id: 'wf-created' } } @@ -179,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 a6eaaf4ebe7..a236a623f2f 100644 --- a/apps/sim/hooks/queries/workspace-files.ts +++ b/apps/sim/hooks/queries/workspace-files.ts @@ -1,9 +1,15 @@ -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' 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' @@ -202,6 +208,63 @@ 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. + * + * 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 + // 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] + ) +} + /** * Fetch file content as text via a content-source URL */ @@ -209,6 +272,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,24 +291,80 @@ 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 + } +): WorkspaceFileContentResult { const source = useFileContentSource() - return useQuery({ + const recoverStaleKey = useStaleKeyRecovery(workspaceId) + const query = 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, }) + 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), + } } /** @@ -280,6 +400,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 +424,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/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/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..96f1f4af931 --- /dev/null +++ b/apps/sim/lib/collab-doc/persist.test.ts @@ -0,0 +1,271 @@ +/** + * @vitest-environment jsdom + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import * as Y from 'yjs' + +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, + getWorkspaceFile: mockGetWorkspaceFile, + fetchWorkspaceFileBuffer: mockFetchBuffer, + updateWorkspaceFileContent: mockUpdateContent, +})) + +vi.mock('./collab-state', () => ({ + hashMarkdown: (buffer: Buffer) => `hash:${buffer.toString('utf-8')}`, + saveCollabDocState: mockSaveState, + collabDocStateSourceHash: mockStateSourceHash, +})) + +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) + mockStateSourceHash.mockResolvedValue(null) + }) + + 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(), + `hash:${projectionOf(md).toString('utf-8')}` + ) + }) + + 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) + }) +}) + +/** + * 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 357e3d20a47..2e8e9692ff4 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 { collabDocStateSourceHash, hashMarkdown, saveCollabDocState } from './collab-state' +import { canonicalizeYDoc, yDocToFileMarkdown } from './converter' const logger = createLogger('FileDocPersist') @@ -69,20 +69,51 @@ 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() } - try { + // 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(), + } + } + } + + const write = async (ifMatch: number): Promise => { const updated = await updateWorkspaceFileContent( workspaceId, fileId, @@ -93,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' }, } ) @@ -119,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 28e8562464a..be67bab3f73 100644 --- a/apps/sim/lib/collab-doc/seed.test.ts +++ b/apps/sim/lib/collab-doc/seed.test.ts @@ -2,13 +2,16 @@ * @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' -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', () => ({ @@ -20,11 +23,17 @@ 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 { 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', () => { @@ -37,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 () => { @@ -54,12 +64,17 @@ 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') + // 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')) - mockLoadFresh.mockResolvedValue(cached) + mockLoadState.mockResolvedValue({ docState: cached, sourceHash: 'test-source-hash' }) const seed = await buildFileDocSeed('ws-1', 'file-1') @@ -67,13 +82,46 @@ 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')) + mockLoadState.mockResolvedValue({ docState: cached, sourceHash: 'test-source-hash' }) + + 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 () => { // 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() @@ -126,3 +174,144 @@ 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() + }) + + /** + * 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')) + + 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 d0c7fa73f4d..adc10b1aa4a 100644 --- a/apps/sim/lib/collab-doc/seed.ts +++ b/apps/sim/lib/collab-doc/seed.ts @@ -1,12 +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 { markdownToYDoc } from './converter' -import { stripEmptyTopLevelParagraphs } from './normalize' +import { + type CachedCollabDocState, + hashMarkdown, + loadCollabDocState, + saveCollabDocState, +} from './collab-state' +import { applyMarkdownToYDoc, canonicalizeYDoc, markdownToYDoc } from './converter' const logger = createLogger('FileDocSeed') @@ -28,23 +33,65 @@ 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 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 normalizeSeedUpdate(cached: Uint8Array): Uint8Array { +function prepareCachedSeed(cached: Uint8Array): { update: Uint8Array; changed: boolean } { const doc = new Y.Doc() try { Y.applyUpdate(doc, cached) - return stripEmptyTopLevelParagraphs(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. @@ -71,28 +118,34 @@ 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: normalizeSeedUpdate(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) { + 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 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 @@ -102,8 +155,44 @@ 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 } + 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. + await storeDocument(fileId, update, sourceHash) + 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.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 1dba3799607..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 @@ -21,9 +21,21 @@ export interface ReadWorkspaceInlineFileInput { export interface ReadWorkspaceInlineFileResult { file: WorkspaceFileRecord stream: ReadableStream + /** + * 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). + * + * 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. + */ + contentAddressed: boolean } async function executeReadWorkspaceInlineFile({ + input, context, }: AuthorizedWorkspaceUseCaseContext< typeof fileOperations.readContent, @@ -36,7 +48,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), + contentAddressed: input.key !== undefined && input.key === file.key, + } } 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}. */