From c49e7d8bfaceacb22c442e5ba576677f8cfd10d7 Mon Sep 17 00:00:00 2001 From: George Ng Date: Thu, 3 Sep 2026 15:31:56 -0700 Subject: [PATCH 1/4] fix(markdown): persist browser editor changes Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../agents/markdown/src/view/route/service.ts | 107 +++-- .../view/site/core/collaboration-manager.ts | 11 +- .../src/view/site/core/document-manager.ts | 372 ++++++++++++++---- .../agents/markdown/src/view/site/index.ts | 18 +- .../agents/markdown/src/view/site/types.ts | 4 + .../src/view/site/ui/toolbar-manager.ts | 16 +- .../agents/markdown/src/view/site/utils.ts | 17 +- .../markdown/test/browserPersistence.spec.ts | 340 ++++++++++++++++ .../agents/markdown/test/viewService.spec.ts | 34 ++ 9 files changed, 784 insertions(+), 135 deletions(-) create mode 100644 ts/packages/agents/markdown/test/browserPersistence.spec.ts diff --git a/ts/packages/agents/markdown/src/view/route/service.ts b/ts/packages/agents/markdown/src/view/route/service.ts index 75ba650fc3..919dafb4ee 100644 --- a/ts/packages/agents/markdown/src/view/route/service.ts +++ b/ts/packages/agents/markdown/src/view/route/service.ts @@ -1717,6 +1717,38 @@ erDiagram }; } +async function captureStableBindingRevision(): Promise< + | { + snapshot: BindingSnapshot; + revision: string | null; + } + | undefined +> { + for (let attempt = 0; attempt < 3; attempt++) { + const snapshot = captureBindingSnapshot(); + let revision: string | null = null; + if (snapshot.filePath && snapshot.boundRelativePath) { + try { + revision = ( + await readBoundDocument({ + token: snapshot.bindingToken ?? undefined, + root: snapshot.currentRoot, + relativePath: snapshot.boundRelativePath, + filePath: snapshot.filePath, + }) + ).revision; + } catch (error) { + debug(`Unable to read binding revision: ${error}`); + return undefined; + } + } + if (!bindingsDiffer(captureBindingSnapshot(), snapshot)) { + return { snapshot, revision }; + } + } + return undefined; +} + app.get("/events", async (req: Request, res: Response) => { res.setHeader("Content-Type", "text/event-stream"); res.setHeader("Cache-Control", "no-cache"); @@ -1726,57 +1758,52 @@ app.get("/events", async (req: Request, res: Response) => { let closed = false; req.on("close", () => { closed = true; + const wasPrimary = clients[0] === res; clients = clients.filter((client) => client !== res); + if (wasPrimary && clients[0]) { + const nextPrimary = clients[0]; + void captureStableBindingRevision().then((binding) => { + if (binding && clients[0] === nextPrimary) { + safeWriteToResponse( + nextPrimary, + `data: ${JSON.stringify({ + type: "primaryElected", + bindingToken: binding.snapshot.bindingToken, + revision: binding.revision, + timestamp: Date.now(), + })}\n\n`, + ); + } + }); + } }); - let bootstrap = captureBindingSnapshot(); - let revision: string | null = null; - for (let attempt = 0; attempt < 3; attempt++) { - bootstrap = captureBindingSnapshot(); - revision = null; - if (bootstrap.filePath && bootstrap.boundRelativePath) { - try { - revision = ( - await readBoundDocument({ - token: bootstrap.bindingToken ?? undefined, - root: bootstrap.currentRoot, - relativePath: bootstrap.boundRelativePath, - filePath: bootstrap.filePath, - }) - ).revision; - } catch (error) { - debug(`Unable to read binding bootstrap revision: ${error}`); - res.end(); - return; - } - } - if (closed) { - return; - } - if (!bindingsDiffer(captureBindingSnapshot(), bootstrap)) { - break; - } - if (attempt === 2) { - res.end(); - return; - } + const binding = await captureStableBindingRevision(); + if (!binding || closed) { + res.end(); + return; } - res.write( + const clientRole = clients.length === 0 ? "primary" : "secondary"; + const wroteBootstrap = safeWriteToResponse( + res, `data: ${JSON.stringify({ type: "bindingBootstrap", - bindingToken: bootstrap.bindingToken, - documentId: bootstrap.filePath - ? getCurrentDocumentId(bootstrap) + bindingToken: binding.snapshot.bindingToken, + documentId: binding.snapshot.filePath + ? getCurrentDocumentId(binding.snapshot) : null, - documentName: bootstrap.filePath - ? path.basename(bootstrap.filePath, ".md") + documentName: binding.snapshot.filePath + ? path.basename(binding.snapshot.filePath, ".md") : null, - boundRelativePath: bootstrap.boundRelativePath, - revision, + boundRelativePath: binding.snapshot.boundRelativePath, + revision: binding.revision, + clientRole, timestamp: Date.now(), })}\n\n`, ); - clients.push(res); + if (wroteBootstrap && !closed) { + clients.push(res); + } }); // Serve static files AFTER API routes to avoid conflicts diff --git a/ts/packages/agents/markdown/src/view/site/core/collaboration-manager.ts b/ts/packages/agents/markdown/src/view/site/core/collaboration-manager.ts index 308c9819a0..4f9663d15d 100644 --- a/ts/packages/agents/markdown/src/view/site/core/collaboration-manager.ts +++ b/ts/packages/agents/markdown/src/view/site/core/collaboration-manager.ts @@ -180,13 +180,18 @@ export class CollaborationManager { `[COLLAB] Retrieved collaboration info for document: ${collabInfo.currentDocument}`, ); + // The server-provided ID is the Yjs room key. A display + // basename is not unique for documents in different folders. + const authoritativeDocumentId = + typeof collabInfo.currentDocumentId === "string" && + collabInfo.currentDocumentId.length > 0 + ? collabInfo.currentDocumentId + : COLLABORATION_CONFIG.DEFAULT_DOCUMENT_ID; const config = { websocketServerUrl: collabInfo.websocketServerUrl || COLLABORATION_CONFIG.DEFAULT_WEBSOCKET_URL, - documentId: - collabInfo.currentDocumentId || - COLLABORATION_CONFIG.DEFAULT_DOCUMENT_ID, + documentId: authoritativeDocumentId, fallbackToLocal: true, }; diff --git a/ts/packages/agents/markdown/src/view/site/core/document-manager.ts b/ts/packages/agents/markdown/src/view/site/core/document-manager.ts index b03f076f1d..8f03e1d37b 100644 --- a/ts/packages/agents/markdown/src/view/site/core/document-manager.ts +++ b/ts/packages/agents/markdown/src/view/site/core/document-manager.ts @@ -5,7 +5,24 @@ import type { Editor } from "@milkdown/core"; import { editorViewCtx, parserCtx } from "@milkdown/core"; import { AI_CONFIG, DEFAULT_MARKDOWN_CONTENT, EDITOR_CONFIG } from "../config"; import { getMarkdownFromEditor, getEditorPositionInfo } from "../utils"; -import { encodeDocumentPathForUrl } from "../../route/urlPath"; +import { + encodeDocumentPathForUrl, + ensureMarkdownExtension, +} from "../../route/urlPath"; + +class DocumentWriteConflictError extends Error { + public constructor(message: string) { + super(message); + this.name = "DocumentWriteConflictError"; + } +} + +interface DocumentWriteResponse { + bindingToken?: unknown; + content?: unknown; + error?: unknown; + revision?: unknown; +} interface SSEEventData { type: string; @@ -29,6 +46,7 @@ interface SSEEventData { interface BindingStateData { documentId?: unknown; bindingToken?: unknown; + boundRelativePath?: unknown; revision?: unknown; } @@ -42,10 +60,34 @@ export class DocumentManager { private isPrimaryClient = false; private isBindingTransitionInProgress = false; private lastAutoSaveContent = ""; + private lastConflictedAutoSaveContent: string | null = null; private currentDocumentId = "default"; private bindingToken: string | null = null; private revision: string | null = null; private bindingVersion = 0; + private currentBoundRelativePath: string | null = null; + + // Keep the names introduced by the persistence change as aliases while + // retaining the parent's binding state machine and transition guards. + private get currentBindingToken(): string | null { + return this.bindingToken; + } + + private set currentBindingToken(value: string | null) { + this.bindingToken = value; + } + + private get currentRevision(): string | null { + return this.revision; + } + + private set currentRevision(value: string | null) { + this.revision = value; + } + + public getCurrentBoundRelativePath(): string | null { + return this.currentBoundRelativePath; + } public setNotificationManager(notificationManager: any): void { this.notificationManager = notificationManager; @@ -66,6 +108,7 @@ export class DocumentManager { public async initialize(): Promise { // Set up SSE connection for document change notifications this.setupSSEConnection(); + await this.loadCurrentBindingPath(); // Initialize auto-save if enabled if (EDITOR_CONFIG.FEATURES.AUTO_SAVE) { @@ -73,6 +116,26 @@ export class DocumentManager { } } + private async loadCurrentBindingPath(): Promise { + try { + const response = await fetch("/api/current-document"); + if (!response.ok) { + return; + } + const current = (await response.json()) as { + boundRelativePath?: unknown; + }; + if (typeof current.boundRelativePath === "string") { + this.currentBoundRelativePath = current.boundRelativePath; + } + } catch (error) { + console.warn( + "[DOCUMENT] Failed to load current binding path:", + error, + ); + } + } + /** * Start auto-save timer for primary client */ @@ -91,7 +154,7 @@ export class DocumentManager { } /** - * Perform auto-save if content has changed + * Persist changed editor Markdown from the primary browser only. */ private async performAutoSave(): Promise { try { @@ -113,14 +176,17 @@ export class DocumentManager { return; } - // Get current content using editor API const currentContent = await this.getMarkdownContent(editor); - - // Only save if content has changed if (currentContent === this.lastAutoSaveContent) { console.log("[AUTO-SAVE] Skipping - content unchanged"); return; } + if (currentContent === this.lastConflictedAutoSaveContent) { + console.warn( + "[AUTO-SAVE] Skipping unchanged content after a write conflict", + ); + return; + } console.log(`[AUTO-SAVE] Content changed, auto-saving...`); @@ -138,25 +204,31 @@ export class DocumentManager { }), }); + await this.reconcileDocumentWriteResponse( + response, + currentContent, + "Auto-save", + bindingToken, + bindingVersion, + ); if (response.ok) { - const result = await response.json(); - if ( - bindingVersion === this.bindingVersion && - bindingToken === this.bindingToken && - result.bindingToken === bindingToken - ) { - this.adoptRevision(result); - this.lastAutoSaveContent = currentContent; - } console.log("[AUTO-SAVE] Successfully saved document"); } else { - console.error( - "[AUTO-SAVE] Failed to save:", - response.statusText, + console.log( + "[AUTO-SAVE] Reconciled with content already persisted by another client", ); } } catch (error) { console.error("[AUTO-SAVE] Error during auto-save:", error); + if ( + error instanceof DocumentWriteConflictError && + this.notificationManager + ) { + this.notificationManager.showNotification( + error.message, + "error", + ); + } } } @@ -223,8 +295,17 @@ export class DocumentManager { break; case "autoSave": - this.adoptRevision(data); console.log(`[SSE] Auto-save completed for: ${data.filePath}`); + if ( + typeof data.bindingToken === "string" && + data.bindingToken === this.bindingToken + ) { + this.adoptRevision(data); + } else { + console.warn( + "[SSE] Ignoring autoSave revision for a stale binding", + ); + } // Auto-save notification removed per user request break; @@ -241,6 +322,22 @@ export class DocumentManager { await this.handleLLMOperations(data); break; + case "primaryElected": + if ( + typeof data.bindingToken !== "string" || + data.bindingToken !== this.bindingToken || + typeof data.revision !== "string" + ) { + console.warn( + "[SSE] Ignoring incomplete or stale primaryElected event", + ); + break; + } + this.isPrimaryClient = true; + this.adoptRevision(data); + console.log("[SSE] Promoted to PRIMARY for auto-save"); + break; + case "operationsBeingApplied": // Handle notification that operations are being applied by primary client console.log( @@ -272,25 +369,51 @@ export class DocumentManager { if ( typeof data.bindingToken === "string" && typeof data.documentId === "string" && - typeof data.revision === "string" + typeof data.revision === "string" && + typeof data.boundRelativePath === "string" ) { + const documentName = + typeof data.documentName === "string" + ? data.documentName + : data.boundRelativePath.replace(/\.md$/i, ""); await this.transitionToBinding( { documentId: data.documentId, bindingToken: data.bindingToken, + boundRelativePath: data.boundRelativePath, revision: data.revision, }, - data.documentName, + documentName, data.boundRelativePath, ); - } else if (data.bindingToken === null && data.documentId === null) { + } else if ( + data.bindingToken === null && + data.documentId === null && + data.boundRelativePath === null + ) { this.adoptBinding(data); + } else { + console.warn("[SSE] Ignoring incomplete bindingBootstrap event"); + return; + } + + if (data.clientRole === "primary") { + this.isPrimaryClient = true; + } else if (data.clientRole === "secondary") { + this.isPrimaryClient = false; } } private async handleDocumentChanged(data: SSEEventData): Promise { console.log(`[SSE] Document changed to: ${data.newDocumentId}`); - if (typeof data.bindingToken !== "string") { + if ( + typeof data.newDocumentId !== "string" || + typeof data.newDocumentName !== "string" || + typeof data.bindingToken !== "string" || + typeof data.boundRelativePath !== "string" || + typeof data.revision !== "string" + ) { + console.warn("[SSE] Ignoring incomplete documentChanged event"); return; } if (this.notificationManager) { @@ -300,6 +423,7 @@ export class DocumentManager { { documentId: data.newDocumentId, bindingToken: data.bindingToken, + boundRelativePath: data.boundRelativePath, revision: data.revision, }, data.newDocumentName, @@ -312,6 +436,7 @@ export class DocumentManager { data.bindingToken !== this.bindingToken || typeof data.markdown !== "string" || typeof data.baseMarkdown !== "string" || + typeof data.revision !== "string" || !this.editorManager ) { return; @@ -416,11 +541,12 @@ export class DocumentManager { } await this.editorManager.switchToDocument(documentId, content); - document.title = `${documentName} - AI-Enhanced Markdown Editor`; const documentPath = relativePath || documentName; + const displayPath = documentPath.replace(/\.md$/i, ""); + document.title = `${displayPath} - AI-Enhanced Markdown Editor`; const newUrl = `/document/${encodeDocumentPathForUrl(documentPath)}`; window.history.pushState( - { documentName: documentPath }, + { documentPath: displayPath }, document.title, newUrl, ); @@ -430,6 +556,7 @@ export class DocumentManager { binding: { documentId: string; bindingToken: string; + boundRelativePath: string; revision: unknown; }, documentName: string, @@ -440,18 +567,23 @@ export class DocumentManager { binding.bindingToken === this.bindingToken && binding.documentId === this.currentDocumentId ) { + this.currentBoundRelativePath = binding.boundRelativePath; this.adoptRevision(binding); return; } this.isBindingTransitionInProgress = true; try { - await this.handleDocumentChangeFromBackend( - binding.documentId, - documentName, - relativePath, - binding.bindingToken, - ); + // Unit tests and headless clients have no editor to switch. + // They still need to track the server-authoritative binding. + if (this.editorManager) { + await this.handleDocumentChangeFromBackend( + binding.documentId, + documentName, + relativePath, + binding.bindingToken, + ); + } this.adoptBinding(binding); } finally { this.isBindingTransitionInProgress = false; @@ -571,6 +703,9 @@ export class DocumentManager { } } + /** + * Persist serialized Markdown with the active binding and base revision. + */ public async saveDocument(editor?: Editor): Promise { try { if ( @@ -598,19 +733,13 @@ export class DocumentManager { }), }); - if (!response.ok) { - throw new Error(`Save failed: ${response.status}`); - } - - const result = await response.json(); - if ( - bindingVersion !== this.bindingVersion || - bindingToken !== this.bindingToken || - result.bindingToken !== bindingToken - ) { - throw new Error("Binding changed while saving"); - } - this.adoptRevision(result); + await this.reconcileDocumentWriteResponse( + response, + content, + "Save", + bindingToken, + bindingVersion, + ); console.log(` [DOCUMENT] Document saved successfully`); } catch (error) { console.error("[DOCUMENT] Failed to save document:", error); @@ -622,7 +751,6 @@ export class DocumentManager { } public async getMarkdownContent(editor: Editor): Promise { - if (!editor) return ""; return getMarkdownFromEditor(editor); } @@ -635,6 +763,7 @@ export class DocumentManager { if (response.ok) { const content = await response.text(); this.adoptRevisionHeader(response); + this.lastAutoSaveContent = content; return content; } else { return this.getDefaultContent(); @@ -708,21 +837,13 @@ export class DocumentManager { }), }); - if (!response.ok) { - throw new Error( - `Failed to set document content: ${response.status} ${response.statusText}`, - ); - } - - const result = await response.json(); - if ( - bindingVersion !== this.bindingVersion || - bindingToken !== this.bindingToken || - result.bindingToken !== bindingToken - ) { - throw new Error("Binding changed while updating content"); - } - this.adoptRevision(result); + await this.reconcileDocumentWriteResponse( + response, + content, + "Set document content", + bindingToken, + bindingVersion, + ); console.log(` [DOCUMENT] Document content updated successfully`); // Don't reload the whole page, just notify the editor will update via collaboration console.log( @@ -778,16 +899,26 @@ export class DocumentManager { } } - public async switchToDocument(documentName: string): Promise { + public async switchToDocument(documentPath: string): Promise { try { + if (this.currentBoundRelativePath !== null) { + const targetRelativePath = + ensureMarkdownExtension(documentPath); + if (this.currentBoundRelativePath === targetRelativePath) { + console.log( + `[DOCUMENT] Already bound to ${this.currentBoundRelativePath}; skipping /api/switch-document`, + ); + return; + } + } + const switchUrl = "/api/switch-document"; const startingBindingVersion = this.bindingVersion; - // Call server to switch document const response = await fetch(switchUrl, { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ documentPath: documentName }), + body: JSON.stringify({ documentPath }), }); if (!response.ok) { @@ -796,19 +927,37 @@ export class DocumentManager { ); } - const result = await response.json(); + const result = (await response.json()) as BindingStateData & { + documentName?: unknown; + }; if ( this.bindingVersion !== startingBindingVersion && this.bindingToken !== result.bindingToken ) { + console.warn( + `[DOCUMENT] Ignoring stale switch response for ${documentPath}`, + ); return; } - console.log(`[DOCUMENT] Server switched to: ${documentName}`); + if ( + typeof result.documentId !== "string" || + typeof result.bindingToken !== "string" || + typeof result.boundRelativePath !== "string" || + typeof result.revision !== "string" + ) { + throw new Error("Switch response is missing binding data"); + } + const documentName = + typeof result.documentName === "string" + ? result.documentName + : documentPath; + console.log(`[DOCUMENT] Server switched to: ${documentPath}`); await this.transitionToBinding( { documentId: result.documentId, bindingToken: result.bindingToken, + boundRelativePath: result.boundRelativePath, revision: result.revision, }, documentName, @@ -825,29 +974,114 @@ export class DocumentManager { if (typeof data.documentId === "string") { this.currentDocumentId = data.documentId; } - this.bindingToken = + this.currentBindingToken = typeof data.bindingToken === "string" ? data.bindingToken : null; - this.revision = + this.currentRevision = typeof data.revision === "string" ? data.revision : null; + this.currentBoundRelativePath = + typeof data.boundRelativePath === "string" + ? data.boundRelativePath + : null; } private adoptRevision(data: BindingStateData): void { if ( typeof data.bindingToken === "string" && - data.bindingToken !== this.bindingToken + data.bindingToken !== this.currentBindingToken ) { return; } if (typeof data.revision === "string") { - this.revision = data.revision; + this.currentRevision = data.revision; } } private adoptRevisionHeader(response: Response): void { const revision = response.headers.get("X-Content-Revision"); if (revision) { - this.revision = revision; + this.currentRevision = revision; + } + } + + private async parseDocumentWriteResponse( + response: Response, + ): Promise { + try { + const result: unknown = await response.json(); + if ( + typeof result === "object" && + result !== null && + !Array.isArray(result) + ) { + return result as DocumentWriteResponse; + } + } catch (error) { + console.warn( + `[DOCUMENT] Could not parse ${response.status} response body:`, + error, + ); + } + return undefined; + } + + private async reconcileDocumentWriteResponse( + response: Response, + attemptedContent: string, + operation: string, + expectedBindingToken: string, + expectedBindingVersion: number, + ): Promise { + const result = await this.parseDocumentWriteResponse(response); + if ( + expectedBindingVersion !== this.bindingVersion || + expectedBindingToken !== this.bindingToken + ) { + throw new Error(`Binding changed while ${operation.toLowerCase()}`); } + + if (response.ok) { + if (typeof result?.revision !== "string") { + throw new Error(`${operation} response is missing a revision`); + } + if ( + result?.bindingToken !== undefined && + result.bindingToken !== expectedBindingToken + ) { + throw new Error( + `${operation} response did not match the active binding`, + ); + } + this.adoptRevision(result ?? {}); + this.lastAutoSaveContent = attemptedContent; + this.lastConflictedAutoSaveContent = null; + return; + } + + if ( + response.status === 409 && + typeof result?.revision === "string" && + result.content === attemptedContent + ) { + this.adoptRevision({ revision: result.revision }); + this.lastAutoSaveContent = attemptedContent; + this.lastConflictedAutoSaveContent = null; + return; + } + + if (response.status === 409) { + this.lastConflictedAutoSaveContent = attemptedContent; + const detail = + typeof result?.error === "string" ? ` ${result.error}` : ""; + throw new DocumentWriteConflictError( + `${operation} conflict: the document changed on disk and was not overwritten.${detail}`, + ); + } + + const detail = + typeof result?.error === "string" ? ` ${result.error}` : ""; + throw new Error( + `${operation} failed: ${response.status} ${response.statusText}.${detail}`, + ); } private async hasUnsavedChanges(): Promise { diff --git a/ts/packages/agents/markdown/src/view/site/index.ts b/ts/packages/agents/markdown/src/view/site/index.ts index 848dd6a315..387416c5dc 100644 --- a/ts/packages/agents/markdown/src/view/site/index.ts +++ b/ts/packages/agents/markdown/src/view/site/index.ts @@ -36,8 +36,7 @@ document.addEventListener("DOMContentLoaded", async () => { }); async function initializeApplication(): Promise { - // Check if we have a document name in the URL - const documentName = parseDocumentPathFromUrl(window.location.pathname); + const documentPath = parseDocumentPathFromUrl(window.location.pathname); // Initialize managers editorManager = new EditorManager(); @@ -61,8 +60,8 @@ async function initializeApplication(): Promise { // Bind the initialized editor before opening SSE. Otherwise an early // serializer request can return empty content with a valid binding token. - if (documentName) { - await switchToDocument(documentName); + if (documentPath) { + await switchToDocument(documentPath); } await documentManager.initialize(); @@ -96,12 +95,11 @@ async function switchToDocument(documentName: string): Promise { } function setupBrowserHistoryHandling(): void { - // Handle browser back/forward navigation - window.addEventListener("popstate", async (event) => { - const documentName = parseDocumentPathFromUrl(window.location.pathname); - - if (documentName && event.state?.documentName !== documentName) { - await switchToDocument(documentName); + // Use the same nested-path parser for initial navigation and history. + window.addEventListener("popstate", async () => { + const documentPath = parseDocumentPathFromUrl(window.location.pathname); + if (documentPath) { + await switchToDocument(documentPath); } }); } diff --git a/ts/packages/agents/markdown/src/view/site/types.ts b/ts/packages/agents/markdown/src/view/site/types.ts index 5022bf3914..dff36c207a 100644 --- a/ts/packages/agents/markdown/src/view/site/types.ts +++ b/ts/packages/agents/markdown/src/view/site/types.ts @@ -81,7 +81,11 @@ export type NotificationType = "success" | "error" | "info"; export interface CollaborationInfo { websocketServerUrl: string; + // Human-readable display name (typically the file basename without + // the .md extension). Not safe to use as a room key. currentDocument: string; + // Opaque server-authoritative Yjs room identifier. Older servers may + // omit it, in which case collaboration falls back to the default room. currentDocumentId?: string; documents: number; totalClients: number; diff --git a/ts/packages/agents/markdown/src/view/site/ui/toolbar-manager.ts b/ts/packages/agents/markdown/src/view/site/ui/toolbar-manager.ts index 5430d94d36..05b55aa57e 100644 --- a/ts/packages/agents/markdown/src/view/site/ui/toolbar-manager.ts +++ b/ts/packages/agents/markdown/src/view/site/ui/toolbar-manager.ts @@ -3,6 +3,7 @@ import { DocumentManager } from "../core/document-manager"; import { getElementById } from "../utils"; +import { encodeDocumentPathForUrl } from "../../route/urlPath.js"; //import { editorViewCtx } from "@milkdown/core"; export class ToolbarManager { @@ -169,17 +170,19 @@ export class ToolbarManager { } const docInfo = await response.json(); - const documentName = docInfo.currentDocument || "live"; + const documentPath = + docInfo.boundRelativePath || docInfo.currentDocument || "live"; // Create shareable URL const baseUrl = window.location.origin; - const shareUrl = `${baseUrl}/document/${documentName}`; + const encodedPath = encodeDocumentPathForUrl(documentPath); + const shareUrl = `${baseUrl}/document/${encodedPath}`; // Copy to clipboard await navigator.clipboard.writeText(shareUrl); this.showNotification( - `🔗 Link copied: /document/${documentName}`, + `🔗 Link copied: /document/${encodedPath}`, "success", ); } catch (error) { @@ -189,8 +192,11 @@ export class ToolbarManager { try { const response = await fetch("/api/current-document"); const docInfo = await response.json(); - const documentName = docInfo.currentDocument || "live"; - const shareUrl = `${window.location.origin}/document/${documentName}`; + const documentPath = + docInfo.boundRelativePath || + docInfo.currentDocument || + "live"; + const shareUrl = `${window.location.origin}/document/${encodeDocumentPathForUrl(documentPath)}`; // Show URL in prompt as fallback prompt("Copy this shareable URL:", shareUrl); diff --git a/ts/packages/agents/markdown/src/view/site/utils.ts b/ts/packages/agents/markdown/src/view/site/utils.ts index 1998b738b5..53215f5939 100644 --- a/ts/packages/agents/markdown/src/view/site/utils.ts +++ b/ts/packages/agents/markdown/src/view/site/utils.ts @@ -93,9 +93,11 @@ export function hasClass(element: HTMLElement, className: string): boolean { * This ensures we get accurate markdown formatting and position information */ export async function getMarkdownFromEditor(editor: Editor): Promise { - if (!editor) return ""; + if (!editor) { + throw new Error("Cannot serialize Markdown without an editor"); + } - return new Promise((resolve) => { + return new Promise((resolve, reject) => { try { editor.action((ctx) => { const view = ctx.get(editorViewCtx); @@ -104,12 +106,11 @@ export async function getMarkdownFromEditor(editor: Editor): Promise { resolve(markdown); }); } catch (error) { - console.warn("Failed to serialize markdown from editor:", error); - // Fallback to text content if serializer fails - editor.action((ctx) => { - const view = ctx.get(editorViewCtx); - resolve(view.state.doc.textContent || ""); - }); + reject( + error instanceof Error + ? error + : new Error("Failed to serialize Markdown"), + ); } }); } diff --git a/ts/packages/agents/markdown/test/browserPersistence.spec.ts b/ts/packages/agents/markdown/test/browserPersistence.spec.ts new file mode 100644 index 0000000000..125ea23536 --- /dev/null +++ b/ts/packages/agents/markdown/test/browserPersistence.spec.ts @@ -0,0 +1,340 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +import { fileURLToPath } from "node:url"; +import { jest } from "@jest/globals"; +import { createServer, type ViteDevServer } from "vite"; + +const packageRoot = fileURLToPath(new URL("../../", import.meta.url)); + +describe("browser document persistence", () => { + let vite: ViteDevServer; + let DocumentManager: new () => any; + let CollaborationManager: new () => any; + const originalFetch = globalThis.fetch; + + beforeAll(async () => { + vite = await createServer({ + root: packageRoot, + appType: "custom", + logLevel: "silent", + server: { middlewareMode: true }, + }); + ({ DocumentManager } = await vite.ssrLoadModule( + "/src/view/site/core/document-manager.ts", + )); + ({ CollaborationManager } = await vite.ssrLoadModule( + "/src/view/site/core/collaboration-manager.ts", + )); + }); + + beforeEach(() => { + jest.spyOn(console, "log").mockImplementation(() => {}); + jest.spyOn(console, "warn").mockImplementation(() => {}); + jest.spyOn(console, "error").mockImplementation(() => {}); + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + jest.restoreAllMocks(); + }); + + afterAll(async () => { + await vite.close(); + }); + + test("autosave and saveDocument persist serializer Markdown, including formatting-only edits", async () => { + let markdown = + "# Heading\n\nParagraph with **bold** text.\n\n```ts\nconst x = 1;\n```\n"; + const editor = createEditor( + () => markdown, + "HeadingParagraph with bold text.const x = 1;", + ); + const requests: Array> = []; + globalThis.fetch = (async (_input, init) => { + requests.push(JSON.parse(init?.body as string)); + return Response.json({ revision: `revision-${requests.length}` }); + }) as typeof fetch; + + const manager = new DocumentManager(); + manager.editorManager = { getEditor: () => editor }; + manager.isPrimaryClient = true; + manager.currentBindingToken = "binding-1"; + manager.currentDocumentId = "binding-1"; + manager.currentRevision = "revision-0"; + + await manager.performAutoSave(); + markdown = + "# Heading\n\nParagraph with *bold* text.\n\n```ts\nconst x = 1;\n```\n"; + await manager.performAutoSave(); + await manager.saveDocument(editor); + + expect(requests).toHaveLength(3); + expect(requests[0]).toMatchObject({ + content: + "# Heading\n\nParagraph with **bold** text.\n\n```ts\nconst x = 1;\n```\n", + bindingToken: "binding-1", + expectedRevision: "revision-0", + }); + expect(requests[1]).toMatchObject({ + content: + "# Heading\n\nParagraph with *bold* text.\n\n```ts\nconst x = 1;\n```\n", + expectedRevision: "revision-1", + }); + expect(requests[2]).toMatchObject({ + content: + "# Heading\n\nParagraph with *bold* text.\n\n```ts\nconst x = 1;\n```\n", + expectedRevision: "revision-2", + }); + }); + + test("serializer failure aborts persistence instead of falling back to textContent", async () => { + const editor = { + action(callback: (ctx: { get: () => unknown }) => void): void { + let getCount = 0; + callback({ + get: () => { + if (getCount++ === 0) { + return { + state: { + doc: { textContent: "formatting was lost" }, + }, + }; + } + throw new Error("serializer unavailable"); + }, + }); + }, + }; + const fetchMock = jest.fn(); + globalThis.fetch = fetchMock as typeof fetch; + + const manager = new DocumentManager(); + manager.currentBindingToken = "binding-1"; + manager.currentRevision = "revision-0"; + + await expect(manager.saveDocument(editor)).rejects.toThrow( + "serializer unavailable", + ); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + test("adopts current revisions from autosave and primary promotion events", async () => { + const manager = new DocumentManager(); + manager.currentBindingToken = "binding-1"; + manager.currentRevision = "revision-0"; + + await manager.handleSSEEvent({ + type: "autoSave", + bindingToken: "binding-1", + revision: "revision-1", + }); + expect(manager.currentRevision).toBe("revision-1"); + + await manager.handleSSEEvent({ + type: "autoSave", + bindingToken: "stale-binding", + revision: "wrong-revision", + }); + expect(manager.currentRevision).toBe("revision-1"); + + await manager.handleSSEEvent({ + type: "primaryElected", + bindingToken: "stale-binding", + revision: "wrong-revision", + }); + expect(manager.isPrimaryClient).toBe(false); + expect(manager.currentRevision).toBe("revision-1"); + + await manager.handleSSEEvent({ + type: "primaryElected", + bindingToken: "binding-1", + revision: "revision-2", + }); + expect(manager.isPrimaryClient).toBe(true); + expect(manager.currentRevision).toBe("revision-2"); + }); + + test("reconciles a 409 only when the same content is already on disk", async () => { + const markdown = "# Shared edit\n"; + const editor = createEditor(() => markdown, "Shared edit"); + const fetchMock = jest.fn(async () => + Response.json( + { + error: "Document content changed since it was loaded.", + content: markdown, + revision: "revision-from-primary", + }, + { status: 409 }, + ), + ); + globalThis.fetch = fetchMock as typeof fetch; + + const manager = new DocumentManager(); + manager.editorManager = { getEditor: () => editor }; + manager.currentBindingToken = "binding-1"; + manager.currentDocumentId = "binding-1"; + manager.currentRevision = "stale-revision"; + + await manager.performAutoSave(); + await manager.performAutoSave(); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(manager.currentRevision).toBe("revision-from-primary"); + expect(manager.lastAutoSaveContent).toBe(markdown); + }); + + test("surfaces a divergent or malformed 409 without retrying or adopting its revision", async () => { + const markdown = "# Local edit\n"; + const editor = createEditor(() => markdown, "Local edit"); + const fetchMock = jest + .fn<() => Promise>() + .mockResolvedValueOnce( + Response.json( + { + error: "Document content changed since it was loaded.", + content: "# Newer disk edit\n", + revision: "newer-disk-revision", + }, + { status: 409 }, + ), + ) + .mockResolvedValue( + new Response("not-json", { + status: 409, + statusText: "Conflict", + }), + ); + globalThis.fetch = fetchMock as typeof fetch; + + const manager = new DocumentManager(); + manager.editorManager = { getEditor: () => editor }; + manager.currentBindingToken = "binding-1"; + manager.currentDocumentId = "binding-1"; + manager.currentRevision = "local-base-revision"; + + await manager.performAutoSave(); + await manager.performAutoSave(); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(manager.currentRevision).toBe("local-base-revision"); + expect(console.error).toHaveBeenCalledWith( + "[AUTO-SAVE] Error during auto-save:", + expect.objectContaining({ name: "DocumentWriteConflictError" }), + ); + + manager.lastConflictedAutoSaveContent = null; + await expect(manager.saveDocument(editor)).rejects.toThrow( + "changed on disk and was not overwritten", + ); + expect(manager.currentRevision).toBe("local-base-revision"); + }); + + test("matching bootstrap path skips a redundant switch request", async () => { + const fetchMock = jest.fn(async () => + Response.json({ + boundRelativePath: "team/2025/plan.md", + }), + ); + globalThis.fetch = fetchMock as typeof fetch; + + const manager = new DocumentManager(); + try { + await manager.initialize(); + await manager.switchToDocument("team/2025/plan.md"); + } finally { + manager.destroy(); + } + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(fetchMock).toHaveBeenCalledWith("/api/current-document"); + }); + + test("ignores switch responses superseded by a newer binding", async () => { + let resolveSwitch!: (response: Response) => void; + globalThis.fetch = jest.fn( + () => + new Promise((resolve) => { + resolveSwitch = resolve; + }), + ) as typeof fetch; + + const manager = new DocumentManager(); + const switching = manager.switchToDocument("old.md"); + await Promise.resolve(); + await manager.handleSSEEvent({ + type: "bindingBootstrap", + bindingToken: "new-binding", + documentId: "new-room", + boundRelativePath: "new.md", + revision: "new-revision", + }); + resolveSwitch( + Response.json({ + bindingToken: "old-binding", + documentId: "old-room", + boundRelativePath: "old.md", + revision: "old-revision", + content: "old content", + }), + ); + await switching; + + expect(manager.currentBindingToken).toBe("new-binding"); + expect(manager.currentDocumentId).toBe("new-room"); + expect(manager.currentBoundRelativePath).toBe("new.md"); + expect(manager.currentRevision).toBe("new-revision"); + }); + + test("ignores incomplete binding bootstrap data", async () => { + const manager = new DocumentManager(); + manager.currentBindingToken = "binding-1"; + manager.currentDocumentId = "room-1"; + manager.currentBoundRelativePath = "one.md"; + + await manager.handleSSEEvent({ + type: "bindingBootstrap", + bindingToken: "binding-2", + documentId: "room-2", + }); + + expect(manager.currentBindingToken).toBe("binding-1"); + expect(manager.currentDocumentId).toBe("room-1"); + expect(manager.currentBoundRelativePath).toBe("one.md"); + }); + + test("collaboration config uses the server room id, not the basename", async () => { + globalThis.fetch = (async () => + Response.json({ + websocketServerUrl: "ws://127.0.0.1:4321", + currentDocumentId: "opaque-binding-token", + currentDocument: "note", + documents: 1, + totalClients: 0, + })) as typeof fetch; + + const manager = new CollaborationManager(); + const config = await manager.getCollaborationConfig(); + + expect(config.documentId).toBe("opaque-binding-token"); + }); +}); + +function createEditor( + getMarkdown: () => string, + textContent: string, +): { + action(callback: (ctx: { get: () => unknown }) => void): void; +} { + return { + action(callback): void { + let getCount = 0; + callback({ + get: () => + getCount++ === 0 + ? { state: { doc: { textContent } } } + : () => getMarkdown(), + }); + }, + }; +} diff --git a/ts/packages/agents/markdown/test/viewService.spec.ts b/ts/packages/agents/markdown/test/viewService.spec.ts index 4500f2bb2b..8200af4322 100644 --- a/ts/packages/agents/markdown/test/viewService.spec.ts +++ b/ts/packages/agents/markdown/test/viewService.spec.ts @@ -240,6 +240,7 @@ describe("markdown view service binding isolation", () => { bindingToken: first.bindingToken, documentId: first.bindingToken, boundRelativePath: "first.md", + clientRole: "primary", }); const switchedResponse = await fetch( @@ -332,6 +333,39 @@ describe("markdown view service binding isolation", () => { } }); + test("elects the next SSE client when the primary disconnects", async () => { + const port = await start({ "shared.md": "shared" }); + const bound = await bind("shared.md", "bind-shared"); + const primaryController = new AbortController(); + const secondaryController = new AbortController(); + const primaryEvents = await openSseEvents( + `http://127.0.0.1:${port}/events`, + primaryController.signal, + ); + const secondaryEvents = await openSseEvents( + `http://127.0.0.1:${port}/events`, + secondaryController.signal, + ); + try { + expect( + await waitForEvent(primaryEvents, "bindingBootstrap"), + ).toMatchObject({ clientRole: "primary" }); + expect( + await waitForEvent(secondaryEvents, "bindingBootstrap"), + ).toMatchObject({ clientRole: "secondary" }); + + primaryController.abort(); + expect( + await waitForEvent(secondaryEvents, "primaryElected"), + ).toMatchObject({ + bindingToken: bound.bindingToken, + }); + } finally { + primaryController.abort(); + secondaryController.abort(); + } + }); + test("evicts idle rooms after binding rotation", async () => { const port = await start({ "a.md": "A", "b.md": "B" }); await bind("a.md", "room-a"); From 532a5e9ea1cac8e78fd264a2b0e71f74a85269ba Mon Sep 17 00:00:00 2001 From: George Ng Date: Thu, 10 Sep 2026 23:17:27 -0700 Subject: [PATCH 2/4] style(markdown): satisfy browser persistence lint Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../markdown/src/view/site/core/document-manager.ts | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/ts/packages/agents/markdown/src/view/site/core/document-manager.ts b/ts/packages/agents/markdown/src/view/site/core/document-manager.ts index 8f03e1d37b..fa9cee1a6c 100644 --- a/ts/packages/agents/markdown/src/view/site/core/document-manager.ts +++ b/ts/packages/agents/markdown/src/view/site/core/document-manager.ts @@ -211,13 +211,6 @@ export class DocumentManager { bindingToken, bindingVersion, ); - if (response.ok) { - console.log("[AUTO-SAVE] Successfully saved document"); - } else { - console.log( - "[AUTO-SAVE] Reconciled with content already persisted by another client", - ); - } } catch (error) { console.error("[AUTO-SAVE] Error during auto-save:", error); if ( @@ -335,7 +328,6 @@ export class DocumentManager { } this.isPrimaryClient = true; this.adoptRevision(data); - console.log("[SSE] Promoted to PRIMARY for auto-save"); break; case "operationsBeingApplied": @@ -905,9 +897,6 @@ export class DocumentManager { const targetRelativePath = ensureMarkdownExtension(documentPath); if (this.currentBoundRelativePath === targetRelativePath) { - console.log( - `[DOCUMENT] Already bound to ${this.currentBoundRelativePath}; skipping /api/switch-document`, - ); return; } } @@ -951,7 +940,6 @@ export class DocumentManager { typeof result.documentName === "string" ? result.documentName : documentPath; - console.log(`[DOCUMENT] Server switched to: ${documentPath}`); await this.transitionToBinding( { From 5346d82c3ea61974a2b9fd67d9e534c873819a41 Mon Sep 17 00:00:00 2001 From: George Ng Date: Thu, 10 Sep 2026 23:47:18 -0700 Subject: [PATCH 3/4] fix(markdown): harden browser persistence races Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/view/site/core/document-manager.ts | 60 +++++++--- .../agents/markdown/src/view/site/index.ts | 11 +- .../markdown/test/browserPersistence.spec.ts | 110 +++++++++++++++++- 3 files changed, 157 insertions(+), 24 deletions(-) diff --git a/ts/packages/agents/markdown/src/view/site/core/document-manager.ts b/ts/packages/agents/markdown/src/view/site/core/document-manager.ts index fa9cee1a6c..edbd076598 100644 --- a/ts/packages/agents/markdown/src/view/site/core/document-manager.ts +++ b/ts/packages/agents/markdown/src/view/site/core/document-manager.ts @@ -66,6 +66,7 @@ export class DocumentManager { private revision: string | null = null; private bindingVersion = 0; private currentBoundRelativePath: string | null = null; + private historyNavigationTarget: string | null = null; // Keep the names introduced by the persistence change as aliases while // retaining the parent's binding state machine and transition guards. @@ -316,18 +317,13 @@ export class DocumentManager { break; case "primaryElected": - if ( - typeof data.bindingToken !== "string" || - data.bindingToken !== this.bindingToken || - typeof data.revision !== "string" - ) { + if (data.bindingToken !== this.bindingToken) { console.warn( "[SSE] Ignoring incomplete or stale primaryElected event", ); break; } this.isPrimaryClient = true; - this.adoptRevision(data); break; case "operationsBeingApplied": @@ -420,6 +416,7 @@ export class DocumentManager { }, data.newDocumentName, data.boundRelativePath, + this.consumeHistoryNavigation(data.boundRelativePath), ); } @@ -506,6 +503,7 @@ export class DocumentManager { documentName: string, relativePath?: string, expectedBindingToken?: string, + updateHistory = true, ): Promise { console.log( `[DOCUMENT] Backend switched to: ${documentName}, reconnecting frontend...`, @@ -532,16 +530,22 @@ export class DocumentManager { throw new Error("Editor is not ready for a binding transition"); } await this.editorManager.switchToDocument(documentId, content); + this.lastAutoSaveContent = await this.getMarkdownContent( + this.editorManager.getEditor(), + ); + this.lastConflictedAutoSaveContent = null; const documentPath = relativePath || documentName; const displayPath = documentPath.replace(/\.md$/i, ""); document.title = `${displayPath} - AI-Enhanced Markdown Editor`; - const newUrl = `/document/${encodeDocumentPathForUrl(documentPath)}`; - window.history.pushState( - { documentPath: displayPath }, - document.title, - newUrl, - ); + if (updateHistory) { + const newUrl = `/document/${encodeDocumentPathForUrl(documentPath)}`; + window.history.pushState( + { documentPath: displayPath }, + document.title, + newUrl, + ); + } } private async transitionToBinding( @@ -553,6 +557,7 @@ export class DocumentManager { }, documentName: string, relativePath?: string, + updateHistory = true, ): Promise { const transition = this.bindingTransitionQueue.then(async () => { if ( @@ -574,6 +579,7 @@ export class DocumentManager { documentName, relativePath, binding.bindingToken, + updateHistory, ); } this.adoptBinding(binding); @@ -891,15 +897,20 @@ export class DocumentManager { } } - public async switchToDocument(documentPath: string): Promise { + public async switchToDocument( + documentPath: string, + updateHistory = true, + ): Promise { + const targetRelativePath = ensureMarkdownExtension(documentPath); try { if (this.currentBoundRelativePath !== null) { - const targetRelativePath = - ensureMarkdownExtension(documentPath); if (this.currentBoundRelativePath === targetRelativePath) { return; } } + if (!updateHistory) { + this.historyNavigationTarget = targetRelativePath; + } const switchUrl = "/api/switch-document"; const startingBindingVersion = this.bindingVersion; @@ -950,11 +961,24 @@ export class DocumentManager { }, documentName, result.boundRelativePath, + updateHistory, ); } catch (error) { console.error("[DOCUMENT] Failed to switch document:", error); throw error; + } finally { + if (this.historyNavigationTarget === targetRelativePath) { + this.historyNavigationTarget = null; + } + } + } + + private consumeHistoryNavigation(relativePath: string): boolean { + if (this.historyNavigationTarget !== relativePath) { + return true; } + this.historyNavigationTarget = null; + return false; } private adoptBinding(data: BindingStateData): void { @@ -1056,7 +1080,11 @@ export class DocumentManager { return; } - if (response.status === 409) { + if ( + response.status === 409 && + typeof result?.revision === "string" && + typeof result.content === "string" + ) { this.lastConflictedAutoSaveContent = attemptedContent; const detail = typeof result?.error === "string" ? ` ${result.error}` : ""; diff --git a/ts/packages/agents/markdown/src/view/site/index.ts b/ts/packages/agents/markdown/src/view/site/index.ts index 387416c5dc..10a7eb74bc 100644 --- a/ts/packages/agents/markdown/src/view/site/index.ts +++ b/ts/packages/agents/markdown/src/view/site/index.ts @@ -61,7 +61,7 @@ async function initializeApplication(): Promise { // Bind the initialized editor before opening SSE. Otherwise an early // serializer request can return empty content with a valid binding token. if (documentPath) { - await switchToDocument(documentPath); + await switchToDocument(documentPath, false); } await documentManager.initialize(); @@ -78,10 +78,13 @@ async function initializeApplication(): Promise { console.log("[APP] Application initialized successfully"); } -async function switchToDocument(documentName: string): Promise { +async function switchToDocument( + documentName: string, + updateHistory = true, +): Promise { try { if (documentManager) { - await documentManager.switchToDocument(documentName); + await documentManager.switchToDocument(documentName, updateHistory); console.log( `[APP] Successfully switched to document: ${documentName}`, ); @@ -99,7 +102,7 @@ function setupBrowserHistoryHandling(): void { window.addEventListener("popstate", async () => { const documentPath = parseDocumentPathFromUrl(window.location.pathname); if (documentPath) { - await switchToDocument(documentPath); + await switchToDocument(documentPath, false); } }); } diff --git a/ts/packages/agents/markdown/test/browserPersistence.spec.ts b/ts/packages/agents/markdown/test/browserPersistence.spec.ts index 125ea23536..a66bfcf668 100644 --- a/ts/packages/agents/markdown/test/browserPersistence.spec.ts +++ b/ts/packages/agents/markdown/test/browserPersistence.spec.ts @@ -119,7 +119,7 @@ describe("browser document persistence", () => { expect(fetchMock).not.toHaveBeenCalled(); }); - test("adopts current revisions from autosave and primary promotion events", async () => { + test("promotes only the active binding without adopting a newer disk revision", async () => { const manager = new DocumentManager(); manager.currentBindingToken = "binding-1"; manager.currentRevision = "revision-0"; @@ -152,7 +152,16 @@ describe("browser document persistence", () => { revision: "revision-2", }); expect(manager.isPrimaryClient).toBe(true); - expect(manager.currentRevision).toBe("revision-2"); + expect(manager.currentRevision).toBe("revision-1"); + + const unbound = new DocumentManager(); + await unbound.handleSSEEvent({ + type: "primaryElected", + bindingToken: null, + revision: null, + }); + expect(unbound.isPrimaryClient).toBe(true); + expect(unbound.currentRevision).toBeNull(); }); test("reconciles a 409 only when the same content is already on disk", async () => { @@ -184,7 +193,7 @@ describe("browser document persistence", () => { expect(manager.lastAutoSaveContent).toBe(markdown); }); - test("surfaces a divergent or malformed 409 without retrying or adopting its revision", async () => { + test("surfaces a divergent conflict without retrying or adopting its revision", async () => { const markdown = "# Local edit\n"; const editor = createEditor(() => markdown, "Local edit"); const fetchMock = jest @@ -225,11 +234,104 @@ describe("browser document persistence", () => { manager.lastConflictedAutoSaveContent = null; await expect(manager.saveDocument(editor)).rejects.toThrow( - "changed on disk and was not overwritten", + "Save failed: 409 Conflict", ); expect(manager.currentRevision).toBe("local-base-revision"); }); + test("retries autosave after transient binding contention", async () => { + const markdown = "# Pending edit\n"; + const editor = createEditor(() => markdown, "Pending edit"); + const fetchMock = jest + .fn<() => Promise>() + .mockResolvedValueOnce( + Response.json( + { + error: "Another document update is already in progress for this binding", + }, + { status: 409 }, + ), + ) + .mockResolvedValueOnce( + Response.json({ + bindingToken: "binding-1", + revision: "revision-1", + }), + ); + globalThis.fetch = fetchMock as typeof fetch; + + const manager = new DocumentManager(); + manager.editorManager = { getEditor: () => editor }; + manager.currentBindingToken = "binding-1"; + manager.currentDocumentId = "binding-1"; + manager.currentRevision = "revision-0"; + + await manager.performAutoSave(); + await manager.performAutoSave(); + + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(manager.lastConflictedAutoSaveContent).toBeNull(); + expect(manager.lastAutoSaveContent).toBe(markdown); + expect(manager.currentRevision).toBe("revision-1"); + }); + + test("records the loaded editor baseline without adding history during navigation", async () => { + const markdown = "# Serialized baseline\n"; + const editor = createEditor(() => markdown, "Serialized baseline"); + const pushState = jest.fn(); + const originalWindow = Object.getOwnPropertyDescriptor( + globalThis, + "window", + ); + const originalDocument = Object.getOwnPropertyDescriptor( + globalThis, + "document", + ); + Object.defineProperty(globalThis, "window", { + configurable: true, + value: { history: { pushState } }, + }); + Object.defineProperty(globalThis, "document", { + configurable: true, + value: { title: "" }, + }); + globalThis.fetch = jest.fn(async (input) => { + if (input === "/api/switch-document") { + return Response.json({ + bindingToken: "binding-1", + documentId: "room-1", + boundRelativePath: "team/plan.md", + documentName: "plan", + revision: "revision-1", + }); + } + return new Response("# Raw baseline\n"); + }) as typeof fetch; + + const manager = new DocumentManager(); + manager.editorManager = { + getEditor: () => editor, + switchToDocument: jest.fn(async () => {}), + }; + try { + await manager.switchToDocument("team/plan.md", false); + } finally { + if (originalWindow) { + Object.defineProperty(globalThis, "window", originalWindow); + } else { + Reflect.deleteProperty(globalThis, "window"); + } + if (originalDocument) { + Object.defineProperty(globalThis, "document", originalDocument); + } else { + Reflect.deleteProperty(globalThis, "document"); + } + } + + expect(manager.lastAutoSaveContent).toBe(markdown); + expect(pushState).not.toHaveBeenCalled(); + }); + test("matching bootstrap path skips a redundant switch request", async () => { const fetchMock = jest.fn(async () => Response.json({ From 874caeee57958242da41c2e44ac6c3a1357b0be2 Mon Sep 17 00:00:00 2001 From: George Ng Date: Fri, 11 Sep 2026 00:06:48 -0700 Subject: [PATCH 4/4] fix(markdown): preserve browser revision ownership Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/view/site/core/document-manager.ts | 16 +- .../markdown/test/browserPersistence.spec.ts | 146 +++++++++++++++++- 2 files changed, 156 insertions(+), 6 deletions(-) diff --git a/ts/packages/agents/markdown/src/view/site/core/document-manager.ts b/ts/packages/agents/markdown/src/view/site/core/document-manager.ts index edbd076598..9f8fadd612 100644 --- a/ts/packages/agents/markdown/src/view/site/core/document-manager.ts +++ b/ts/packages/agents/markdown/src/view/site/core/document-manager.ts @@ -56,6 +56,7 @@ export class DocumentManager { private eventSource: EventSource | null = null; private sseEventQueue: Promise = Promise.resolve(); private bindingTransitionQueue: Promise = Promise.resolve(); + private switchRequestQueue: Promise = Promise.resolve(); private autoSaveTimer: NodeJS.Timeout | null = null; private isPrimaryClient = false; private isBindingTransitionInProgress = false; @@ -291,6 +292,7 @@ export class DocumentManager { case "autoSave": console.log(`[SSE] Auto-save completed for: ${data.filePath}`); if ( + this.isPrimaryClient && typeof data.bindingToken === "string" && data.bindingToken === this.bindingToken ) { @@ -444,6 +446,7 @@ export class DocumentManager { const currentMarkdown = await this.getMarkdownContent(editor); if (currentMarkdown === data.markdown) { this.lastAutoSaveContent = data.markdown; + this.lastConflictedAutoSaveContent = null; this.adoptRevision(data); return; } @@ -456,6 +459,7 @@ export class DocumentManager { await this.editorManager.setContent(data.markdown); this.adoptRevision(data); this.lastAutoSaveContent = data.markdown; + this.lastConflictedAutoSaveContent = null; } private async handleLLMOperations(data: SSEEventData): Promise { @@ -565,7 +569,6 @@ export class DocumentManager { binding.documentId === this.currentDocumentId ) { this.currentBoundRelativePath = binding.boundRelativePath; - this.adoptRevision(binding); return; } @@ -900,6 +903,17 @@ export class DocumentManager { public async switchToDocument( documentPath: string, updateHistory = true, + ): Promise { + const request = this.switchRequestQueue.then(() => + this.performDocumentSwitch(documentPath, updateHistory), + ); + this.switchRequestQueue = request.catch(() => undefined); + await request; + } + + private async performDocumentSwitch( + documentPath: string, + updateHistory: boolean, ): Promise { const targetRelativePath = ensureMarkdownExtension(documentPath); try { diff --git a/ts/packages/agents/markdown/test/browserPersistence.spec.ts b/ts/packages/agents/markdown/test/browserPersistence.spec.ts index a66bfcf668..a0a3ce78a8 100644 --- a/ts/packages/agents/markdown/test/browserPersistence.spec.ts +++ b/ts/packages/agents/markdown/test/browserPersistence.spec.ts @@ -119,7 +119,7 @@ describe("browser document persistence", () => { expect(fetchMock).not.toHaveBeenCalled(); }); - test("promotes only the active binding without adopting a newer disk revision", async () => { + test("only the primary adopts autosave revisions and promotion does not advance them", async () => { const manager = new DocumentManager(); manager.currentBindingToken = "binding-1"; manager.currentRevision = "revision-0"; @@ -129,14 +129,14 @@ describe("browser document persistence", () => { bindingToken: "binding-1", revision: "revision-1", }); - expect(manager.currentRevision).toBe("revision-1"); + expect(manager.currentRevision).toBe("revision-0"); await manager.handleSSEEvent({ type: "autoSave", bindingToken: "stale-binding", revision: "wrong-revision", }); - expect(manager.currentRevision).toBe("revision-1"); + expect(manager.currentRevision).toBe("revision-0"); await manager.handleSSEEvent({ type: "primaryElected", @@ -144,7 +144,7 @@ describe("browser document persistence", () => { revision: "wrong-revision", }); expect(manager.isPrimaryClient).toBe(false); - expect(manager.currentRevision).toBe("revision-1"); + expect(manager.currentRevision).toBe("revision-0"); await manager.handleSSEEvent({ type: "primaryElected", @@ -152,7 +152,14 @@ describe("browser document persistence", () => { revision: "revision-2", }); expect(manager.isPrimaryClient).toBe(true); - expect(manager.currentRevision).toBe("revision-1"); + expect(manager.currentRevision).toBe("revision-0"); + + await manager.handleSSEEvent({ + type: "autoSave", + bindingToken: "binding-1", + revision: "revision-3", + }); + expect(manager.currentRevision).toBe("revision-3"); const unbound = new DocumentManager(); await unbound.handleSSEEvent({ @@ -164,6 +171,27 @@ describe("browser document persistence", () => { expect(unbound.currentRevision).toBeNull(); }); + test("same-binding bootstrap does not adopt an unseen disk revision", async () => { + const manager = new DocumentManager(); + manager.currentBindingToken = "binding-1"; + manager.currentDocumentId = "room-1"; + manager.currentRevision = "revision-0"; + manager.currentBoundRelativePath = "one.md"; + + await manager.handleSSEEvent({ + type: "bindingBootstrap", + bindingToken: "binding-1", + documentId: "room-1", + boundRelativePath: "one.md", + documentName: "one", + revision: "revision-from-disk", + clientRole: "primary", + }); + + expect(manager.isPrimaryClient).toBe(true); + expect(manager.currentRevision).toBe("revision-0"); + }); + test("reconciles a 409 only when the same content is already on disk", async () => { const markdown = "# Shared edit\n"; const editor = createEditor(() => markdown, "Shared edit"); @@ -332,6 +360,114 @@ describe("browser document persistence", () => { expect(pushState).not.toHaveBeenCalled(); }); + test("serializes rapid history switches instead of skipping the final target", async () => { + const editor = createEditor(() => "# Content\n", "Content"); + const originalWindow = Object.getOwnPropertyDescriptor( + globalThis, + "window", + ); + const originalDocument = Object.getOwnPropertyDescriptor( + globalThis, + "document", + ); + Object.defineProperty(globalThis, "window", { + configurable: true, + value: { history: { pushState: jest.fn() } }, + }); + Object.defineProperty(globalThis, "document", { + configurable: true, + value: { title: "" }, + }); + + let resolveFirstSwitch!: (response: Response) => void; + const fetchMock = jest.fn( + async (input: string | URL | Request, init?: RequestInit) => { + if (input !== "/api/switch-document") { + return new Response("# Content\n"); + } + const request = JSON.parse(init?.body as string) as { + documentPath: string; + }; + if (request.documentPath === "a.md") { + return new Promise((resolve) => { + resolveFirstSwitch = resolve; + }); + } + return Response.json({ + bindingToken: "binding-b-new", + documentId: "room-b-new", + boundRelativePath: "b.md", + documentName: "b", + revision: "revision-b-new", + }); + }, + ); + globalThis.fetch = fetchMock as typeof fetch; + + const manager = new DocumentManager(); + manager.editorManager = { + getEditor: () => editor, + switchToDocument: jest.fn(async () => {}), + }; + manager.currentBindingToken = "binding-b-old"; + manager.currentDocumentId = "room-b-old"; + manager.currentRevision = "revision-b-old"; + manager.currentBoundRelativePath = "b.md"; + + try { + const back = manager.switchToDocument("a.md", false); + await Promise.resolve(); + const forward = manager.switchToDocument("b.md", false); + resolveFirstSwitch( + Response.json({ + bindingToken: "binding-a", + documentId: "room-a", + boundRelativePath: "a.md", + documentName: "a", + revision: "revision-a", + }), + ); + await Promise.all([back, forward]); + } finally { + if (originalWindow) { + Object.defineProperty(globalThis, "window", originalWindow); + } else { + Reflect.deleteProperty(globalThis, "window"); + } + if (originalDocument) { + Object.defineProperty(globalThis, "document", originalDocument); + } else { + Reflect.deleteProperty(globalThis, "document"); + } + } + + expect(manager.currentBoundRelativePath).toBe("b.md"); + expect(manager.currentBindingToken).toBe("binding-b-new"); + expect(fetchMock).toHaveBeenCalledTimes(4); + }); + + test("accepted snapshots clear suppression from an earlier conflict", async () => { + const markdown = "# Persisted by agent\n"; + const editor = createEditor(() => markdown, "Persisted by agent"); + const manager = new DocumentManager(); + manager.editorManager = { getEditor: () => editor }; + manager.currentBindingToken = "binding-1"; + manager.currentRevision = "revision-0"; + manager.lastConflictedAutoSaveContent = "# Earlier conflict\n"; + + await manager.handleSSEEvent({ + type: "documentSnapshot", + bindingToken: "binding-1", + markdown, + baseMarkdown: "# Old baseline\n", + revision: "revision-1", + }); + + expect(manager.lastConflictedAutoSaveContent).toBeNull(); + expect(manager.lastAutoSaveContent).toBe(markdown); + expect(manager.currentRevision).toBe("revision-1"); + }); + test("matching bootstrap path skips a redundant switch request", async () => { const fetchMock = jest.fn(async () => Response.json({