From 5f8d45e5fb1782081bd72f120b932ac6030fc0dc Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 27 Aug 2026 14:46:21 -0700 Subject: [PATCH 01/17] fix(annotate): fall back to the startup snapshot when the HTML root is unreadable readRootHtml guarded only existence and the 2 MB cap, so a root replaced by a directory or stripped of read permission made the read throw: the Pi server left /api/plan unanswered (the tab hung on reload) and the Bun server answered 500. Both runtimes now treat an unreadable root exactly like a missing one and serve the startup snapshot with its version-diff fields. Tests on both sides cover the directory swap and, where the runner is not root, a chmod 000 file. --- .../server/serverAnnotate-root-html.test.ts | 89 ++++++++++++++++++- apps/pi-extension/server/serverAnnotate.ts | 19 ++-- packages/server/annotate.test.ts | 82 ++++++++++++++++- packages/server/annotate.ts | 18 ++-- 4 files changed, 196 insertions(+), 12 deletions(-) diff --git a/apps/pi-extension/server/serverAnnotate-root-html.test.ts b/apps/pi-extension/server/serverAnnotate-root-html.test.ts index b5983c045..681734e06 100644 --- a/apps/pi-extension/server/serverAnnotate-root-html.test.ts +++ b/apps/pi-extension/server/serverAnnotate-root-html.test.ts @@ -12,7 +12,7 @@ */ import { afterAll, afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdtempSync, realpathSync, rmSync, unlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdirSync, mkdtempSync, realpathSync, rmSync, unlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { startAnnotateServer } from "./serverAnnotate.ts"; @@ -154,4 +154,91 @@ describe("pi annotate server: local rendered-HTML root freshness", () => { server.stop(); } }); + + // A root that exists but cannot be read (the path replaced by a directory, + // or permissions revoked) used to throw out of the request handler as an + // unhandled rejection: /api/plan never answered and the tab hung. It is + // the missing-file fallback: the startup snapshot, with its version diff. + async function seedTwoVersions(label: string): Promise<{ pagePath: string; project: string }> { + const pagePath = join(freshDocDir(label), "page.html"); + const project = uniqueProject(label); + writeFileSync(pagePath, page("V1"), "utf-8"); + const seed = await startAnnotateServer({ + markdown: "", + filePath: pagePath, + htmlContent: MINIMAL_HTML, + rawHtml: page("V1"), + renderHtml: true, + project, + }); + seed.stop(); + writeFileSync(pagePath, page("V2"), "utf-8"); + return { pagePath, project }; + } + + type FallbackPayload = { rawHtml?: string; previousPlan?: string | null; versionInfo?: { version: number }; diffHtml?: string }; + + // The old behavior hung forever, so the request is raced against a timeout. + const withTimeout = (p: Promise, ms = 5000): Promise => + Promise.race([ + p, + new Promise((_, reject) => setTimeout(() => reject(new Error(`request did not answer within ${ms}ms`)), ms)), + ]); + + test("/api/plan falls back to the startup snapshot (with its version diff) when the root path becomes a directory", async () => { + const { pagePath, project } = await seedTwoVersions("dir"); + const server = await startAnnotateServer({ + markdown: "", + filePath: pagePath, + htmlContent: MINIMAL_HTML, + rawHtml: page("V2"), + renderHtml: true, + project, + }); + try { + unlinkSync(pagePath); + mkdirSync(pagePath); + const res = await withTimeout(fetch(`${server.url}/api/plan`)); + expect(res.status).toBe(200); + const fallback = (await res.json()) as FallbackPayload; + expect(fallback.rawHtml).toContain("V2"); + expect(fallback.previousPlan).toBe(page("V1")); + expect(fallback.versionInfo?.version).toBe(2); + expect(fallback.diffHtml).toBeDefined(); + + const share = await withTimeout(fetch(`${server.url}/api/share-html`)); + expect(share.status).toBe(200); + expect(((await share.json()) as { shareHtml: string }).shareHtml).toContain("V2"); + } finally { + server.stop(); + } + }); + + // chmod 000 is not a restriction for root, so the check is skipped there. + const canRevokeRead = process.platform !== "win32" && typeof process.getuid === "function" && process.getuid() !== 0; + test.skipIf(!canRevokeRead)("/api/plan falls back to the startup snapshot when the root file is unreadable", async () => { + const { pagePath, project } = await seedTwoVersions("perm"); + const server = await startAnnotateServer({ + markdown: "", + filePath: pagePath, + htmlContent: MINIMAL_HTML, + rawHtml: page("V2"), + renderHtml: true, + project, + }); + try { + writeFileSync(pagePath, page("V3"), "utf-8"); + chmodSync(pagePath, 0o000); + const res = await withTimeout(fetch(`${server.url}/api/plan`)); + expect(res.status).toBe(200); + const fallback = (await res.json()) as FallbackPayload; + expect(fallback.rawHtml).toContain("V2"); + expect(fallback.rawHtml).not.toContain("V3"); + expect(fallback.previousPlan).toBe(page("V1")); + expect(fallback.diffHtml).toBeDefined(); + } finally { + chmodSync(pagePath, 0o644); + server.stop(); + } + }); }); diff --git a/apps/pi-extension/server/serverAnnotate.ts b/apps/pi-extension/server/serverAnnotate.ts index a948a8604..19ac8bb1f 100644 --- a/apps/pi-extension/server/serverAnnotate.ts +++ b/apps/pi-extension/server/serverAnnotate.ts @@ -453,14 +453,23 @@ export async function startAnnotateServer(options: { : null; type RootHtmlRead = | { kind: "current"; html: string } - | { kind: "snapshot"; reason: "missing" | "too-large" }; + | { kind: "snapshot"; reason: "missing" | "too-large" | "unreadable" }; function readRootHtml(): RootHtmlRead | null { if (!rootHtmlSourcePath) return null; - if (!existsSync(rootHtmlSourcePath)) return { kind: "snapshot", reason: "missing" }; - if (statSync(rootHtmlSourcePath).size > MAX_ANNOTATABLE_FILE_BYTES) { - return { kind: "snapshot", reason: "too-large" }; + // A present-but-unreadable root (permissions revoked, the path replaced + // by a directory) is the same fallback as a missing one: the startup + // snapshot, with its version diff. The read must never throw out of the + // request handler: an unhandled rejection there leaves /api/plan + // unanswered and the tab hangs on reload. + try { + if (!existsSync(rootHtmlSourcePath)) return { kind: "snapshot", reason: "missing" }; + if (statSync(rootHtmlSourcePath).size > MAX_ANNOTATABLE_FILE_BYTES) { + return { kind: "snapshot", reason: "too-large" }; + } + return { kind: "current", html: readFileSync(rootHtmlSourcePath, "utf-8") }; + } catch { + return { kind: "snapshot", reason: "unreadable" }; } - return { kind: "current", html: readFileSync(rootHtmlSourcePath, "utf-8") }; } function handleShareHtml(res: import("node:http").ServerResponse, url: URL): void { diff --git a/packages/server/annotate.test.ts b/packages/server/annotate.test.ts index 9b3e86b57..bb95351fb 100644 --- a/packages/server/annotate.test.ts +++ b/packages/server/annotate.test.ts @@ -15,7 +15,7 @@ */ import { afterAll, afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, realpathSync, rmSync, symlinkSync, unlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, realpathSync, rmSync, symlinkSync, unlinkSync, writeFileSync } from "node:fs"; import { createHash } from "node:crypto"; import { tmpdir } from "os"; import { join, resolve } from "path"; @@ -343,6 +343,86 @@ describe("annotate server: local rendered-HTML root freshness", () => { server.stop(); } }); + + // A root that exists but cannot be read (the path replaced by a directory, + // or permissions revoked) used to throw out of the request handler: a tab + // reload answered 500. It is the missing-file fallback: the startup + // snapshot, with its version diff, and the share endpoint agrees. + async function seedTwoVersions(label: string): Promise<{ pagePath: string; project: string }> { + const pagePath = join(freshDocDir(label), "page.html"); + const project = uniqueProject(label); + writeFileSync(pagePath, page("V1"), "utf-8"); + const seed = await startAnnotateServer({ + markdown: "", + filePath: pagePath, + htmlContent: MINIMAL_HTML, + rawHtml: page("V1"), + renderHtml: true, + project, + }); + seed.stop(); + writeFileSync(pagePath, page("V2"), "utf-8"); + return { pagePath, project }; + } + + type FallbackPayload = { rawHtml?: string; previousPlan?: string | null; versionInfo?: { version: number }; diffHtml?: string }; + + test("/api/plan falls back to the startup snapshot (with its version diff) when the root path becomes a directory", async () => { + const { pagePath, project } = await seedTwoVersions("dir"); + const server = await startAnnotateServer({ + markdown: "", + filePath: pagePath, + htmlContent: MINIMAL_HTML, + rawHtml: page("V2"), + renderHtml: true, + project, + }); + try { + unlinkSync(pagePath); + mkdirSync(pagePath); + const res = await fetch(`${server.url}/api/plan`); + expect(res.status).toBe(200); + const fallback = (await res.json()) as FallbackPayload; + expect(fallback.rawHtml).toContain("V2"); + expect(fallback.previousPlan).toBe(page("V1")); + expect(fallback.versionInfo?.version).toBe(2); + expect(fallback.diffHtml).toBeDefined(); + + const share = await fetch(`${server.url}/api/share-html`); + expect(share.status).toBe(200); + expect(((await share.json()) as { shareHtml: string }).shareHtml).toContain("V2"); + } finally { + server.stop(); + } + }); + + // chmod 000 is not a restriction for root, so the check is skipped there. + const canRevokeRead = process.platform !== "win32" && typeof process.getuid === "function" && process.getuid() !== 0; + test.skipIf(!canRevokeRead)("/api/plan falls back to the startup snapshot when the root file is unreadable", async () => { + const { pagePath, project } = await seedTwoVersions("perm"); + const server = await startAnnotateServer({ + markdown: "", + filePath: pagePath, + htmlContent: MINIMAL_HTML, + rawHtml: page("V2"), + renderHtml: true, + project, + }); + try { + writeFileSync(pagePath, page("V3"), "utf-8"); + chmodSync(pagePath, 0o000); + const res = await fetch(`${server.url}/api/plan`); + expect(res.status).toBe(200); + const fallback = (await res.json()) as FallbackPayload; + expect(fallback.rawHtml).toContain("V2"); + expect(fallback.rawHtml).not.toContain("V3"); + expect(fallback.previousPlan).toBe(page("V1")); + expect(fallback.diffHtml).toBeDefined(); + } finally { + chmodSync(pagePath, 0o644); + server.stop(); + } + }); }); describe("annotate server: source save", () => { diff --git a/packages/server/annotate.ts b/packages/server/annotate.ts index 4eec35961..93d37d793 100644 --- a/packages/server/annotate.ts +++ b/packages/server/annotate.ts @@ -414,13 +414,21 @@ export async function startAnnotateServer( renderHtml && rawHtml && !/^https?:\/\//i.test(filePath) ? resolvePath(filePath) : null; type RootHtmlRead = | { kind: "current"; html: string } - | { kind: "snapshot"; reason: "missing" | "too-large" }; + | { kind: "snapshot"; reason: "missing" | "too-large" | "unreadable" }; async function readRootHtml(): Promise { if (!rootHtmlSourcePath) return null; - const file = Bun.file(rootHtmlSourcePath); - if (!(await file.exists())) return { kind: "snapshot", reason: "missing" }; - if (file.size > MAX_ANNOTATABLE_FILE_BYTES) return { kind: "snapshot", reason: "too-large" }; - return { kind: "current", html: await file.text() }; + // A present-but-unreadable root (permissions revoked, the path replaced + // by a directory) is the same fallback as a missing one: the startup + // snapshot, with its version diff. The read must never throw out of a + // request handler, which would turn a tab reload into a 500. + try { + const file = Bun.file(rootHtmlSourcePath); + if (!(await file.exists())) return { kind: "snapshot", reason: "missing" }; + if (file.size > MAX_ANNOTATABLE_FILE_BYTES) return { kind: "snapshot", reason: "too-large" }; + return { kind: "current", html: await file.text() }; + } catch { + return { kind: "snapshot", reason: "unreadable" }; + } } async function loadShareHtml(pathParam: string | null): Promise { From 66474980bdf1c95391e8d5fe237a8d7950d60871 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 27 Aug 2026 14:50:20 -0700 Subject: [PATCH 02/17] fix(annotate): keep the HTML version diff across Refresh and reload Once the served root bytes differed from the startup snapshot, both servers omitted the version-diff fields and applyRefreshedHtml nulled them client-side, so the "Show changes vs previous version" toggle vanished for the rest of the session the moment a reviewer refreshed: exactly when an agent had just edited the file. previousPlan/versionInfo describe the saved baseline, which stays the correct previous version, so both runtimes now keep them and recompute diffCurrent/diffHtml against the served bytes (htmlDiff is pure; a GET never writes history). /api/doc carries the same recomputed fields when it serves the root document (rootHtmlVersionDiff, root only; linked docs unchanged), the published useHtmlRefresh passes the whole snapshot to onSnapshot, and applyRefreshedHtml sets the fields instead of nulling them while still resetting isPlanDiffActive. Server tests on both runtimes assert the recomputed diff (and its absence for a sibling document); a DOM test asserts the toggle survives a refresh. CLAUDE.md documents the /api/plan contract. --- AGENTS.md | 2 +- apps/pi-extension/server/reference.ts | 24 ++++++ .../server/serverAnnotate-root-html.test.ts | 42 +++++++++-- apps/pi-extension/server/serverAnnotate.ts | 44 ++++++++--- packages/editor/App.htmlChrome.test.tsx | 73 ++++++++++++++++++- packages/editor/App.tsx | 21 ++++-- packages/editor/hooks/useHtmlRefresh.ts | 36 ++++++--- packages/editor/sourceDocumentClient.ts | 29 +++++++- packages/server/annotate.test.ts | 44 +++++++++-- packages/server/annotate.ts | 44 ++++++++--- packages/server/reference-handlers.ts | 23 ++++++ packages/ui/hooks/useHtmlRefresh.ts | 29 +++++--- 12 files changed, 340 insertions(+), 71 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 91b4c913d..1dada1bdb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -457,7 +457,7 @@ During normal plan review, an Archive sidebar tab provides the same browsing via | Endpoint | Method | Purpose | | --------------------- | ------ | ------------------------------------------ | -| `/api/plan` | GET | Returns `{ plan, origin, mode: "annotate", filePath, sourceInfo?, gate, renderAs?, rawHtml?, previousPlan?, versionInfo?, diffCurrent?, diffHtml? }`. The last four power the per-file version diff: `previousPlan`/`versionInfo`/`diffCurrent` for the markdown diff, `diffHtml` (the previous→current page rendered with inline ``/``) for `--render-html` files. Live app sessions return `{ mode: "annotate-app", appUrl, targetUrl, liveToken, sharingEnabled: false, ... }` instead: no rawHtml, no version fields (see "Live app annotation"). | +| `/api/plan` | GET | Returns `{ plan, origin, mode: "annotate", filePath, sourceInfo?, gate, renderAs?, rawHtml?, previousPlan?, versionInfo?, diffCurrent?, diffHtml? }`. The last four power the per-file version diff: `previousPlan`/`versionInfo`/`diffCurrent` for the markdown diff, `diffHtml` (the previous→current page rendered with inline ``/``) for `--render-html` files. A local rendered-HTML root is served from its CURRENT bytes on every read (`readRootHtml`), with the startup snapshot as the fallback when the file is missing, unreadable, or over the 2MB cap; when the served bytes differ from the snapshot, `previousPlan`/`versionInfo` still name the saved baseline and `diffCurrent`/`diffHtml` are recomputed against the served bytes (`htmlDiff` is pure; a GET never writes history), so a reload after an agent edit keeps the version diff. `/api/doc` carries the same recomputed `previousPlan`/`versionInfo`/`diffHtml` when it serves that root document (the in-app Refresh path, `rootHtmlVersionDiff`), and nothing extra for any other document. Live app sessions return `{ mode: "annotate-app", appUrl, targetUrl, liveToken, sharingEnabled: false, ... }` instead: no rawHtml, no version fields (see "Live app annotation"). | | `/api/plan/version` | GET | Fetch a specific stored version of the annotated file (`?v=N`) | | `/api/plan/versions` | GET | List all stored versions of the annotated file | | `/api/feedback` | POST | Submit annotations (body: feedback, annotations) | diff --git a/apps/pi-extension/server/reference.ts b/apps/pi-extension/server/reference.ts index 193d1be6e..fb690b2b8 100644 --- a/apps/pi-extension/server/reference.ts +++ b/apps/pi-extension/server/reference.ts @@ -94,6 +94,19 @@ export interface HandleDocOptions { annotateHistory?: { compute: (resolvedFilePath: string, content: string) => FolderAnnotateHistory | null; }; + /** + * Single-file rendered-HTML sessions: when /api/doc serves the session's + * ROOT document (`path` equals the resolved root path) as raw HTML, merge + * the version-diff fields `compute` derives from the bytes just read + * (`previousPlan`/`versionInfo`/`diffHtml`, the same names /api/plan + * uses) into the response. This is what lets the in-app Refresh keep the + * version diff. Every other document is untouched. Mirrors the Bun + * handler in packages/server/reference-handlers.ts. + */ + rootHtmlVersionDiff?: { + path: string; + compute: (currentHtml: string) => Record; + }; } interface HandleDocExistsOptions { @@ -233,6 +246,17 @@ function applyDocOptions>( sourceSnapshot?: SourceFileSnapshot, ): DocOptionsResult { const next: Record = { ...data }; + // Root-document version diff (see HandleDocOptions.rootHtmlVersionDiff): + // computed on the raw bytes, before the asset rewrite below, because the + // diff renderer rewrites its own output the same way. + if ( + options.rootHtmlVersionDiff && + data.renderAs === "html" && + typeof data.rawHtml === "string" && + data.filepath === options.rootHtmlVersionDiff.path + ) { + Object.assign(next, options.rootHtmlVersionDiff.compute(data.rawHtml)); + } if ( typeof next.rawHtml === "string" && typeof next.filepath === "string" && diff --git a/apps/pi-extension/server/serverAnnotate-root-html.test.ts b/apps/pi-extension/server/serverAnnotate-root-html.test.ts index 681734e06..56819b1ef 100644 --- a/apps/pi-extension/server/serverAnnotate-root-html.test.ts +++ b/apps/pi-extension/server/serverAnnotate-root-html.test.ts @@ -5,7 +5,7 @@ * packages/server/annotate.test.ts: a local rendered-HTML root is served from * its current bytes by both /api/plan (tab reload) and /api/share-html (share * after Refresh), with the startup snapshot only as the deleted-file fallback, - * and the startup version diff is dropped once the served bytes differ. + * and the version diff is recomputed against the served bytes once they differ. * * History lives in the real data dir (storage resolves it at import time), so * every test uses its own project namespace, removed in afterAll. @@ -14,7 +14,7 @@ import { afterAll, afterEach, beforeEach, describe, expect, test } from "bun:test"; import { chmodSync, mkdirSync, mkdtempSync, realpathSync, rmSync, unlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { startAnnotateServer } from "./serverAnnotate.ts"; import { getPlannotatorDataDir } from "../generated/data-dir.ts"; @@ -93,7 +93,10 @@ describe("pi annotate server: local rendered-HTML root freshness", () => { } }); - test("/api/plan serves the root document's current bytes and drops the startup version diff once they differ", async () => { + // A tab reload after an agent edit keeps the version diff: the saved + // baseline is still the previous version, so the diff is recomputed + // against the served bytes rather than dropped. Reads never write history. + test("/api/plan serves the root document's current bytes and recomputes the version diff against them", async () => { const pagePath = join(freshDocDir("plan"), "page.html"); const project = uniqueProject("plan"); type PlanPayload = { @@ -137,10 +140,35 @@ describe("pi annotate server: local rendered-HTML root freshness", () => { const reloaded = await plan(); expect(reloaded.rawHtml).toContain("V3"); expect(reloaded.rawHtml).not.toContain("V2"); - expect(reloaded.previousPlan).toBeUndefined(); - expect(reloaded.versionInfo).toBeUndefined(); - expect(reloaded.diffCurrent).toBeUndefined(); - expect(reloaded.diffHtml).toBeUndefined(); + // The baseline still names the saved previous version... + expect(reloaded.previousPlan).toBe(page("V1")); + expect(reloaded.versionInfo?.version).toBe(2); + // ...and the diff describes V1 -> V3, the page actually on screen. + expect(reloaded.diffCurrent).toBe(page("V3")); + expect(reloaded.diffHtml).toContain(" ({ + previousPlan: rootHistory.previousPlan, + versionInfo: rootHistory.versionInfo, + ...(rootHistory.previousPlan + ? { diffHtml: htmlAssets.rewriteHtml(htmlDiff(rootHistory.previousPlan, currentHtml), options.filePath) } + : {}), + }), + } + : undefined; + function handleShareHtml(res: import("node:http").ServerResponse, url: URL): void { if (/^https?:\/\//i.test(options.filePath)) { json(res, { error: "Raw HTML sharing is unavailable for URL annotations" }, 400); @@ -666,14 +685,16 @@ export async function startAnnotateServer(options: { // readRootHtml); every other session serves what it started with. const rootRead = readRootHtml(); const servedHtml = rootRead?.kind === "current" ? rootRead.html : options.rawHtml; - // The version-diff fields (previousPlan/versionInfo/diffCurrent and - // the rendered diffHtml) describe the STARTUP snapshot, which is - // the version history saved. Once the served bytes differ from it - // they are omitted rather than recomputed: the current bytes are - // not a saved version, so version metadata for them would be - // wrong, and re-snapshotting would turn a read into a history - // write. This mirrors what the client does after an in-app - // Refresh, so refresh and reload converge on the same state. + // The version-diff fields describe the SAVED baseline: previousPlan + // and versionInfo name the version history saved at startup, which + // stays the correct "previous version" however often the file is + // edited afterwards. When the served bytes differ from the startup + // snapshot the diff is RECOMPUTED against them (htmlDiff is pure, + // and a GET never writes history), so a tab reload after an agent + // edit keeps the "Show changes" toggle instead of losing it for + // the rest of the session. The in-app Refresh reads the same + // fields off /api/doc (rootHtmlVersionDiff), so refresh and + // reload converge on the same state. Mirrors packages/server/annotate.ts. const servedIsSnapshot = servedHtml === options.rawHtml; const displayRawHtml = options.renderHtml && servedHtml ? htmlAssets.rewriteHtml(servedHtml, options.filePath) @@ -682,7 +703,7 @@ export async function startAnnotateServer(options: { // / highlights (tag-aware htmlDiff), asset-rewritten the // same way as the live page so it renders identically. const diffHtml = - options.renderHtml && servedHtml && servedIsSnapshot && annotateHistory?.previousPlan + options.renderHtml && servedHtml && annotateHistory?.previousPlan ? htmlAssets.rewriteHtml(htmlDiff(annotateHistory.previousPlan, servedHtml), options.filePath) : undefined; const primarySource = getPrimarySource(); @@ -703,11 +724,11 @@ export async function startAnnotateServer(options: { ...(displayRawHtml ? { rawHtml: displayRawHtml } : {}), ...(diffHtml ? { diffHtml } : {}), convertHtml: options.convertHtml ?? false, - ...(annotateHistory && servedIsSnapshot + ...(annotateHistory ? { previousPlan: annotateHistory.previousPlan, versionInfo: annotateHistory.versionInfo, - diffCurrent: annotateHistory.diffCurrent, + diffCurrent: servedIsSnapshot || !servedHtml ? annotateHistory.diffCurrent : servedHtml, } : {}), sharingEnabled, @@ -895,6 +916,7 @@ export async function startAnnotateServer(options: { options.mode === "annotate-folder" && annotateHistoryEnabled ? { compute: computeFolderAnnotateHistory } : undefined, + rootHtmlVersionDiff, }); } else if (url.pathname === "/api/source/save" && req.method === "POST") { let body: SourceSaveRequest; diff --git a/packages/editor/App.htmlChrome.test.tsx b/packages/editor/App.htmlChrome.test.tsx index 3d09f5f12..989fd0984 100644 --- a/packages/editor/App.htmlChrome.test.tsx +++ b/packages/editor/App.htmlChrome.test.tsx @@ -114,6 +114,44 @@ const annotateFetch: typeof fetch = async (input) => { return Response.json({}); }; +// A session whose file has a saved previous version: /api/plan carries the +// rendered diff, and the root's /api/doc read (what Refresh performs) carries +// the diff recomputed against the bytes just read. +const DIFF_HTML = "

Rendered page

Body copy.

"; +const REFRESHED_HTML = "

Rendered page

Body copy, edited.

"; +const REFRESHED_DIFF_HTML = "

Rendered page

Body copy, edited.

"; +const versionedPlan = { + ...htmlAnnotatePlan, + diffHtml: DIFF_HTML, + previousPlan: "

Rendered

Body copy.

", + versionInfo: { version: 2, totalVersions: 2, project: "test" }, +}; +const versionedFetch: typeof fetch = async (input) => { + const rawUrl = input instanceof Request ? input.url : String(input); + const url = new URL(rawUrl, "http://localhost"); + if (url.pathname === "/api/plan") return Response.json(versionedPlan); + if (url.pathname === "/api/doc" && url.searchParams.get("path") === htmlAnnotatePlan.filePath) { + return Response.json({ + rawHtml: REFRESHED_HTML, + renderAs: "html", + filepath: htmlAnnotatePlan.filePath, + diffHtml: REFRESHED_DIFF_HTML, + previousPlan: versionedPlan.previousPlan, + versionInfo: versionedPlan.versionInfo, + }); + } + return annotateFetch(input); +}; + +function diffToggle(): HTMLButtonElement | undefined { + return Array.from(document.querySelectorAll("button")) + .find((button) => /changes vs previous version/.test(button.title)); +} + +function refreshButton(): HTMLButtonElement | null { + return document.querySelector("[data-html-refresh]"); +} + function findButtonByText(label: string): HTMLButtonElement | undefined { return Array.from(document.querySelectorAll("button")) .find((button) => button.textContent?.trim() === label); @@ -142,8 +180,8 @@ async function settle(): Promise { }); } -async function mountHtmlAnnotate(): Promise { - globalThis.fetch = annotateFetch; +async function mountHtmlAnnotate(fetchImpl: typeof fetch = annotateFetch): Promise { + globalThis.fetch = fetchImpl; // SAFETY: the App only uses EventSource's constructor, handlers, and close; // this test double implements those browser-facing members without I/O. globalThis.EventSource = SilentEventSource as unknown as typeof EventSource; @@ -383,6 +421,37 @@ describe.if(hasDom)("HTML annotate chrome (tools toggle + pen toggle)", () => { expect(findButtonByText("Contents")).not.toBeUndefined(); }); + test("Refresh keeps the version-diff toggle available (the server recomputes the diff for the root document)", async () => { + setStorageBackend(memoryBackend); + seedAnnouncementsSeen(); + await mountHtmlAnnotate(versionedFetch); + await settle(); + + // The toggle is offered on load, in normal (non-diff) mode. + expect(diffToggle()).not.toBeUndefined(); + expect(diffToggle()!.title).toBe("Show changes vs previous version"); + + // Enter the diff view, then refresh: the view returns to normal mode and + // the toggle is still there (it used to disappear for the session). + await act(async () => diffToggle()!.click()); + expect(diffToggle()!.title).toBe("Hide changes vs previous version"); + + const refresh = refreshButton(); + if (!refresh) throw new Error("refresh button missing"); + await act(async () => refresh.click()); + for (let attempt = 0; attempt < 20 && refreshButton()?.getAttribute("aria-disabled") !== "false"; attempt += 1) { + await settle(); + } + + expect(diffToggle()).not.toBeUndefined(); + expect(diffToggle()!.title).toBe("Show changes vs previous version"); + // And the recomputed diff is what a second toggle renders. + await act(async () => diffToggle()!.click()); + await settle(); + const frame = document.querySelector("iframe[srcdoc]"); + expect(frame?.getAttribute("srcdoc")).toContain(", edited"); + }); + test("the pen toggle starts ARMED (aria-pressed) on a static HTML session and click flips it to Interact", async () => { setStorageBackend(memoryBackend); seedAnnouncementsSeen(); diff --git a/packages/editor/App.tsx b/packages/editor/App.tsx index 198c611e8..8a8e7b1bc 100644 --- a/packages/editor/App.tsx +++ b/packages/editor/App.tsx @@ -145,7 +145,7 @@ import { usePlanDiffViewAutoExit, } from './hooks/usePlanDiffViewAutoExit'; import { AppHeader } from './components/AppHeader'; -import { useHtmlRefresh } from './hooks/useHtmlRefresh'; +import { useHtmlRefresh, type HtmlRefreshedDocument } from './hooks/useHtmlRefresh'; import { AgentNudgeBanner } from './components/AgentNudgeBanner'; import { useDocumentWebMcp } from './webmcp/useDocumentWebMcp'; import { useWebMcpActivity } from '@plannotator/ui/webmcp'; @@ -1200,15 +1200,22 @@ const App: React.FC = () => { setMarkdown, setAnnotations, setSelectedAnnotationId, setSubmitted, }); const documentReadOnly = archive.archiveMode; - const applyRefreshedHtml = useCallback((nextRawHtml: string) => { - setRawHtml(nextRawHtml); + // A Refresh lands the bytes and, for the root document, the version diff + // the server recomputed against them (previousPlan/versionInfo still name + // the saved baseline). The view returns to normal mode with the "Show + // changes" toggle available whenever a diff came back; a refresh of a + // linked doc keeps the root's version fields untouched, as before. + const applyRefreshedHtml = useCallback((refreshed: HtmlRefreshedDocument) => { + setRawHtml(refreshed.rawHtml); setShareHtml(''); - setHtmlDiffHtml(null); setIsPlanDiffActive(false); - if (!linkedDocHook.isActive) { - setPreviousPlan(null); - setVersionInfo(null); + if (linkedDocHook.isActive) { + setHtmlDiffHtml(null); + return; } + setHtmlDiffHtml(refreshed.diffHtml ?? null); + setPreviousPlan(refreshed.previousPlan ?? null); + setVersionInfo(refreshed.versionInfo ?? null); }, [linkedDocHook.isActive]); // Annotations a Refresh could no longer anchor: the panel shows an // "Unanchored" chip on them. Set from the refresh's restore report only, diff --git a/packages/editor/hooks/useHtmlRefresh.ts b/packages/editor/hooks/useHtmlRefresh.ts index b4b58bc60..94413ea42 100644 --- a/packages/editor/hooks/useHtmlRefresh.ts +++ b/packages/editor/hooks/useHtmlRefresh.ts @@ -4,12 +4,18 @@ import { useHtmlRefresh as usePublishedHtmlRefresh, type HtmlRefreshSnapshot, } from '@plannotator/ui/hooks/useHtmlRefresh'; -import { fetchHtmlDocumentSnapshot } from '../sourceDocumentClient'; +import { fetchHtmlDocumentSnapshot, type HtmlVersionDiffFields } from '../sourceDocumentClient'; + +/** What a refresh hands the app: the bytes plus, for the root document, the + * version-diff fields the server recomputed against them. */ +export interface HtmlRefreshedDocument extends HtmlVersionDiffFields { + rawHtml: string; +} interface UseHtmlRefreshOptions { enabled: boolean; activePath: string | null; - onSnapshot: (rawHtml: string) => void; + onSnapshot: (document: HtmlRefreshedDocument) => void; /** The ids the remounted viewer could not re-anchor, once per refresh. */ onUnanchored?: (missingIds: string[]) => void; } @@ -24,8 +30,9 @@ interface UseHtmlRefreshResult { /** * Plannotator's binding of the published `useHtmlRefresh`: the snapshot - * comes from `/api/doc` through `fetchHtmlDocumentSnapshot`, URL sessions - * (http(s) paths) cannot refresh, and every outcome toasts. + * comes from `/api/doc` through `fetchHtmlDocumentSnapshot` (which, for the + * session's root document, also carries the recomputed version diff), URL + * sessions (http(s) paths) cannot refresh, and every outcome toasts. */ export function useHtmlRefresh({ enabled, @@ -35,13 +42,22 @@ export function useHtmlRefresh({ }: UseHtmlRefreshOptions): UseHtmlRefreshResult { const canRefresh = enabled && !!activePath && !/^https?:\/\//i.test(activePath); - const fetchSnapshot = useCallback(async (path: string | null): Promise => { + const fetchSnapshot = useCallback(async (path: string | null): Promise> => { const result = await fetchHtmlDocumentSnapshot(path ?? ''); - return result.status === 'ok' - ? { status: 'ok', rawHtml: result.snapshot.rawHtml } - : { status: result.status }; + if (result.status !== 'ok') return { status: result.status }; + const { rawHtml, diffHtml, previousPlan, versionInfo } = result.snapshot; + return { status: 'ok', rawHtml, diffHtml, previousPlan, versionInfo }; }, []); + const handleSnapshot = useCallback((rawHtml: string, snapshot: HtmlVersionDiffFields) => { + onSnapshot({ + rawHtml, + diffHtml: snapshot.diffHtml, + previousPlan: snapshot.previousPlan, + versionInfo: snapshot.versionInfo, + }); + }, [onSnapshot]); + const handleResult = useCallback((result: 'refreshed' | 'missing' | 'unavailable') => { if (result === 'missing') { toast.error('HTML file no longer exists', { @@ -65,11 +81,11 @@ export function useHtmlRefresh({ }); }, [onUnanchored]); - return usePublishedHtmlRefresh({ + return usePublishedHtmlRefresh({ enabled: canRefresh, documentKey: activePath, fetchSnapshot, - onSnapshot, + onSnapshot: handleSnapshot, onUnanchored: handleUnanchored, onResult: handleResult, }); diff --git a/packages/editor/sourceDocumentClient.ts b/packages/editor/sourceDocumentClient.ts index 477a75daa..5dec6de68 100644 --- a/packages/editor/sourceDocumentClient.ts +++ b/packages/editor/sourceDocumentClient.ts @@ -7,7 +7,14 @@ export type SourceSaveProbeResult = | { status: 'missing' } | { status: 'unavailable' }; -interface SourceDocumentResponse { +export interface HtmlVersionDiffFields { + /** The previous-version page rendered with inline ins/del highlights. */ + diffHtml?: string; + previousPlan?: string | null; + versionInfo?: { version: number; totalVersions: number; project: string }; +} + +interface SourceDocumentResponse extends HtmlVersionDiffFields { markdown?: string; rawHtml?: string; filepath?: string; @@ -30,7 +37,12 @@ export type SourceDocumentSnapshotResult = | { status: 'missing' } | { status: 'unavailable' }; -export interface HtmlDocumentSnapshot { +/** + * A rendered-HTML document read from /api/doc. The version-diff fields are + * present only when the server served the session's ROOT document (it + * recomputes them against the bytes just read); linked docs carry none. + */ +export interface HtmlDocumentSnapshot extends HtmlVersionDiffFields { rawHtml: string; filepath: string; } @@ -79,9 +91,18 @@ export async function fetchHtmlDocumentSnapshot(path: string): Promise { }); // A tab reload after an agent edit must show the edited page (the draft - // annotations were placed on it), and must not pair it with a version diff - // computed for the startup snapshot. - test("/api/plan serves the root document's current bytes and drops the startup version diff once they differ", async () => { + // annotations were placed on it) AND keep the version diff: the saved + // baseline is still the previous version, so the diff is recomputed + // against the served bytes rather than dropped (a reload used to lose the + // "Show changes" toggle for the rest of the session). Reads never write + // history. + test("/api/plan serves the root document's current bytes and recomputes the version diff against them", async () => { const pagePath = join(freshDocDir("plan"), "page.html"); const project = uniqueProject("plan"); type PlanPayload = { @@ -325,10 +328,35 @@ describe("annotate server: local rendered-HTML root freshness", () => { const reloaded = await plan(); expect(reloaded.rawHtml).toContain("V3"); expect(reloaded.rawHtml).not.toContain("V2"); - expect(reloaded.previousPlan).toBeUndefined(); - expect(reloaded.versionInfo).toBeUndefined(); - expect(reloaded.diffCurrent).toBeUndefined(); - expect(reloaded.diffHtml).toBeUndefined(); + // The baseline still names the saved previous version... + expect(reloaded.previousPlan).toBe(page("V1")); + expect(reloaded.versionInfo?.version).toBe(2); + // ...and the diff describes V1 -> V3, the page actually on screen. + expect(reloaded.diffCurrent).toBe(page("V3")); + expect(reloaded.diffHtml).toContain(" ({ + previousPlan: rootHistory.previousPlan, + versionInfo: rootHistory.versionInfo, + ...(rootHistory.previousPlan + ? { diffHtml: htmlAssets.rewriteHtml(htmlDiff(rootHistory.previousPlan, currentHtml), filePath) } + : {}), + }), + } + : undefined; + async function loadShareHtml(pathParam: string | null): Promise { if (/^https?:\/\//i.test(filePath)) { return Response.json({ error: "Raw HTML sharing is unavailable for URL annotations" }, { status: 400 }); @@ -646,21 +665,23 @@ export async function startAnnotateServer( // readRootHtml); every other session serves what it started with. const rootRead = await readRootHtml(); const servedHtml = rootRead?.kind === "current" ? rootRead.html : rawHtml; - // The version-diff fields (previousPlan/versionInfo/diffCurrent and - // the rendered diffHtml) describe the STARTUP snapshot, which is - // the version history saved. Once the served bytes differ from it - // they are omitted rather than recomputed: the current bytes are - // not a saved version, so version metadata for them would be - // wrong, and re-snapshotting would turn a read into a history - // write. This mirrors what the client does after an in-app - // Refresh, so refresh and reload converge on the same state. + // The version-diff fields describe the SAVED baseline: previousPlan + // and versionInfo name the version history saved at startup, which + // stays the correct "previous version" however often the file is + // edited afterwards. When the served bytes differ from the startup + // snapshot the diff is RECOMPUTED against them (htmlDiff is pure, + // and a GET never writes history), so a tab reload after an agent + // edit keeps the "Show changes" toggle instead of losing it for + // the rest of the session. The in-app Refresh reads the same + // fields off /api/doc (rootHtmlVersionDiff), so refresh and + // reload converge on the same state. const servedIsSnapshot = servedHtml === rawHtml; const displayRawHtml = renderHtml && servedHtml ? htmlAssets.rewriteHtml(servedHtml, filePath) : undefined; // For HTML, render the version diff as the real page with inline // / highlights (tag-aware htmlDiff), asset-rewritten the // same way as the live page so it renders identically. const diffHtml = - renderHtml && servedHtml && servedIsSnapshot && annotateHistory?.previousPlan + renderHtml && servedHtml && annotateHistory?.previousPlan ? htmlAssets.rewriteHtml(htmlDiff(annotateHistory.previousPlan, servedHtml), filePath) : undefined; const primarySource = getPrimarySource(); @@ -681,11 +702,11 @@ export async function startAnnotateServer( ...(displayRawHtml ? { rawHtml: displayRawHtml } : {}), ...(diffHtml ? { diffHtml } : {}), convertHtml, - ...(annotateHistory && servedIsSnapshot + ...(annotateHistory ? { previousPlan: annotateHistory.previousPlan, versionInfo: annotateHistory.versionInfo, - diffCurrent: annotateHistory.diffCurrent, + diffCurrent: servedIsSnapshot || !servedHtml ? annotateHistory.diffCurrent : servedHtml, } : {}), sharingEnabled, @@ -866,6 +887,7 @@ export async function startAnnotateServer( mode === "annotate-folder" && annotateHistoryEnabled ? { compute: computeFolderAnnotateHistory } : undefined, + rootHtmlVersionDiff, }); } diff --git a/packages/server/reference-handlers.ts b/packages/server/reference-handlers.ts index a449c4dfe..02549b801 100644 --- a/packages/server/reference-handlers.ts +++ b/packages/server/reference-handlers.ts @@ -81,6 +81,18 @@ export interface HandleDocOptions { annotateHistory?: { compute: (resolvedFilePath: string, content: string) => FolderAnnotateHistory | null; }; + /** + * Single-file rendered-HTML sessions: when /api/doc serves the session's + * ROOT document (`path` equals the resolved root path) as raw HTML, merge + * the version-diff fields `compute` derives from the bytes just read + * (`previousPlan`/`versionInfo`/`diffHtml`, the same names /api/plan + * uses) into the response. This is what lets the in-app Refresh keep the + * version diff. Every other document is untouched. + */ + rootHtmlVersionDiff?: { + path: string; + compute: (currentHtml: string) => Record; + }; } interface HandleDocExistsOptions { @@ -219,6 +231,17 @@ function applyDocOptions>( sourceSnapshot?: SourceFileSnapshot, ): DocOptionsResult { const next: Record = { ...data }; + // Root-document version diff (see HandleDocOptions.rootHtmlVersionDiff): + // computed on the raw bytes, before the asset rewrite below, because the + // diff renderer rewrites its own output the same way. + if ( + options.rootHtmlVersionDiff && + data.renderAs === "html" && + typeof data.rawHtml === "string" && + data.filepath === options.rootHtmlVersionDiff.path + ) { + Object.assign(next, options.rootHtmlVersionDiff.compute(data.rawHtml)); + } if ( typeof next.rawHtml === "string" && typeof next.filepath === "string" && diff --git a/packages/ui/hooks/useHtmlRefresh.ts b/packages/ui/hooks/useHtmlRefresh.ts index 25c038e96..3e03c9e40 100644 --- a/packages/ui/hooks/useHtmlRefresh.ts +++ b/packages/ui/hooks/useHtmlRefresh.ts @@ -1,15 +1,22 @@ import { useCallback, useLayoutEffect, useRef, useState } from 'react'; +/** + * A successful snapshot. `Extra` lets a host carry document metadata read + * alongside the bytes (Plannotator: the root document's version diff) through + * to `onSnapshot`; the hook never reads anything but `rawHtml`. + */ +export type HtmlRefreshOkSnapshot = { status: 'ok'; rawHtml: string } & Extra; + /** What a host's `fetchSnapshot` resolves to. */ -export type HtmlRefreshSnapshot = - | { status: 'ok'; rawHtml: string } +export type HtmlRefreshSnapshot = + | HtmlRefreshOkSnapshot | { status: 'missing' } | { status: 'unavailable' }; /** The outcome of one `refresh()` call, for host notifications (toasts). */ export type HtmlRefreshResult = 'refreshed' | 'missing' | 'unavailable'; -export interface UseHtmlRefreshOptions { +export interface UseHtmlRefreshOptions { /** Whether refresh is offered at all. Default true. */ enabled?: boolean; /** @@ -22,9 +29,11 @@ export interface UseHtmlRefreshOptions { documentKey?: string | null; /** Fetch the current bytes of the document. Called with `documentKey`. * A rejection is treated as `{ status: 'unavailable' }`. */ - fetchSnapshot: (documentKey: string | null) => Promise; - /** Apply the refreshed bytes (the host owns the viewer's `rawHtml`). */ - onSnapshot: (rawHtml: string) => void; + fetchSnapshot: (documentKey: string | null) => Promise>; + /** Apply the refreshed bytes (the host owns the viewer's `rawHtml`). The + * whole successful snapshot is the second argument, for hosts whose + * `fetchSnapshot` reads metadata alongside the bytes. */ + onSnapshot: (rawHtml: string, snapshot: HtmlRefreshOkSnapshot) => void; /** * Once per refresh: the ids the remounted viewer could not re-anchor, * possibly empty. Wire the viewer's `onUnanchoredChange` to the returned @@ -57,14 +66,14 @@ export interface UseHtmlRefreshReturn { * restore acknowledgement is armed per reload generation and consumed by * the first viewer report for that generation. */ -export function useHtmlRefresh({ +export function useHtmlRefresh({ enabled = true, documentKey, fetchSnapshot, onSnapshot, onUnanchored, onResult, -}: UseHtmlRefreshOptions): UseHtmlRefreshReturn { +}: UseHtmlRefreshOptions): UseHtmlRefreshReturn { const [isRefreshing, setIsRefreshing] = useState(false); const [reloadGeneration, setReloadGeneration] = useState(0); const keyed = documentKey !== undefined; @@ -98,7 +107,7 @@ export function useHtmlRefresh({ // A rejecting fetch is an unavailable snapshot: the host hears it // through onResult like any other outcome, never as an unhandled // rejection out of refresh(). - let result: HtmlRefreshSnapshot; + let result: HtmlRefreshSnapshot; try { result = await fetchSnapshot(requestKey); } catch { @@ -111,7 +120,7 @@ export function useHtmlRefresh({ return; } - onSnapshot(result.rawHtml); + onSnapshot(result.rawHtml, result); const nextGeneration = reloadGenerationRef.current + 1; reloadGenerationRef.current = nextGeneration; // Armed until the remounted viewer's bridge reports its restore. The From b3c1b8b28762a7634d79c0088d1fead7d5fb751e Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 27 Aug 2026 14:51:19 -0700 Subject: [PATCH 03/17] feat(annotate): offer Refresh from disk in the compact touch shell HtmlSurfaceControls renders nothing on compact and the Options menu had equivalents for the pen and the eye but none for Refresh, while the header comment claimed the compact shell offered the same actions. The menu now has a "Refresh from disk" entry gated on the same canRefresh as the header button (local HTML files only; never URL or live-app sessions) and disabled while a refresh is in flight, and the comment names all three actions. A DOM test at a coarse-pointer compact viewport asserts the action and that the refreshed bytes reach the viewer. HtmlSurfaceActions.tsx was dead (only its own test consumed it; the header renders the published HtmlSurfaceControls directly and the parity sign-off happened in the browser), so it and its test are removed and PLANNOTATOR_HTML_REFRESH_LABELS now lives in AppHeader, the only importer. The CI DOM list drops the deleted test file. --- .github/workflows/test.yml | 1 - packages/editor/App.htmlChrome.test.tsx | 26 ++++++- packages/editor/App.tsx | 13 ++++ packages/editor/components/AppHeader.tsx | 13 +++- .../components/HtmlSurfaceActions.test.tsx | 76 ------------------- .../editor/components/HtmlSurfaceActions.tsx | 38 ---------- packages/ui/components/PlanHeaderMenu.tsx | 2 +- 7 files changed, 48 insertions(+), 121 deletions(-) delete mode 100644 packages/editor/components/HtmlSurfaceActions.test.tsx delete mode 100644 packages/editor/components/HtmlSurfaceActions.tsx diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f4b90cdd7..31f9bfc75 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -148,7 +148,6 @@ jobs: packages/editor/planDiffAutoExit.test.tsx packages/editor/App.archiveReadOnly.test.tsx packages/editor/App.htmlChrome.test.tsx - packages/editor/components/HtmlSurfaceActions.test.tsx packages/ui/components/HtmlSurfaceControls.test.tsx packages/ui/components/AnnotationPanel.unanchored.test.tsx packages/ui/hooks/useHtmlRefresh.test.tsx diff --git a/packages/editor/App.htmlChrome.test.tsx b/packages/editor/App.htmlChrome.test.tsx index 989fd0984..d6b2e6722 100644 --- a/packages/editor/App.htmlChrome.test.tsx +++ b/packages/editor/App.htmlChrome.test.tsx @@ -203,8 +203,8 @@ function armedRing(): HTMLElement | null { /** Compact mounts wait on the armed ring: neither the pen nor the eye toggle * exists on the compact touch shell (that absence is what these tests guard). */ -async function mountCompactHtmlAnnotate(): Promise { - globalThis.fetch = annotateFetch; +async function mountCompactHtmlAnnotate(fetchImpl: typeof fetch = annotateFetch): Promise { + globalThis.fetch = fetchImpl; // SAFETY: the App only uses EventSource's constructor, handlers, and close. globalThis.EventSource = SilentEventSource as unknown as typeof EventSource; host = document.createElement("div"); @@ -351,6 +351,28 @@ describe.if(hasDom)("HTML annotate chrome (tools toggle + pen toggle)", () => { expect(armedRing()).not.toBeNull(); }); + test("compact touch layout: Refresh from disk is offered through the Options menu (the header refresh is absent)", async () => { + setStorageBackend(memoryBackend); + seedAnnouncementsSeen(); + window.matchMedia = coarseMatchMedia as typeof window.matchMedia; + await mountCompactHtmlAnnotate(versionedFetch); + + // The desktop-only header refresh is not rendered on compact, so the + // menu action is the only way to re-read the file. + expect(refreshButton()).toBeNull(); + + await openOptionsMenu(); + const refresh = findMenuItem("Refresh from disk"); + if (!refresh) throw new Error('compact menu is missing the "Refresh from disk" action'); + expect(refresh.disabled).toBe(false); + await act(async () => refresh.click()); + for (let attempt = 0; attempt < 20 && !document.querySelector('iframe[srcdoc*="edited"]'); attempt += 1) { + await settle(); + } + // The refreshed bytes reached the viewer. + expect(document.querySelector("iframe[srcdoc]")?.getAttribute("srcdoc")).toContain("Body copy, edited."); + }); + test("the restore commit never writes stale pre-restore values to the cookie", async () => { // The chrome writer runs in the same commit as the restore effect, before // the restored state has landed. If it saved there, a returning user's diff --git a/packages/editor/App.tsx b/packages/editor/App.tsx index 8a8e7b1bc..49fc6d041 100644 --- a/packages/editor/App.tsx +++ b/packages/editor/App.tsx @@ -4847,6 +4847,19 @@ const App: React.FC = () => { onSelect: () => setHtmlToolsHidden((v) => !v), }] : []), + // The desktop header's Refresh is header-only too; local HTML files + // (never URL or live-app sessions) get the same action here. + ...(isHtmlSurface && htmlRefresh.canRefresh + ? [{ + id: 'refresh' as const, + label: 'Refresh from disk', + subtitle: htmlRefresh.isRefreshing + ? 'Refreshing the HTML file' + : 'Reload the HTML file and keep the annotations that still match', + onSelect: () => { void htmlRefresh.refresh(); }, + disabled: htmlRefresh.isRefreshing, + }] + : []), ]; const planMaxWidth = useMemo(() => { diff --git a/packages/editor/components/AppHeader.tsx b/packages/editor/components/AppHeader.tsx index 1b69bf6f3..140db2550 100644 --- a/packages/editor/components/AppHeader.tsx +++ b/packages/editor/components/AppHeader.tsx @@ -11,7 +11,13 @@ import type { UIPreferences } from '@plannotator/ui/utils/uiPreferences'; import { SparklesIcon } from '@plannotator/ui/components/SparklesIcon'; import type { CompactPlanAction } from '@plannotator/ui/components/PlanHeaderMenu'; import { HtmlSurfaceControls } from '@plannotator/ui/components/HtmlSurfaceControls'; -import { PLANNOTATOR_HTML_REFRESH_LABELS } from './HtmlSurfaceActions'; + +/** Plannotator's refresh strings for the published control: the document + * is a file on disk, so the refresh says so. */ +export const PLANNOTATOR_HTML_REFRESH_LABELS = { + refreshTitle: 'Refresh HTML from disk', + refreshingTitle: 'Refreshing HTML from disk', +} as const; interface AppHeaderProps { /** Mobile document-scroll surfaces let Safari own the top edge and scroll @@ -369,8 +375,9 @@ export const AppHeader = React.memo(({ {/* HTML and live-app surfaces only: the eye (show/hide tools, the only way back from hidden), the refresh, and the Interact/Annotate pen, in that order. The published control carries the markup; - the compact touch shell offers the same actions in its Options - menu instead. */} + the compact touch shell offers the same three actions in its + Options menu instead (compactDocumentActions in App: Show/Hide + tools, Interact/Annotate, Refresh from disk). */} {htmlSurface && (onToggleHtmlTools || onToggleHtmlAnnotate) && ( { - if (!hasDom) return; - act(() => root?.unmount()); - root = null; - container?.remove(); - container = null; -}); - -function renderActions(props?: Partial>) { - container = document.createElement('div'); - document.body.appendChild(container); - root = createRoot(container); - act(() => { - root?.render( - {}} - onToggleTools={() => {}} - {...props} - />, - ); - }); - return container; -} - -const refreshButton = (el: HTMLElement) => el.querySelector('[data-html-refresh]'); -const toolsToggle = (el: HTMLElement) => el.querySelector('[data-html-tools-toggle]'); - -describe.if(hasDom)('HtmlSurfaceActions', () => { - test('refresh fires the handler and the tools toggle keeps its pressed state', () => { - let refreshCount = 0; - const element = renderActions({ onRefresh: () => { refreshCount += 1; } }); - - const refresh = refreshButton(element); - expect(refresh?.getAttribute('aria-disabled')).toBe('false'); - act(() => refresh?.click()); - expect(refreshCount).toBe(1); - expect(toolsToggle(element)?.getAttribute('aria-pressed')).toBe('false'); - }); - - test('an in-flight refresh ignores clicks without dropping focus', () => { - let refreshCount = 0; - const element = renderActions({ isRefreshing: true, onRefresh: () => { refreshCount += 1; } }); - const refresh = refreshButton(element); - - // aria-disabled keeps the control focusable (a disabled button drops - // keyboard focus to body) while the click stays inert. - expect(refresh?.getAttribute('aria-disabled')).toBe('true'); - expect(refresh?.disabled).toBe(false); - refresh?.focus(); - act(() => refresh?.click()); - expect(refreshCount).toBe(0); - expect(document.activeElement).toBe(refresh); - }); - - test('omits refresh when the active HTML source is not refreshable', () => { - const element = renderActions({ canRefresh: false, toolsHidden: true }); - - expect(refreshButton(element)).toBeNull(); - // The tools toggle is the only way back from a hidden state, so it must - // render regardless of refresh availability. - expect(toolsToggle(element)?.getAttribute('aria-pressed')).toBe('true'); - }); -}); diff --git a/packages/editor/components/HtmlSurfaceActions.tsx b/packages/editor/components/HtmlSurfaceActions.tsx deleted file mode 100644 index 061154243..000000000 --- a/packages/editor/components/HtmlSurfaceActions.tsx +++ /dev/null @@ -1,38 +0,0 @@ -import { HtmlSurfaceControls } from '@plannotator/ui/components/HtmlSurfaceControls'; - -interface HtmlSurfaceActionsProps { - canRefresh: boolean; - isRefreshing: boolean; - toolsHidden: boolean; - onRefresh: () => void; - onToggleTools: () => void; -} - -/** Plannotator's refresh strings for the published control: the document - * is a file on disk, so the refresh says so. Shared with AppHeader. */ -export const PLANNOTATOR_HTML_REFRESH_LABELS = { - refreshTitle: 'Refresh HTML from disk', - refreshingTitle: 'Refreshing HTML from disk', -} as const; - -/** Eye + refresh, without the pen: the published HtmlSurfaceControls with - * Plannotator's refresh strings. Kept as the local name the header used. */ -export function HtmlSurfaceActions({ - canRefresh, - isRefreshing, - toolsHidden, - onRefresh, - onToggleTools, -}: HtmlSurfaceActionsProps) { - return ( - - ); -} diff --git a/packages/ui/components/PlanHeaderMenu.tsx b/packages/ui/components/PlanHeaderMenu.tsx index d054cf1c4..fece325ab 100644 --- a/packages/ui/components/PlanHeaderMenu.tsx +++ b/packages/ui/components/PlanHeaderMenu.tsx @@ -40,7 +40,7 @@ interface PlanHeaderMenuProps { } export interface CompactPlanAction { - id: 'exit' | 'feedback' | 'approve' | 'copy' | 'done' | 'edit' | 'tools' | 'annotate' | 'annotations' | 'ai' | 'review'; + id: 'exit' | 'feedback' | 'approve' | 'copy' | 'done' | 'edit' | 'tools' | 'annotate' | 'refresh' | 'annotations' | 'ai' | 'review'; label: string; subtitle?: string; onSelect: () => void; From f70cc4d9e71f3ebd139cf7804c9ba1873584618c Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 27 Aug 2026 14:51:48 -0700 Subject: [PATCH 04/17] fix(ui): render the HtmlSurfaceControls refresh independently of the eye The Refresh button only rendered inside the onToggleTools branch, so a host that passed canRefresh and onRefresh without a tools toggle got no Refresh, contradicting the documented props. The refresh and the eye still share one group left of the pen, but each now renders on its own terms; the test covers refresh without the eye, refresh alone, and the all-absent case. --- .../components/HtmlSurfaceControls.test.tsx | 20 +++++++++++++++- .../ui/components/HtmlSurfaceControls.tsx | 23 +++++++++++++------ 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/packages/ui/components/HtmlSurfaceControls.test.tsx b/packages/ui/components/HtmlSurfaceControls.test.tsx index d1cdff6d4..85d994cf1 100644 --- a/packages/ui/components/HtmlSurfaceControls.test.tsx +++ b/packages/ui/components/HtmlSurfaceControls.test.tsx @@ -66,11 +66,29 @@ describe.if(hasDom)('HtmlSurfaceControls', () => { expect(refresh(readOnly)).toBeNull(); expect(eye(readOnly)).not.toBeNull(); + // A host without the tools toggle still gets the refresh it asked for: + // the documented contract is canRefresh + onRefresh, not the eye. act(() => root?.unmount()); const noTools = render({ onToggleTools: undefined }); expect(eye(noTools)).toBeNull(); - expect(refresh(noTools)).toBeNull(); + expect(refresh(noTools)).not.toBeNull(); expect(pen(noTools)).not.toBeNull(); + + act(() => root?.unmount()); + const refreshOnly = render({ onToggleTools: undefined, onToggleArmed: undefined }); + expect(refreshOnly.querySelectorAll('button').length).toBe(1); + expect(refresh(refreshOnly)).not.toBeNull(); + + act(() => root?.unmount()); + const nothing = render({ onToggleTools: undefined, onToggleArmed: undefined, canRefresh: false }); + expect(nothing.childElementCount).toBe(0); + }); + + test('the refresh fires its handler without the eye present', () => { + let refreshes = 0; + const el = render({ onToggleTools: undefined, onRefresh: () => { refreshes += 1; } }); + act(() => refresh(el)!.click()); + expect(refreshes).toBe(1); }); test('compact renders nothing', () => { diff --git a/packages/ui/components/HtmlSurfaceControls.tsx b/packages/ui/components/HtmlSurfaceControls.tsx index 352cccf51..7ff996fa5 100644 --- a/packages/ui/components/HtmlSurfaceControls.tsx +++ b/packages/ui/components/HtmlSurfaceControls.tsx @@ -55,9 +55,10 @@ export interface HtmlSurfaceControlsProps { onToggleArmed?: () => void; /** Whether the floating tools over the page are hidden (eye-off). */ toolsHidden?: boolean; - /** Flip the tools. The eye (and the refresh beside it) render only when provided. */ + /** Flip the tools. The eye renders only when provided. */ onToggleTools?: () => void; - /** Whether a refresh is offered for this document. */ + /** Whether a refresh is offered for this document. The refresh renders + * whenever this is true and `onRefresh` is passed, with or without the eye. */ canRefresh?: boolean; onRefresh?: () => void; isRefreshing?: boolean; @@ -80,15 +81,21 @@ export function HtmlSurfaceControls({ if (compact) return null; const text = { ...DEFAULT_HTML_SURFACE_CONTROL_LABELS, ...labels }; const penLabel = armed ? labels?.annotateLabel : labels?.interactLabel; + const showRefresh = canRefresh && !!onRefresh; return ( <> - {/* Show/hide tools: removes ALL floating chrome (sidebar tongue tabs + + {/* The refresh and the eye share one group, left of the pen. Each + renders on its own terms: the refresh whenever it is offered + (canRefresh + onRefresh), the eye whenever onToggleTools is passed, + so a host without the tools toggle still gets its refresh. + + Show/hide tools: removes ALL floating chrome (sidebar tongue tabs + the comment/attachments cluster) from the DOM, leaving nothing over - the page. Sits left of the pen; this button is the only way back, - so it never hides itself. Eye = tools visible, eye-off = hidden. */} - {onToggleTools && ( + the page. This button is the only way back, so it never hides + itself. Eye = tools visible, eye-off = hidden. */} + {(showRefresh || onToggleTools) && (
- {canRefresh && onRefresh && ( + {showRefresh && ( )} + {onToggleTools && ( + )}
)} From ebae086007f58bf2df4da12aa7df8efa99694a71 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 27 Aug 2026 14:54:41 -0700 Subject: [PATCH 05/17] fix(annotations): make inReplyTo threading cycle-safe and refuse cycles at ingest exportAnnotations dropped every annotation that formed an inReplyTo cycle (reachable through PATCH /api/external-annotations, which merges arbitrary fields) while the header count still included it. The threading rule now lives once in @plannotator/core/annotation-threads (resolveReplyParents): an annotation is a reply only when its target is a different annotation in the list and the parent chain never returns to it; roots, orphans, self-references and every cycle member are emitted as roots in original order, so nothing is dropped and the count equals what is emitted. The export and AnnotationPanel.threadReplies both apply it. Both runtimes' PATCH handlers additionally validate inReplyTo (validateReplyTarget): it must name an existing, different annotation and must not close a cycle, otherwise 400, so the invalid state cannot be created there. Tests cover the helper, the export, the panel, and ingest on Bun and Pi; vendor.sh vendors the new core module. --- .../server/external-annotations.test.ts | 71 ++++++++++++ .../server/external-annotations.ts | 29 +++-- apps/pi-extension/vendor.sh | 2 +- packages/core/annotation-threads.test.ts | 60 +++++++++++ packages/core/annotation-threads.ts | 102 ++++++++++++++++++ packages/core/external-annotation.ts | 4 + packages/core/package.json | 1 + packages/server/external-annotations.test.ts | 41 +++++++ packages/server/external-annotations.ts | 22 ++-- .../AnnotationPanel.inReplyTo.test.tsx | 8 ++ packages/ui/components/AnnotationPanel.tsx | 28 ++--- packages/ui/utils/parser.inReplyTo.test.ts | 21 ++++ packages/ui/utils/parser.ts | 14 ++- 13 files changed, 370 insertions(+), 33 deletions(-) create mode 100644 apps/pi-extension/server/external-annotations.test.ts create mode 100644 packages/core/annotation-threads.test.ts create mode 100644 packages/core/annotation-threads.ts diff --git a/apps/pi-extension/server/external-annotations.test.ts b/apps/pi-extension/server/external-annotations.test.ts new file mode 100644 index 000000000..997f8e631 --- /dev/null +++ b/apps/pi-extension/server/external-annotations.test.ts @@ -0,0 +1,71 @@ +/** + * External annotations (Pi/Node): PATCH ingest of `inReplyTo`. + * + * Node mirror of the PATCH describe in packages/server/external-annotations.test.ts: + * PATCH merges arbitrary fields, so it was the one way to create an inReplyTo + * self-reference or cycle; the invalid state is refused at ingest on both + * runtimes. + */ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { createServer, type Server } from "node:http"; +import { createExternalAnnotationHandler } from "./external-annotations.ts"; +import { requestUrl } from "./helpers.ts"; + +describe("pi external annotations: PATCH inReplyTo", () => { + const handler = createExternalAnnotationHandler("plan"); + let server: Server; + let base = ""; + + beforeAll(async () => { + server = createServer(async (req, res) => { + const handled = await handler.handle(req, res, requestUrl(req)); + if (!handled) { + res.writeHead(404); + res.end(); + } + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const address = server.address(); + if (!address || typeof address === "string") throw new Error("no port"); + base = `http://127.0.0.1:${address.port}`; + }); + + afterAll(() => { + server.close(); + }); + + const patch = async (id: string, body: unknown) => { + const res = await fetch(`${base}/api/external-annotations?id=${encodeURIComponent(id)}`, { + method: "PATCH", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }); + return { status: res.status, body: (await res.json()) as { error?: string; annotation?: { inReplyTo?: string } } }; + }; + + test("refuses an inReplyTo that is self, missing, or would close a cycle; accepts a valid reply", async () => { + const added = handler.addAnnotations({ + annotations: [ + { source: "tool", text: "first" }, + { source: "tool", text: "second" }, + ], + }); + if ("error" in added) throw new Error(added.error); + const [first, second] = added.ids; + + expect((await patch(first, { inReplyTo: first })).status).toBe(400); + expect((await patch(first, { inReplyTo: "nope" })).status).toBe(400); + expect((await patch(first, { inReplyTo: 7 })).status).toBe(400); + + const ok = await patch(second, { inReplyTo: first }); + expect(ok.status).toBe(200); + expect(ok.body.annotation?.inReplyTo).toBe(first); + + const cycle = await patch(first, { inReplyTo: second }); + expect(cycle.status).toBe(400); + expect(cycle.body.error).toContain("cycle"); + + expect((await patch(second, { inReplyTo: null })).status).toBe(200); + expect((await patch(second, { text: "still fine" })).status).toBe(200); + }); +}); diff --git a/apps/pi-extension/server/external-annotations.ts b/apps/pi-extension/server/external-annotations.ts index 4ca376b1b..ef0eccdb8 100644 --- a/apps/pi-extension/server/external-annotations.ts +++ b/apps/pi-extension/server/external-annotations.ts @@ -14,6 +14,7 @@ import { serializeSSEEvent, HEARTBEAT_COMMENT, HEARTBEAT_INTERVAL_MS, + validateReplyTarget, type StorableAnnotation, type ExternalAnnotationEvent, } from "../generated/external-annotation.ts"; @@ -146,17 +147,31 @@ export function createExternalAnnotationHandler(mode: "plan" | "review") { json(res, { error: "Missing ?id parameter" }, 400); return true; } + let body: unknown; try { - const body = await parseBody(req); - const updated = store.update(id, body as Partial); - if (!updated) { - json(res, { error: "Not found" }, 404); - return true; - } - json(res, { annotation: updated }); + body = await parseBody(req); } catch { json(res, { error: "Invalid JSON" }, 400); + return true; + } + // A reply must point at an existing, different annotation and must + // not close a cycle: the export and the panel treat cycle members as + // roots, but the invalid state should not be creatable in the first + // place. (POST never carries inReplyTo, so PATCH is the only ingest.) + // Mirrors packages/server/external-annotations.ts. + if (body && typeof body === "object" && "inReplyTo" in body) { + const problem = validateReplyTarget(store.getAll(), id, (body as { inReplyTo?: unknown }).inReplyTo); + if (problem) { + json(res, { error: problem }, 400); + return true; + } + } + const updated = store.update(id, body as Partial); + if (!updated) { + json(res, { error: "Not found" }, 404); + return true; } + json(res, { annotation: updated }); return true; } diff --git a/apps/pi-extension/vendor.sh b/apps/pi-extension/vendor.sh index 478928edf..844c9bdc7 100755 --- a/apps/pi-extension/vendor.sh +++ b/apps/pi-extension/vendor.sh @@ -8,7 +8,7 @@ rm -rf generated mkdir -p generated generated/ai/providers # Modules that MOVED to @plannotator/core — vendor the real impl from core. -for f in feedback-templates project favicon code-file annotatable external-annotation agent-jobs agent-terminal source-save open-in-apps diff-paths diff-files guide guide-format guide-viewer-manifest compress crypto; do +for f in feedback-templates project favicon code-file annotatable annotation-threads external-annotation agent-jobs agent-terminal source-save open-in-apps diff-paths diff-files guide guide-format guide-viewer-manifest compress crypto; do src="../../packages/core/$f.ts" printf '// @generated — DO NOT EDIT. Source: packages/core/%s.ts\n' "$f" | cat - "$src" > "generated/$f.ts" done diff --git a/packages/core/annotation-threads.test.ts b/packages/core/annotation-threads.test.ts new file mode 100644 index 000000000..9069e0473 --- /dev/null +++ b/packages/core/annotation-threads.test.ts @@ -0,0 +1,60 @@ +/** + * The one threading rule every `inReplyTo` consumer applies. Failures to + * catch: a cycle member being treated as a reply (and then dropped by a + * renderer that only walks down from roots), a self-reference threading + * under itself, and the ingest accepting a write that would close a cycle. + */ +import { describe, expect, test } from "bun:test"; +import { resolveReplyParents, validateReplyTarget } from "./annotation-threads"; + +const a = (id: string, inReplyTo?: string) => ({ id, inReplyTo }); + +describe("resolveReplyParents", () => { + test("valid replies keep their parent; roots, orphans and self-references are roots", () => { + const parents = resolveReplyParents([a("p"), a("r", "p"), a("orphan", "gone"), a("self", "self"), a("deep", "r")]); + expect(parents.get("p")).toBeNull(); + expect(parents.get("r")).toBe("p"); + expect(parents.get("orphan")).toBeNull(); + expect(parents.get("self")).toBeNull(); + expect(parents.get("deep")).toBe("r"); + }); + + test("every member of a cycle is a root, and a reply to a cycle member still threads under it", () => { + const parents = resolveReplyParents([a("x", "y"), a("y", "x"), a("c", "x"), a("l1", "l3"), a("l2", "l1"), a("l3", "l2")]); + expect(parents.get("x")).toBeNull(); + expect(parents.get("y")).toBeNull(); + expect(parents.get("c")).toBe("x"); + expect(parents.get("l1")).toBeNull(); + expect(parents.get("l2")).toBeNull(); + expect(parents.get("l3")).toBeNull(); + }); + + test("a reply whose parent is an orphan is still a reply (the orphan renders as a root)", () => { + const parents = resolveReplyParents([a("o", "gone"), a("r", "o")]); + expect(parents.get("o")).toBeNull(); + expect(parents.get("r")).toBe("o"); + }); +}); + +describe("validateReplyTarget", () => { + const all = [a("p"), a("r", "p"), a("x", "y"), a("y", "x")]; + + test("accepts an existing different target and clearing", () => { + expect(validateReplyTarget(all, "x", "p")).toBeNull(); + expect(validateReplyTarget(all, "r", null)).toBeNull(); + expect(validateReplyTarget(all, "r", undefined)).toBeNull(); + }); + + test("rejects self, missing, non-string and cycle-closing targets", () => { + expect(validateReplyTarget(all, "p", "p")).toContain("itself"); + expect(validateReplyTarget(all, "p", "nope")).toContain("nope"); + expect(validateReplyTarget(all, "p", 42)).toContain("inReplyTo"); + expect(validateReplyTarget(all, "p", "")).toContain("inReplyTo"); + // r -> p; setting p -> r closes p -> r -> p. + expect(validateReplyTarget(all, "p", "r")).toContain("cycle"); + }); + + test("a pre-existing cycle elsewhere does not block an unrelated reply", () => { + expect(validateReplyTarget(all, "p", "x")).toBeNull(); + }); +}); diff --git a/packages/core/annotation-threads.ts b/packages/core/annotation-threads.ts new file mode 100644 index 000000000..03735fbf4 --- /dev/null +++ b/packages/core/annotation-threads.ts @@ -0,0 +1,102 @@ +/** + * Reply threading (`inReplyTo`) rules shared by every consumer: the feedback + * export, the annotations panel, and the external-annotation ingest. + * + * Browser-safe, zero-dep. `inReplyTo` is an additive field whose value can + * come from anywhere (a browser agent's tool call, a PATCH on + * /api/external-annotations that merges arbitrary fields), so consumers must + * never trust it to form a tree. The one rule every consumer applies: + * + * An annotation is a reply only when its `inReplyTo` names a DIFFERENT + * annotation in the same list AND following the chain of parents never + * comes back to the annotation itself. Everything else is a root: a plain + * comment, an orphan whose parent is absent, a self-reference, and every + * member of a cycle. Roots keep their original order. A reply to a cycle + * member stays a reply: its parent is rendered (as a root). + * + * Consequence: no annotation is ever dropped from a threaded rendering, and + * a reply always hangs under something that is itself rendered. + */ + +export interface ThreadableAnnotation { + id: string; + inReplyTo?: string | null; +} + +/** + * The effective parent of every annotation in `items`: the `inReplyTo` + * target for a valid reply, `null` for a root (see the module comment). + */ +export function resolveReplyParents( + items: readonly T[], +): Map { + const byId = new Map(); + for (const item of items) byId.set(item.id, item); + const parents = new Map(); + for (const item of items) { + const target = typeof item.inReplyTo === "string" ? item.inReplyTo : null; + if (!target || target === item.id || !byId.has(target)) { + parents.set(item.id, null); + continue; + } + // Walk up until the chain leaves the list or ends at a root. Coming back + // to the item itself means it is a MEMBER of a cycle: a root. A chain + // that merely leads into a cycle elsewhere leaves the item a reply of + // its parent, which is itself rendered (as a cycle member, a root). + const seen = new Set(); + let current: T | undefined = byId.get(target); + let member = false; + while (current) { + if (current.id === item.id) { + member = true; + break; + } + if (seen.has(current.id)) break; + seen.add(current.id); + const next = typeof current.inReplyTo === "string" ? current.inReplyTo : null; + current = next ? byId.get(next) : undefined; + } + parents.set(item.id, member ? null : target); + } + return parents; +} + +/** + * Validate an `inReplyTo` value about to be written onto annotation `id` + * (external-annotation PATCH ingest, both runtimes). A reply must point at + * an existing, different annotation, and must not close a cycle through the + * existing chain. `null`/`undefined` clear the field and are always valid. + * Returns the error message, or `null` when the value may be applied. + */ +export function validateReplyTarget( + all: readonly ThreadableAnnotation[], + id: string, + inReplyTo: unknown, +): string | null { + if (inReplyTo === undefined || inReplyTo === null) return null; + if (typeof inReplyTo !== "string" || inReplyTo.length === 0) { + return 'invalid "inReplyTo": must be the id of an existing annotation'; + } + if (inReplyTo === id) { + return 'invalid "inReplyTo": an annotation cannot reply to itself'; + } + const byId = new Map(); + for (const item of all) byId.set(item.id, item); + if (!byId.has(inReplyTo)) { + return `invalid "inReplyTo": no annotation with id "${inReplyTo}"`; + } + // Would the target's own chain lead back to `id`? Then the write would + // create a cycle. + const seen = new Set(); + let current: ThreadableAnnotation | undefined = byId.get(inReplyTo); + while (current) { + if (current.id === id) { + return 'invalid "inReplyTo": the reply chain would form a cycle'; + } + if (seen.has(current.id)) break; // pre-existing cycle elsewhere; not ours to close + seen.add(current.id); + const next = typeof current.inReplyTo === "string" ? current.inReplyTo : null; + current = next ? byId.get(next) : undefined; + } + return null; +} diff --git a/packages/core/external-annotation.ts b/packages/core/external-annotation.ts index 879fd4183..da2e8c568 100644 --- a/packages/core/external-annotation.ts +++ b/packages/core/external-annotation.ts @@ -10,6 +10,10 @@ * input transformers handle validation and field assignment. */ +// Reply-threading validation for PATCH ingest, re-exported so both HTTP +// adapters import it from the module they already use. +export { validateReplyTarget } from "./annotation-threads"; + // --------------------------------------------------------------------------- // Types // --------------------------------------------------------------------------- diff --git a/packages/core/package.json b/packages/core/package.json index e66c9a64d..bebb00cb3 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -9,6 +9,7 @@ "./guide": "./guide.ts", "./guide-format": "./guide-format.ts", "./annotatable": "./annotatable.ts", + "./annotation-threads": "./annotation-threads.ts", "./agent-jobs": "./agent-jobs.ts", "./agent-terminal": "./agent-terminal.ts", "./browser-paths": "./browser-paths.ts", diff --git a/packages/server/external-annotations.test.ts b/packages/server/external-annotations.test.ts index 7b3872482..61a055705 100644 --- a/packages/server/external-annotations.test.ts +++ b/packages/server/external-annotations.test.ts @@ -58,4 +58,45 @@ describe("PATCH /api/external-annotations", () => { expect(edited.annotation.text).toBe("edited text"); expect(edited.annotation.source).toBe("rogue-agent"); }); + + // PATCH merges arbitrary fields, so it was the one way to create an + // inReplyTo self-reference or cycle (which the export used to drop while + // still counting). The invalid state is refused at ingest. + test("refuses an inReplyTo that is self, missing, or would close a cycle; accepts a valid reply", async () => { + const handler = createExternalAnnotationHandler("plan"); + const added = handler.addAnnotations({ + annotations: [ + { source: "tool", text: "first" }, + { source: "tool", text: "second" }, + ], + }); + if ("error" in added) throw new Error(added.error); + const [first, second] = added.ids; + + const patch = async (id: string, body: unknown) => { + const url = `http://localhost/api/external-annotations?id=${encodeURIComponent(id)}`; + const res = await handler.handle( + new Request(url, { method: "PATCH", headers: { "Content-Type": "application/json" }, body: JSON.stringify(body) }), + new URL(url), + ); + return { status: res!.status, body: (await res!.json()) as { error?: string; annotation?: { inReplyTo?: string } } }; + }; + + expect((await patch(first, { inReplyTo: first })).status).toBe(400); + expect((await patch(first, { inReplyTo: "nope" })).status).toBe(400); + expect((await patch(first, { inReplyTo: 7 })).status).toBe(400); + + const ok = await patch(second, { inReplyTo: first }); + expect(ok.status).toBe(200); + expect(ok.body.annotation?.inReplyTo).toBe(first); + + // second -> first is in place; first -> second would close the loop. + const cycle = await patch(first, { inReplyTo: second }); + expect(cycle.status).toBe(400); + expect(cycle.body.error).toContain("cycle"); + + // Clearing stays allowed, and an unrelated patch does not touch the field. + expect((await patch(second, { inReplyTo: null })).status).toBe(200); + expect((await patch(second, { text: "still fine" })).status).toBe(200); + }); }); diff --git a/packages/server/external-annotations.ts b/packages/server/external-annotations.ts index 0f49be66f..3cf2eb964 100644 --- a/packages/server/external-annotations.ts +++ b/packages/server/external-annotations.ts @@ -16,6 +16,7 @@ import { serializeSSEEvent, HEARTBEAT_COMMENT, HEARTBEAT_INTERVAL_MS, + validateReplyTarget, type AnnotationStore, type StorableAnnotation, type ExternalAnnotationEvent, @@ -169,16 +170,25 @@ export function createExternalAnnotationHandler( if (!id) { return Response.json({ error: "Missing ?id parameter" }, { status: 400 }); } + let body: unknown; try { - const body = await req.json(); - const updated = store.update(id, body as Partial); - if (!updated) { - return Response.json({ error: "Not found" }, { status: 404 }); - } - return Response.json({ annotation: updated }); + body = await req.json(); } catch { return Response.json({ error: "Invalid JSON" }, { status: 400 }); } + // A reply must point at an existing, different annotation and must + // not close a cycle: the export and the panel treat cycle members as + // roots, but the invalid state should not be creatable in the first + // place. (POST never carries inReplyTo, so PATCH is the only ingest.) + if (body && typeof body === "object" && "inReplyTo" in body) { + const problem = validateReplyTarget(store.getAll(), id, (body as { inReplyTo?: unknown }).inReplyTo); + if (problem) return Response.json({ error: problem }, { status: 400 }); + } + const updated = store.update(id, body as Partial); + if (!updated) { + return Response.json({ error: "Not found" }, { status: 404 }); + } + return Response.json({ annotation: updated }); } // --- DELETE (by id, by source, or clear all) --- diff --git a/packages/ui/components/AnnotationPanel.inReplyTo.test.tsx b/packages/ui/components/AnnotationPanel.inReplyTo.test.tsx index 95b3d09c7..8bae8c9ea 100644 --- a/packages/ui/components/AnnotationPanel.inReplyTo.test.tsx +++ b/packages/ui/components/AnnotationPanel.inReplyTo.test.tsx @@ -55,6 +55,14 @@ describe.skipIf(!hasDom)('AnnotationPanel inReplyTo threading', () => { expect([...el.querySelectorAll('[data-annotation-id]')].map((n) => n.getAttribute('data-annotation-id'))).toEqual(['a', 'r']); }); + test('an inReplyTo cycle renders every member as a top-level card, in order, and a reply to one still threads', async () => { + const el = await render([ann('x', 1, { inReplyTo: 'y' }), ann('y', 2, { inReplyTo: 'x' }), ann('r', 3, { inReplyTo: 'x' })]); + expect([...el.querySelectorAll('[data-annotation-id]')].map((n) => n.getAttribute('data-annotation-id'))).toEqual(['x', 'r', 'y']); + const replies = el.querySelectorAll('[data-annotation-reply="true"]'); + expect(replies.length).toBe(1); + expect(replies[0].querySelector('[data-annotation-id="r"]')).not.toBeNull(); + }); + test('without replies there is no reply wrapper and the order is creation order', async () => { const el = await render([ann('b', 2), ann('a', 1)]); expect(el.querySelectorAll('[data-annotation-reply="true"]').length).toBe(0); diff --git a/packages/ui/components/AnnotationPanel.tsx b/packages/ui/components/AnnotationPanel.tsx index b4e94cd79..a2981f633 100644 --- a/packages/ui/components/AnnotationPanel.tsx +++ b/packages/ui/components/AnnotationPanel.tsx @@ -7,6 +7,7 @@ import { useIsMobile } from '../hooks/useIsMobile'; import { OverlayScrollArea } from './OverlayScrollArea'; import { Button } from './ui/button'; import { cn } from '../lib/utils'; +import { resolveReplyParents } from '@plannotator/core/annotation-threads'; // Card type-word colors. Deletion uses `destructive` (reliably red on every // theme, matching the in-document .deletion highlight). Comment uses the @@ -40,34 +41,33 @@ const TrashCardIcon = () => ( /** * Order annotations so every reply follows its parent (replies among - * themselves stay in creation order). A reply whose parent is absent renders - * as a top-level card. Without any `inReplyTo` the input order is returned - * unchanged, so annotations without replies render exactly as before. + * themselves stay in creation order). The threading rule is the shared one + * (resolveReplyParents, also what the export applies): a reply whose parent + * is absent, a self-reference, and every member of an `inReplyTo` cycle + * render as top-level cards in input order, so nothing is ever dropped. + * Without any `inReplyTo` the input order is returned unchanged, so + * annotations without replies render exactly as before. */ export function threadReplies(sorted: Annotation[]): Array<{ annotation: Annotation; isReply: boolean }> { if (!sorted.some((a) => a.inReplyTo)) return sorted.map((annotation) => ({ annotation, isReply: false })); - const ids = new Set(sorted.map((a) => a.id)); + const parents = resolveReplyParents(sorted); const byParent = new Map(); for (const a of sorted) { - if (a.inReplyTo && ids.has(a.inReplyTo) && a.inReplyTo !== a.id) { - const list = byParent.get(a.inReplyTo) ?? []; - list.push(a); - byParent.set(a.inReplyTo, list); - } + const parent = parents.get(a.id); + if (!parent) continue; + const list = byParent.get(parent) ?? []; + list.push(a); + byParent.set(parent, list); } const out: Array<{ annotation: Annotation; isReply: boolean }> = []; - const emitted = new Set(); const emit = (a: Annotation, isReply: boolean) => { - if (emitted.has(a.id)) return; - emitted.add(a.id); out.push({ annotation: a, isReply }); for (const reply of byParent.get(a.id) ?? []) emit(reply, true); }; for (const a of sorted) { - if (a.inReplyTo && ids.has(a.inReplyTo) && a.inReplyTo !== a.id) continue; + if (parents.get(a.id)) continue; emit(a, false); } - for (const a of sorted) emit(a, false); return out; } diff --git a/packages/ui/utils/parser.inReplyTo.test.ts b/packages/ui/utils/parser.inReplyTo.test.ts index a0fb244fa..b8c032330 100644 --- a/packages/ui/utils/parser.inReplyTo.test.ts +++ b/packages/ui/utils/parser.inReplyTo.test.ts @@ -47,6 +47,27 @@ describe('exportAnnotations with inReplyTo', () => { expect(out).not.toContain('**Reply'); }); + // Reachable through PATCH /api/external-annotations before ingest refused + // it, and still possible in drafts: a cycle used to be dropped from the + // body while the header still counted it. + test('an inReplyTo cycle drops nothing: its members are roots in original order and the count matches', () => { + const x = comment('x', 'Rotate the key', 'X says', { inReplyTo: 'y', createdA: 1 }); + const y = comment('y', 'Rotate the key', 'Y says', { inReplyTo: 'x', createdA: 2 }); + const self = comment('s', 'Ship behind a flag', 'Self says', { inReplyTo: 's', createdA: 3 }); + const reply = comment('r', 'Rotate the key', 'Reply to X', { inReplyTo: 'x', author: 'tater', createdA: 4 }); + const out = exportAnnotations(blocks, [x, y, self, reply]); + expect(out).toContain('have 4 pieces of feedback'); + for (const text of ['X says', 'Y says', 'Self says']) expect(out).toContain(text); + // Three roots, numbered consecutively; the valid reply nests under x. + expect(out).toContain('## 1. '); + expect(out).toContain('## 2. '); + expect(out).toContain('## 3. '); + expect(out).not.toContain('## 4. '); + expect(out).toContain('- **Reply (tater):** Reply to X'); + expect(out.indexOf('X says')).toBeLessThan(out.indexOf('Y says')); + expect(out.indexOf('Reply to X')).toBeLessThan(out.indexOf('Y says')); + }); + test('without any inReplyTo the export is byte-identical to the plain export', () => { const a = comment('a', 'Rotate the key', 'one'); const b = comment('b', 'Ship behind a flag', 'two', { createdA: 2 }); diff --git a/packages/ui/utils/parser.ts b/packages/ui/utils/parser.ts index 28f4bcda1..31d9f4828 100644 --- a/packages/ui/utils/parser.ts +++ b/packages/ui/utils/parser.ts @@ -1,5 +1,6 @@ import type { Block, Annotation, CodeAnnotation, EditorAnnotation, ImageAttachment } from '../types'; import { planDenyFeedback } from '@plannotator/core/feedback-templates'; +import { resolveReplyParents } from '@plannotator/core/annotation-threads'; import { skillReferenceExportBlock } from './skillReferences'; /** @@ -1210,14 +1211,17 @@ export const exportAnnotations = ( // Threaded replies (`inReplyTo`): a reply is emitted as a nested exchange // under its parent's entry rather than as its own numbered entry, so the - // coding agent reads the conversation in order. Replies whose parent is - // not in the export render as ordinary entries. With no `inReplyTo` + // coding agent reads the conversation in order. The threading rule is the + // shared one (resolveReplyParents): a reply whose parent is not in the + // export, a self-reference, and every member of an inReplyTo cycle render + // as ordinary entries in original order, so no annotation is ever dropped + // and the header count always equals what is emitted. With no `inReplyTo` // anywhere the output is byte-identical to the ungrouped export. - const exportedIds = new Set(sortedAnns.map((a: any) => a.id)); - const isReply = (a: any) => typeof a.inReplyTo === 'string' && a.inReplyTo !== a.id && exportedIds.has(a.inReplyTo); + const replyParents = resolveReplyParents(sortedAnns as any[]); + const isReply = (a: any) => replyParents.get(a.id) != null; const hasReplies = sortedAnns.some(isReply); const repliesOf = (parent: any): any[] => - hasReplies ? sortedAnns.filter((a: any) => isReply(a) && a.inReplyTo === parent.id).sort((a: any, b: any) => a.createdA - b.createdA) : []; + hasReplies ? sortedAnns.filter((a: any) => replyParents.get(a.id) === parent.id).sort((a: any, b: any) => a.createdA - b.createdA) : []; if (hasReplies) { emitOrder = emitOrder.filter((a) => !isReply(a)); // Numbers stay consecutive over the entries that are actually emitted. From 4142494f49d266891680900f27f1c3604233a224 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 27 Aug 2026 14:55:28 -0700 Subject: [PATCH 06/17] docs: describe HTML Refresh, the threading rule, and the WebMCP design pointer CLAUDE.md/AGENTS.md gains an HTML Refresh paragraph under the Annotation System (what Refresh does, which anchors survive, the Unanchored chip, the recomputed version diff, the compact-shell menu action, and that URL and live-app sessions have no Refresh), notes the shared inReplyTo threading rule and the PATCH validation, and stops pointing the WebMCP section at an untracked design file. The marketing annotate page documents Refresh in its HTML section, and the WebMCP reference lists the comment_only_surface nudge that packages/ui/webmcp/nudges.ts already emits. --- AGENTS.md | 6 ++++-- apps/marketing/src/content/docs/commands/annotate.md | 4 ++++ apps/marketing/src/content/docs/reference/webmcp-tools.md | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1dada1bdb..16c342f4a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -639,11 +639,13 @@ Text highlighting uses `web-highlighter` library. Code blocks use manual ` These surfaces are **comment-only**. `redline` (auto-DELETION) and `quickLabel` are clamped at the trust boundary, which is the parent's postMessage ingest rather than the server, covering the host mode and a page-supplied `modeOverride` alike so a hostile page cannot force a DELETION (`packages/ui/components/html-viewer/useHtmlAnnotation.ts:535-547`). Only CREATION is restricted: persisted DELETION annotations still restore and still render their deletion styling (`useHtmlAnnotation.ts:903`). The selection toolbar drops Delete behind a `commentOnly` seam and is passed no quick-label handler (`packages/ui/components/AnnotationToolbar.tsx:217-224`, `HtmlViewer.tsx:880-883`); markdown surfaces keep the full toolbar. HTML surfaces also pin the viewer input method to pinpoint (`App.tsx:5480`), so there is no floating input-method toolstrip on them at all (`toolstripVisible` is gated on `!isHtmlSurface`, `App.tsx:2788-2793`) and the `Shift+1`-`4` annotation-mode shortcuts cannot fire there. A header **eye** button immediately left of the pen toggles Show/Hide tools: hiding REMOVES all floating chrome over the page from the DOM (the sidebar tongue tabs and the comment/attachments cluster) rather than merely hiding it (`AppHeader.tsx:364-385`, `App.tsx:5193`, `HtmlViewer.tsx:810`). The toggle lives in the header, so a hidden state always has a way back, which is what makes honoring the persisted `toolsHidden` cookie safe (`packages/ui/utils/htmlChrome.ts:17-21`). +**HTML Refresh (#1232).** A local rendered-HTML session can re-read its file from disk without reloading the tab, for the loop where an agent edits the page while the reviewer keeps annotating. The header **Refresh** button (left of the eye, `data-html-refresh`, titled "Refresh HTML from disk") fetches the active document through `/api/doc`, hands the bytes to the app, and remounts the viewer under a bumped `reloadGeneration` key (`packages/editor/App.tsx`, viewer `key`). The engine is the published `useHtmlRefresh` (`packages/ui/hooks/useHtmlRefresh.ts`: superseded and cross-document fetches are dropped, one restore acknowledgement per generation) and Plannotator's binding over `fetchHtmlDocumentSnapshot` is `packages/editor/hooks/useHtmlRefresh.ts` (toasts for refreshed, missing, and unavailable). Committed annotations survive on their durable anchors: the remounted viewer re-resolves every element selector and text snapshot against the new page, and the ones it cannot re-anchor are reported once (`onUnanchoredChange` to `reportAnnotationRestore`), toasted, and marked with an **Unanchored** chip in the annotations panel (`htmlUnanchoredIds` in App, cleared when the document changes); their comments stay in the panel and still export. A refresh keeps the version diff: for the root document `/api/doc` carries `previousPlan`/`versionInfo`/`diffHtml` recomputed against the bytes just read (see the annotate `/api/plan` row), `applyRefreshedHtml` sets them and resets `isPlanDiffActive`, so the view returns to normal mode with "Show changes" still available; a tab reload converges on the same state because `/api/plan` serves the current bytes and recomputes the same diff. `/api/share-html` shares the current bytes too. Only local files refresh: `canRefresh` is false for `http(s)` paths and live-app sessions, and the control is absent on read-only (archive) documents. The compact touch shell renders no header controls (`HtmlSurfaceControls` returns null when `compact`), so its Options menu offers "Refresh from disk" beside the Show/Hide tools and Interact/Annotate actions (`compactDocumentActions` in App, disabled while a refresh is in flight); a host that passes `canRefresh` and `onRefresh` to `HtmlSurfaceControls` gets the Refresh button with or without the eye. + Known limitation: printing a raw-HTML annotate session prints highlight stripes from a best-effort absolute-coordinate layer and is degraded inside the iframe (pre-existing); element-only targets (SVG anchors, multi-select additional element targets) have no print representation. ## WebMCP (browser-agent tools) -Design of record: `DESIGN_webmcp-support.md` (untracked). Phase 1 makes plan review and every annotate surface a WebMCP **provider**: a browser-integrated agent (Chrome/Edge origin trial, `chrome://flags/#enable-webmcp-testing` or `--enable-features=WebMCPTesting` locally; agent-embedded browsers unflagged) calls in-page tools instead of scraping the DOM. Code review (phase 2) and consuming the annotated app's own tools (phase 3) are not built. +The design document lives outside the tree (it is not checked in); the user-facing reference is `apps/marketing/src/content/docs/reference/webmcp-tools.md`. Phase 1 makes plan review and every annotate surface a WebMCP **provider**: a browser-integrated agent (Chrome/Edge origin trial, `chrome://flags/#enable-webmcp-testing` or `--enable-features=WebMCPTesting` locally; agent-embedded browsers unflagged) calls in-page tools instead of scraping the DOM. Code review (phase 2) and consuming the annotated app's own tools (phase 3) are not built. **Shape (mirrors the shortcut system).** Engine in `packages/ui/webmcp/`: `modelContext.ts` is the ONLY file that spells `document.modelContext`, `registerTool`, `getTools`, `executeTool`, `toolchange` and the annotation hints (local structural types, no `webmcp-types` dependency, no `declare global`); `toolset.ts` (tool specs, the `{ ok, data, nudges, error? }` envelope, `runTool`, a per-document registry with reconcile-by-name and one `AbortController` per tool, since unregistration is only by abort); `changes.ts` (per-annotation `seq`, tombstones, per-tab watermark with `since` override, `claimOwn` so the agent's own writes are never "new" to it); `nudges.ts` (the twelve codes: `annotations_new`, `annotations_removed`, `replies_new`, `composer_open`, `source_stale`, `document_edited`, `comment_only_surface`, `page_changed`, `other_document_active`, `pending_unsent`, `session_decided`, `truncated`; messages are static strings, document and comment text never enter a message); `useToolset.ts` (React hook; handlers read through refs so a re-render never touches `registerTool`); `policy.ts` (the `webmcp` seam on `configurePlannotatorUI`, `{ enabled, namePrefix }`, default enabled with prefix `plannotator.`). Catalog in `packages/editor/webmcp/`: `documentTools.ts` builds the tools over a narrow `DocumentToolAdapter` (never imports App), `documentText.ts` holds the pure outline / windowing / quote-resolution helpers, `useDocumentWebMcp.ts` builds the adapter over App state through one ref. @@ -653,7 +655,7 @@ Design of record: `DESIGN_webmcp-support.md` (untracked). Phase 1 makes plan rev **Folder sessions.** `list_documents` walks the file browser's loaded directories (`fileBrowser.dirs`, absolute path = `${dir.path}/${node.path}`, vault dirs excluded) so every document is listed, not only the ones already visited. The agent learns that the human navigated from `document.path` on its next response, not from a sibling flag: siblings exclude the open path, and after a sidebar click `fileBrowser.activeFile` equals `linkedDoc.filepath`, so a sibling with `open: true` (and with it `openedSinceLastRead` and the "opened" branch of `other_document_active`) exists only transiently, during the load window between the click and the document commit. `reveal { path }` answers `not_found` at once for a path that is neither in the folder tree nor in the linked-doc cache; otherwise it navigates (folder sessions through App's file-browser selection handler, so the active file, the doc URL and the linked document stay in step) and WAITS for the commit that makes that path the open document (an effect settles the waiter, and the linked-doc `error` state settles it early when the load fails, so a bad path never runs out the 5s timeout) before looking the comment or section up; reading state right after the `await` would see the pre-navigation document. After a tool mutation the adapter overlays the pending write on the last committed annotation list (`applyOverlay`) so the response's nudges and the new comment's `seq` reflect the mutation even though `setAnnotations` has not committed yet; the agent's own removals are claimed (`claimRemoved`) so they are never reported back to it as `annotations_removed`. Still not reachable in phase 1: `composerOpen` for a sibling (the composer is detected from the DOM of the open document only, so `other_document_active` never fires for a composer in another document), and writes to a sibling that is not open (see below). -**`inReplyTo`.** One additive field on `Annotation`: a reply inherits its parent's anchor, renders indented under it in the annotations panel (`threadReplies` in `AnnotationPanel.tsx`), and exports nested under the parent's entry (`**Replies:**` block in `exportAnnotations`); an annotation without it renders and exports byte-identically to before. Drafts carry it (annotations are opaque JSON to the draft transport); share links deliberately do not (a reply shares as a plain comment on the same quote, the existing text-restore contract, pinned by `sharing.inReplyTo.test.ts`). Known limitation: comments on a sibling document that is not open answer `not_available` with a hint to `reveal { path }` first, because the linked-doc cache is a copy. +**`inReplyTo`.** One additive field on `Annotation`: a reply inherits its parent's anchor, renders indented under it in the annotations panel (`threadReplies` in `AnnotationPanel.tsx`), and exports nested under the parent's entry (`**Replies:**` block in `exportAnnotations`); an annotation without it renders and exports byte-identically to before. The threading rule is shared (`resolveReplyParents` in `packages/core/annotation-threads.ts`): an annotation is a reply only when its target is a different annotation in the same list and the parent chain never returns to it; orphans, self-references, and every member of a cycle render and export as roots in original order, so nothing is ever dropped and the export's header count equals what is emitted. `PATCH /api/external-annotations` refuses an `inReplyTo` that is self, missing, or would close a cycle (`validateReplyTarget`, both runtimes, `400`). Drafts carry it (annotations are opaque JSON to the draft transport); share links deliberately do not (a reply shares as a plain comment on the same quote, the existing text-restore contract, pinned by `sharing.inReplyTo.test.ts`). Known limitation: comments on a sibling document that is not open answer `not_available` with a hint to `reveal { path }` first, because the linked-doc cache is a copy. Docs: `apps/marketing/src/content/docs/reference/webmcp-tools.md` (the user-facing reference) and the manual five-flow checklist in `tests/UI-TESTING.md`. diff --git a/apps/marketing/src/content/docs/commands/annotate.md b/apps/marketing/src/content/docs/commands/annotate.md index d2d73219e..42ae02b7c 100644 --- a/apps/marketing/src/content/docs/commands/annotate.md +++ b/apps/marketing/src/content/docs/commands/annotate.md @@ -147,6 +147,10 @@ Markdown conversion uses [Turndown](https://github.com/mixmark-io/turndown) with HTML files must be within your current working directory. Files outside the project root return a 403 error. +### Refresh from disk + +When an agent edits the HTML file while you are reviewing it, click **Refresh** in the header (next to the eye button) to re-read the file without reloading the tab. Annotations whose elements or text still exist on the new page stay where they were; any that no longer match are listed in a notice and marked **Unanchored** in the annotations panel, where their comments remain and still send. The "Show changes" toggle keeps working after a refresh, comparing the file as it is now against its previous saved version. On a phone or tablet the action is in the Options menu as **Refresh from disk**. Refresh is available for local HTML files only, not for URLs or live app sessions. + ### `--markdown` For local HTML files, `--markdown` switches from raw HTML rendering to markdown conversion. In folder mode, the same setting applies when you open `.html` or `.htm` files from the file browser. diff --git a/apps/marketing/src/content/docs/reference/webmcp-tools.md b/apps/marketing/src/content/docs/reference/webmcp-tools.md index 4d2590a0f..902869bea 100644 --- a/apps/marketing/src/content/docs/reference/webmcp-tools.md +++ b/apps/marketing/src/content/docs/reference/webmcp-tools.md @@ -28,7 +28,7 @@ All tools are registered under the `plannotator.` prefix. | `nudge_user` | Show you one short, transient message in the page (280 characters), for example "Finished: two comments, nothing blocking, ready for your approval." Not saved, not part of the feedback, dismissible. | | `list_documents` | Folder sessions only: the document tree with per-document comment counts and what changed since the agent last read each one. | -Every response carries `nudges`: short machine-readable notices computed from state the page already holds, such as `annotations_new` (you added or edited comments since the last read), `replies_new`, `annotations_removed` (you deleted one of the agent's comments, which the agent should treat as resolved), `composer_open` (you are typing right now), `source_stale`, `document_edited`, `page_changed`, `other_document_active`, `truncated`, `pending_unsent`, and `session_decided`. +Every response carries `nudges`: short machine-readable notices computed from state the page already holds, such as `annotations_new` (you added or edited comments since the last read), `replies_new`, `annotations_removed` (you deleted one of the agent's comments, which the agent should treat as resolved), `composer_open` (you are typing right now), `source_stale`, `document_edited`, `comment_only_surface` (an HTML or live app page, where comments anchor on a text quote or the whole document and nothing can be marked for deletion), `page_changed`, `other_document_active`, `truncated`, `pending_unsent`, and `session_decided`. Comments the agent creates appear in the annotations panel like any other external-tool comment, labeled `browser-agent`. A reply threads under the comment it answers and is exported nested under it, so the coding agent reads the exchange in order. From 040a21732a83d459661d27ae22c770b59e2a56c9 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 27 Aug 2026 15:06:44 -0700 Subject: [PATCH 07/17] perf(core): resolve reply threads in linear time resolveReplyParents walked every chain to its root without memoization, so a 5,000-deep inReplyTo chain cost 12.5 million steps (431 ms; 20,000 took 9.8 s), and the panel re-ran it on every render. Each id is now classified once with path compression, cycle members are still roots and the run-in into a cycle stays replies, and the new resolveThreadRootTimestamps gives the panel its thread ordering from one linear pass. A 5,000-chain timing test guards both. --- packages/core/annotation-threads.test.ts | 54 ++++++++++++- packages/core/annotation-threads.ts | 97 +++++++++++++++++++----- 2 files changed, 131 insertions(+), 20 deletions(-) diff --git a/packages/core/annotation-threads.test.ts b/packages/core/annotation-threads.test.ts index 9069e0473..1d85b560f 100644 --- a/packages/core/annotation-threads.test.ts +++ b/packages/core/annotation-threads.test.ts @@ -5,7 +5,7 @@ * under itself, and the ingest accepting a write that would close a cycle. */ import { describe, expect, test } from "bun:test"; -import { resolveReplyParents, validateReplyTarget } from "./annotation-threads"; +import { resolveReplyParents, resolveThreadRootTimestamps, validateReplyTarget } from "./annotation-threads"; const a = (id: string, inReplyTo?: string) => ({ id, inReplyTo }); @@ -34,6 +34,58 @@ describe("resolveReplyParents", () => { expect(parents.get("o")).toBeNull(); expect(parents.get("r")).toBe("o"); }); + + test("a chain that runs into a cycle: the run-in stays replies, the cycle is roots, in any input order", () => { + // c -> b -> x -> y -> x. Listed cycle-first and run-in-first. + for (const items of [ + [a("x", "y"), a("y", "x"), a("b", "x"), a("c", "b")], + [a("c", "b"), a("b", "x"), a("x", "y"), a("y", "x")], + ]) { + const parents = resolveReplyParents(items); + expect(parents.get("c")).toBe("b"); + expect(parents.get("b")).toBe("x"); + expect(parents.get("x")).toBeNull(); + expect(parents.get("y")).toBeNull(); + } + }); + + // A hostile or buggy tool can POST a 5,000-deep chain; walking each chain + // to its root without memoization made this quadratic (5,000 = 431 ms, + // 20,000 = 9.8 s), and the panel re-ran it on every render. + test("a 5,000-deep chain resolves parents and root timestamps in linear time", () => { + const items = [{ id: "0", inReplyTo: undefined as string | undefined, createdA: 0 }]; + for (let i = 1; i < 5000; i++) items.push({ id: String(i), inReplyTo: String(i - 1), createdA: i }); + const start = performance.now(); + const parents = resolveReplyParents(items); + const rootTs = resolveThreadRootTimestamps(items, parents); + const elapsed = performance.now() - start; + expect(parents.get("4999")).toBe("4998"); + expect(parents.get("0")).toBeNull(); + expect(rootTs.get("4999")).toBe(0); + expect(rootTs.get("0")).toBe(0); + expect(elapsed).toBeLessThan(100); + }); +}); + +describe("resolveThreadRootTimestamps", () => { + test("replies take their root's timestamp; roots, orphans and cycle members keep their own", () => { + const items = [ + { id: "p", createdA: 10 }, + { id: "r", inReplyTo: "p", createdA: 50 }, + { id: "rr", inReplyTo: "r", createdA: 60 }, + { id: "orphan", inReplyTo: "gone", createdA: 20 }, + { id: "x", inReplyTo: "y", createdA: 30 }, + { id: "y", inReplyTo: "x", createdA: 40 }, + { id: "c", inReplyTo: "x", createdA: 70 }, + ]; + const ts = resolveThreadRootTimestamps(items); + expect(ts.get("r")).toBe(10); + expect(ts.get("rr")).toBe(10); + expect(ts.get("orphan")).toBe(20); + expect(ts.get("x")).toBe(30); + expect(ts.get("y")).toBe(40); + expect(ts.get("c")).toBe(30); + }); }); describe("validateReplyTarget", () => { diff --git a/packages/core/annotation-threads.ts b/packages/core/annotation-threads.ts index 03735fbf4..82891a03f 100644 --- a/packages/core/annotation-threads.ts +++ b/packages/core/annotation-threads.ts @@ -16,6 +16,11 @@ * * Consequence: no annotation is ever dropped from a threaded rendering, and * a reply always hangs under something that is itself rendered. + * + * Every walk here is linear in the number of annotations: each id is + * classified once and later chains stop at the first classified id, so a + * 5,000-deep chain (which a hostile or buggy tool can POST) costs 5,000 + * steps, not 12.5 million. */ export interface ThreadableAnnotation { @@ -23,9 +28,17 @@ export interface ThreadableAnnotation { inReplyTo?: string | null; } +/** The id `item` points at when that target could be a parent: present, and not itself. */ +function replyTarget(item: T, byId: Map): string | null { + const target = typeof item.inReplyTo === "string" ? item.inReplyTo : null; + if (!target || target === item.id || !byId.has(target)) return null; + return target; +} + /** * The effective parent of every annotation in `items`: the `inReplyTo` * target for a valid reply, `null` for a root (see the module comment). + * O(n): ids are classified once, with path compression along each chain. */ export function resolveReplyParents( items: readonly T[], @@ -34,33 +47,79 @@ export function resolveReplyParents( for (const item of items) byId.set(item.id, item); const parents = new Map(); for (const item of items) { - const target = typeof item.inReplyTo === "string" ? item.inReplyTo : null; - if (!target || target === item.id || !byId.has(target)) { - parents.set(item.id, null); - continue; - } - // Walk up until the chain leaves the list or ends at a root. Coming back - // to the item itself means it is a MEMBER of a cycle: a root. A chain - // that merely leads into a cycle elsewhere leaves the item a reply of - // its parent, which is itself rendered (as a cycle member, a root). - const seen = new Set(); - let current: T | undefined = byId.get(target); - let member = false; + if (parents.has(item.id)) continue; + // Walk the chain until it reaches an already classified id, a root, or + // an id already on this walk (a cycle). Ids on the walk are remembered + // with their position so the cycle segment can be told from the run-in. + const path: T[] = []; + const position = new Map(); + let current: T | undefined = item; + let cycleStart = -1; while (current) { - if (current.id === item.id) { - member = true; + if (parents.has(current.id)) break; + const seen = position.get(current.id); + if (seen !== undefined) { + cycleStart = seen; + break; + } + position.set(current.id, path.length); + path.push(current); + const target = replyTarget(current, byId); + if (!target) { + parents.set(current.id, null); break; } - if (seen.has(current.id)) break; - seen.add(current.id); - const next = typeof current.inReplyTo === "string" ? current.inReplyTo : null; - current = next ? byId.get(next) : undefined; + current = byId.get(target); + } + // Cycle members are roots; everything before the cycle (and every id on + // a walk that ended at a known id or a root) is a reply of its target. + const replyEnd = cycleStart === -1 ? path.length : cycleStart; + for (let i = 0; i < path.length; i++) { + const node = path[i]; + if (parents.has(node.id)) continue; + parents.set(node.id, i < replyEnd ? replyTarget(node, byId) : null); } - parents.set(item.id, member ? null : target); } return parents; } +/** + * The timestamp of every annotation's thread root (its own when it is a + * root), keyed by id, in one linear pass over `parents`. Panels sort threads + * by this so a reply sits at its root's position on the timeline. + */ +export function resolveThreadRootTimestamps( + items: readonly T[], + parents: ReadonlyMap = resolveReplyParents(items), +): Map { + const byId = new Map(); + for (const item of items) byId.set(item.id, item); + const rootTs = new Map(); + for (const item of items) { + if (rootTs.has(item.id)) continue; + const path: string[] = []; + let current: T | undefined = item; + let ts: number | undefined; + while (current) { + const known = rootTs.get(current.id); + if (known !== undefined) { + ts = known; + break; + } + path.push(current.id); + const parent = parents.get(current.id) ?? null; + if (!parent) { + ts = current.createdA; + break; + } + current = byId.get(parent); + } + const resolved = ts ?? item.createdA; + for (const id of path) rootTs.set(id, resolved); + } + return rootTs; +} + /** * Validate an `inReplyTo` value about to be written onto annotation `id` * (external-annotation PATCH ingest, both runtimes). A reply must point at From 5d3564be052c69c24c13c1bd74fc03588acbae16 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 27 Aug 2026 15:06:44 -0700 Subject: [PATCH 08/17] perf(ui): sort the annotations panel timeline from a memoized thread map threadTs walked the reply chain with all.find per hop inside the sort comparator, O(n^2 log n): 2,000 threaded comments took 4.5 s to sort per render and 5,000 over a minute, and POST /api/external-annotations has no depth or count limit. Thread timestamps are now resolved once per render through the shared linear helper and the comparator reads the map; threadReplies emits iteratively so a deep chain costs no stack. A 5,000-chain test asserts the ordering completes well under 100 ms. --- .../AnnotationPanel.threadPerf.test.ts | 39 +++++++++++++++ packages/ui/components/AnnotationPanel.tsx | 47 ++++++++++--------- 2 files changed, 63 insertions(+), 23 deletions(-) create mode 100644 packages/ui/components/AnnotationPanel.threadPerf.test.ts diff --git a/packages/ui/components/AnnotationPanel.threadPerf.test.ts b/packages/ui/components/AnnotationPanel.threadPerf.test.ts new file mode 100644 index 000000000..a31b7a1e6 --- /dev/null +++ b/packages/ui/components/AnnotationPanel.threadPerf.test.ts @@ -0,0 +1,39 @@ +/** + * The panel's thread ordering must stay linear: 2,000 threaded comments took + * 4.5 s and 5,000 over a minute per render when the sort comparator walked + * each reply chain with a linear parent lookup. POST /api/external-annotations + * has no depth or count limit, so a buggy tool could freeze the tab. + * + * Exercises the same helpers the panel renders from (threadReplies plus the + * shared root-timestamp resolution) without a DOM. + */ +import { describe, expect, test } from 'bun:test'; +import { resolveThreadRootTimestamps } from '@plannotator/core/annotation-threads'; +import { AnnotationType, type Annotation } from '../types'; +import { threadReplies } from './AnnotationPanel'; + +function comment(id: string, extra: Partial = {}): Annotation { + return { id, blockId: 'b', startOffset: 0, endOffset: 1, type: AnnotationType.COMMENT, text: `t${id}`, originalText: 'x', createdA: Number(id), author: 'a', ...extra }; +} + +describe('AnnotationPanel threading on a deep chain', () => { + test('5,000 chained replies thread and sort in well under 100 ms, in order, dropping nothing', () => { + const anns: Annotation[] = [comment('0')]; + for (let i = 1; i < 5000; i++) anns.push(comment(String(i), { inReplyTo: String(i - 1) })); + const sorted = [...anns].sort((a, b) => a.createdA - b.createdA); + + const start = performance.now(); + const threaded = threadReplies(sorted); + const rootTs = resolveThreadRootTimestamps(sorted); + const entries = threaded.map(({ annotation, isReply }) => ({ ts: annotation.createdA, threadTs: rootTs.get(annotation.id)!, annotation, isReply })); + entries.sort((a, b) => (a.threadTs !== b.threadTs ? a.threadTs - b.threadTs : a.ts - b.ts)); + const elapsed = performance.now() - start; + + expect(entries.length).toBe(5000); + expect(entries[0].annotation.id).toBe('0'); + expect(entries[0].isReply).toBe(false); + expect(entries[4999].annotation.id).toBe('4999'); + expect(entries[4999].isReply).toBe(true); + expect(elapsed).toBeLessThan(100); + }); +}); diff --git a/packages/ui/components/AnnotationPanel.tsx b/packages/ui/components/AnnotationPanel.tsx index a2981f633..f51275af2 100644 --- a/packages/ui/components/AnnotationPanel.tsx +++ b/packages/ui/components/AnnotationPanel.tsx @@ -7,7 +7,7 @@ import { useIsMobile } from '../hooks/useIsMobile'; import { OverlayScrollArea } from './OverlayScrollArea'; import { Button } from './ui/button'; import { cn } from '../lib/utils'; -import { resolveReplyParents } from '@plannotator/core/annotation-threads'; +import { resolveReplyParents, resolveThreadRootTimestamps } from '@plannotator/core/annotation-threads'; // Card type-word colors. Deletion uses `destructive` (reliably red on every // theme, matching the in-document .deletion highlight). Comment uses the @@ -59,31 +59,29 @@ export function threadReplies(sorted: Annotation[]): Array<{ annotation: Annotat list.push(a); byParent.set(parent, list); } + // Depth-first, iteratively: a 5,000-deep chain must not recurse 5,000 + // frames deep. The stack holds each node's replies in reverse so they pop + // in creation order. const out: Array<{ annotation: Annotation; isReply: boolean }> = []; - const emit = (a: Annotation, isReply: boolean) => { - out.push({ annotation: a, isReply }); - for (const reply of byParent.get(a.id) ?? []) emit(reply, true); + const stack: Array<{ annotation: Annotation; isReply: boolean }> = []; + const pushReplies = (a: Annotation) => { + const replies = byParent.get(a.id); + if (!replies) return; + for (let i = replies.length - 1; i >= 0; i--) stack.push({ annotation: replies[i], isReply: true }); }; for (const a of sorted) { if (parents.get(a.id)) continue; - emit(a, false); + out.push({ annotation: a, isReply: false }); + pushReplies(a); + while (stack.length > 0) { + const next = stack.pop()!; + out.push(next); + pushReplies(next.annotation); + } } return out; } -/** Timeline position of an annotation: its own time, or its thread root's for replies. */ -function threadTs(annotation: Annotation, all: Annotation[]): number { - let current = annotation; - const seen = new Set(); - while (current.inReplyTo && !seen.has(current.id)) { - seen.add(current.id); - const parent = all.find((a) => a.id === current.inReplyTo); - if (!parent) break; - current = parent; - } - return current.createdA; -} - interface DirectEditsPanelItem { id: string; title?: string; @@ -178,13 +176,16 @@ export const AnnotationPanel: React.FC = ({ // sit right after its parent (and the parent's earlier replies) at the // parent's timeline position. With no replies the order is untouched. const threadedAnnotations = threadReplies(sortedAnnotations); + // Thread timestamps are resolved once per render, linearly (shared helper), + // and the comparator only reads the map: resolving each chain inside the + // comparator with a linear parent lookup was O(n^2 log n) and froze the + // tab on a few thousand threaded comments. + const threadRootTs = resolveThreadRootTimestamps(sortedAnnotations); const timelineEntries = [ - ...threadedAnnotations.map(({ annotation, isReply }) => ({ kind: 'plan' as const, ts: annotation.createdA, annotation, isReply })), - ...sortedCodeAnnotations.map(annotation => ({ kind: 'code' as const, ts: annotation.createdAt, annotation, isReply: false })), + ...threadedAnnotations.map(({ annotation, isReply }) => ({ kind: 'plan' as const, ts: annotation.createdA, threadTs: threadRootTs.get(annotation.id) ?? annotation.createdA, annotation, isReply })), + ...sortedCodeAnnotations.map(annotation => ({ kind: 'code' as const, ts: annotation.createdAt, threadTs: annotation.createdAt, annotation, isReply: false })), ].sort((a, b) => { - const ta = a.kind === 'plan' ? threadTs(a.annotation, sortedAnnotations) : a.ts; - const tb = b.kind === 'plan' ? threadTs(b.annotation, sortedAnnotations) : b.ts; - if (ta !== tb) return ta - tb; + if (a.threadTs !== b.threadTs) return a.threadTs - b.threadTs; return a.ts - b.ts; }); const totalCount = annotations.length + codeAnnotations.length + (editorAnnotations?.length ?? 0); From 0bb8e54aa3fe7a2360216a312eaee07e2387b034 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 27 Aug 2026 15:06:44 -0700 Subject: [PATCH 09/17] perf(ui): emit exported reply threads in linear size and time replyBlock re-filtered and re-sorted the whole annotation list at every nesting level and indented without bound, so 10,000 replies produced 100 MiB of markdown on /api/feedback. Children are grouped once, the thread is emitted from an explicit stack into an array of parts, and the nesting indent is capped at eight levels, so a 5,000-reply chain exports in linear size and time (tested). --- packages/ui/utils/parser.inReplyTo.test.ts | 19 ++++++++++ packages/ui/utils/parser.ts | 43 +++++++++++++++++----- 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/packages/ui/utils/parser.inReplyTo.test.ts b/packages/ui/utils/parser.inReplyTo.test.ts index b8c032330..6842642e3 100644 --- a/packages/ui/utils/parser.inReplyTo.test.ts +++ b/packages/ui/utils/parser.inReplyTo.test.ts @@ -68,6 +68,25 @@ describe('exportAnnotations with inReplyTo', () => { expect(out.indexOf('Reply to X')).toBeLessThan(out.indexOf('Y says')); }); + // 10,000 replies once produced 100 MiB of markdown on /api/feedback: the + // nesting re-filtered the whole list per level and the indent grew without + // bound. Size and time must stay linear in the number of replies. + test('a 5,000-reply chain exports in linear size and time', () => { + const anns: Annotation[] = [comment('0', 'Rotate the key', 'root', { createdA: 0 })]; + for (let i = 1; i < 5000; i++) { + anns.push(comment(String(i), 'Rotate the key', `reply ${i}`, { inReplyTo: String(i - 1), createdA: i })); + } + const start = performance.now(); + const out = exportAnnotations(blocks, anns); + const elapsed = performance.now() - start; + expect(out).toContain('have 5000 pieces of feedback'); + expect(out).toContain('reply 4999'); + expect(out).not.toContain('## 2. '); + // Every reply line is bounded (capped indent), so the whole export is too. + expect(out.length).toBeLessThan(5000 * 80); + expect(elapsed).toBeLessThan(500); + }); + test('without any inReplyTo the export is byte-identical to the plain export', () => { const a = comment('a', 'Rotate the key', 'one'); const b = comment('b', 'Ship behind a flag', 'two', { createdA: 2 }); diff --git a/packages/ui/utils/parser.ts b/packages/ui/utils/parser.ts index 31d9f4828..fe2b7af7d 100644 --- a/packages/ui/utils/parser.ts +++ b/packages/ui/utils/parser.ts @@ -1220,28 +1220,51 @@ export const exportAnnotations = ( const replyParents = resolveReplyParents(sortedAnns as any[]); const isReply = (a: any) => replyParents.get(a.id) != null; const hasReplies = sortedAnns.some(isReply); - const repliesOf = (parent: any): any[] => - hasReplies ? sortedAnns.filter((a: any) => replyParents.get(a.id) === parent.id).sort((a: any, b: any) => a.createdA - b.createdA) : []; + // Children are grouped once (creation order within a parent); the old + // per-level re-filter and re-sort of the whole list made a long thread + // quadratic in both time and output size. + const repliesByParent = new Map(); if (hasReplies) { + for (const a of sortedAnns as any[]) { + const parent = replyParents.get(a.id); + if (!parent) continue; + const list = repliesByParent.get(parent) ?? []; + list.push(a); + repliesByParent.set(parent, list); + } + for (const list of repliesByParent.values()) list.sort((a: any, b: any) => a.createdA - b.createdA); emitOrder = emitOrder.filter((a) => !isReply(a)); // Numbers stay consecutive over the entries that are actually emitted. annotationNumbers.clear(); emitOrder.forEach((ann, index) => annotationNumbers.set(ann, index + 1)); } - const replyBlock = (parent: any, depth = 0): string => { - let block = ''; - for (const reply of repliesOf(parent)) { + // Nesting indent is capped so the export stays linear in the thread size + // (an uncapped indent on a 5,000-deep chain is 25 MB of whitespace) and + // the emission is an explicit stack rather than recursion, so a deep chain + // costs neither stack frames nor repeated string copies. + const MAX_REPLY_INDENT_DEPTH = 8; + const replyBlock = (parent: any): string => { + const parts: string[] = []; + const stack: Array<{ reply: any; depth: number }> = []; + const pushReplies = (of: any, depth: number) => { + const replies = repliesByParent.get(of.id); + if (!replies) return; + for (let i = replies.length - 1; i >= 0; i--) stack.push({ reply: replies[i], depth }); + }; + pushReplies(parent, 0); + while (stack.length > 0) { + const { reply, depth } = stack.pop()!; const who = reply.author ? `${reply.author}` : 'reply'; - const indent = ' '.repeat(depth); - block += `${indent}- **Reply (${who}):** ${String(reply.text ?? '').replace(/\r?\n/g, `\n${indent} `)}\n`; + const indent = ' '.repeat(Math.min(depth, MAX_REPLY_INDENT_DEPTH)); + parts.push(`${indent}- **Reply (${who}):** ${String(reply.text ?? '').replace(/\r?\n/g, `\n${indent} `)}\n`); if (reply.images && reply.images.length > 0) { reply.images.forEach((img: ImageAttachment) => { - block += `${indent} - [${img.name}] \`${img.path}\`\n`; + parts.push(`${indent} - [${img.name}] \`${img.path}\`\n`); }); } - block += replyBlock(reply, depth + 1); + pushReplies(reply, depth + 1); } - return block; + return parts.join(''); }; let lastEmittedPage: string | null = null; From 2e9dc9d776d66d72c592053877e855cee8bd0b22 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 27 Aug 2026 15:12:04 -0700 Subject: [PATCH 10/17] fix(annotate): warn once when the HTML root cannot be read The readRootHtml fallback was silent on both runtimes and its unreadable reason was never consumed, so a genuine read bug could hide behind the startup snapshot. Both servers now console.warn once per process with the path and the error message, keeping the fallback itself. The Bun directory-swap test comment is corrected: Bun.file(dir).exists() is false, so that case guards the Pi mirror while chmod 000 is the Bun guard; both tests assert the single warning. --- .../server/serverAnnotate-root-html.test.ts | 9 +++++++++ apps/pi-extension/server/serverAnnotate.ts | 13 +++++++++++- packages/server/annotate.test.ts | 20 +++++++++++++++---- packages/server/annotate.ts | 13 +++++++++++- 4 files changed, 49 insertions(+), 6 deletions(-) diff --git a/apps/pi-extension/server/serverAnnotate-root-html.test.ts b/apps/pi-extension/server/serverAnnotate-root-html.test.ts index 56819b1ef..bd1215848 100644 --- a/apps/pi-extension/server/serverAnnotate-root-html.test.ts +++ b/apps/pi-extension/server/serverAnnotate-root-html.test.ts @@ -223,6 +223,9 @@ describe("pi annotate server: local rendered-HTML root freshness", () => { renderHtml: true, project, }); + const warnings: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { warnings.push(args.map(String).join(" ")); }; try { unlinkSync(pagePath); mkdirSync(pagePath); @@ -237,7 +240,13 @@ describe("pi annotate server: local rendered-HTML root freshness", () => { const share = await withTimeout(fetch(`${server.url}/api/share-html`)); expect(share.status).toBe(200); expect(((await share.json()) as { shareHtml: string }).shareHtml).toContain("V2"); + // The fallback is silent to the reviewer, so the reason is logged once + // per process (path and error), not once per read. + const rootWarnings = warnings.filter((w) => w.includes("could not read the HTML root")); + expect(rootWarnings).toHaveLength(1); + expect(rootWarnings[0]).toContain(pagePath); } finally { + console.warn = originalWarn; server.stop(); } }); diff --git a/apps/pi-extension/server/serverAnnotate.ts b/apps/pi-extension/server/serverAnnotate.ts index 1fa5be897..b2301475c 100644 --- a/apps/pi-extension/server/serverAnnotate.ts +++ b/apps/pi-extension/server/serverAnnotate.ts @@ -442,6 +442,16 @@ export async function startAnnotateServer(options: { return false; } + // The fallback is silent to the reviewer, so the reason is logged once per + // process: a genuine bug in the read must not hide behind the snapshot. + let rootHtmlUnreadableWarned = false; + const warnRootHtmlUnreadable = (path: string, err: unknown) => { + if (rootHtmlUnreadableWarned) return; + rootHtmlUnreadableWarned = true; + const message = err instanceof Error ? err.message : String(err); + console.warn(`[plannotator] could not read the HTML root ${path}; serving the startup snapshot instead: ${message}`); + }; + // A local rendered-HTML root is served from its CURRENT bytes, not the // startup snapshot: the reviewer can Refresh in-app or reload the tab after // an agent edits the file, and both /api/plan and /api/share-html must then @@ -467,7 +477,8 @@ export async function startAnnotateServer(options: { return { kind: "snapshot", reason: "too-large" }; } return { kind: "current", html: readFileSync(rootHtmlSourcePath, "utf-8") }; - } catch { + } catch (err) { + warnRootHtmlUnreadable(rootHtmlSourcePath, err); return { kind: "snapshot", reason: "unreadable" }; } } diff --git a/packages/server/annotate.test.ts b/packages/server/annotate.test.ts index c3d1869a2..59926495a 100644 --- a/packages/server/annotate.test.ts +++ b/packages/server/annotate.test.ts @@ -372,10 +372,12 @@ describe("annotate server: local rendered-HTML root freshness", () => { } }); - // A root that exists but cannot be read (the path replaced by a directory, - // or permissions revoked) used to throw out of the request handler: a tab - // reload answered 500. It is the missing-file fallback: the startup - // snapshot, with its version diff, and the share endpoint agrees. + // A root that exists but cannot be read is the missing-file fallback: the + // startup snapshot, with its version diff, and the share endpoint agrees. + // On Bun, Bun.file(dir).exists() is false, so a path replaced by a + // directory already took the missing path (the case guards the Pi mirror, + // where existsSync is true and the read throws); the chmod 000 case below + // is the one that made the Bun handler throw and answer 500. async function seedTwoVersions(label: string): Promise<{ pagePath: string; project: string }> { const pagePath = join(freshDocDir(label), "page.html"); const project = uniqueProject(label); @@ -436,6 +438,9 @@ describe("annotate server: local rendered-HTML root freshness", () => { renderHtml: true, project, }); + const warnings: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { warnings.push(args.map(String).join(" ")); }; try { writeFileSync(pagePath, page("V3"), "utf-8"); chmodSync(pagePath, 0o000); @@ -446,7 +451,14 @@ describe("annotate server: local rendered-HTML root freshness", () => { expect(fallback.rawHtml).not.toContain("V3"); expect(fallback.previousPlan).toBe(page("V1")); expect(fallback.diffHtml).toBeDefined(); + // The fallback is silent to the reviewer, so the reason is logged once + // per process (path and error), not once per read. + await fetch(`${server.url}/api/plan`); + const rootWarnings = warnings.filter((w) => w.includes("could not read the HTML root")); + expect(rootWarnings).toHaveLength(1); + expect(rootWarnings[0]).toContain(pagePath); } finally { + console.warn = originalWarn; chmodSync(pagePath, 0o644); server.stop(); } diff --git a/packages/server/annotate.ts b/packages/server/annotate.ts index f84c55e3b..3e49ad34d 100644 --- a/packages/server/annotate.ts +++ b/packages/server/annotate.ts @@ -405,6 +405,16 @@ export async function startAnnotateServer( tailnetPublished: options.tailnetPublished === true, }); + // The fallback is silent to the reviewer, so the reason is logged once per + // process: a genuine bug in the read must not hide behind the snapshot. + let rootHtmlUnreadableWarned = false; + const warnRootHtmlUnreadable = (path: string, err: unknown) => { + if (rootHtmlUnreadableWarned) return; + rootHtmlUnreadableWarned = true; + const message = err instanceof Error ? err.message : String(err); + console.warn(`[plannotator] could not read the HTML root ${path}; serving the startup snapshot instead: ${message}`); + }; + // A local rendered-HTML root is served from its CURRENT bytes, not the // startup snapshot: the reviewer can Refresh in-app or reload the tab after // an agent edits the file, and both /api/plan and /api/share-html must then @@ -426,7 +436,8 @@ export async function startAnnotateServer( if (!(await file.exists())) return { kind: "snapshot", reason: "missing" }; if (file.size > MAX_ANNOTATABLE_FILE_BYTES) return { kind: "snapshot", reason: "too-large" }; return { kind: "current", html: await file.text() }; - } catch { + } catch (err) { + warnRootHtmlUnreadable(rootHtmlSourcePath, err); return { kind: "snapshot", reason: "unreadable" }; } } From 0002b4af3504cc504b46c6ccf0507c0450208874 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 27 Aug 2026 15:12:05 -0700 Subject: [PATCH 11/17] fix(webmcp): bound the change tracker's tombstones tombstones, ownHashes and ownSeqs were never evicted, so a create/delete loop of 5,000 left all three at 5,000 for the life of the tab. Tombstones are now capped at MAX_TOMBSTONES (2,000, oldest first) and the ownership and agent-removal records of a forgotten id go with it; live entries keep theirs. Tested past the cap. --- packages/ui/webmcp/changes.test.ts | 24 +++++++++++++++++++++++- packages/ui/webmcp/changes.ts | 24 ++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/packages/ui/webmcp/changes.test.ts b/packages/ui/webmcp/changes.test.ts index 3674eae9a..ea7f4e566 100644 --- a/packages/ui/webmcp/changes.test.ts +++ b/packages/ui/webmcp/changes.test.ts @@ -8,7 +8,7 @@ * per-path set keeps independent watermarks and activity times. */ import { describe, expect, test } from 'bun:test'; -import { AnnotationChangeTracker, BROWSER_AGENT_SOURCE, ChangeTrackerSet, type TrackedAnnotation } from './changes'; +import { AnnotationChangeTracker, BROWSER_AGENT_SOURCE, ChangeTrackerSet, MAX_TOMBSTONES, type TrackedAnnotation } from './changes'; const human = (id: string, text: string): TrackedAnnotation => ({ id, text, originalText: 'q' }); const agent = (id: string, text: string): TrackedAnnotation => ({ id, text, originalText: 'q', source: BROWSER_AGENT_SOURCE }); @@ -96,6 +96,28 @@ describe('AnnotationChangeTracker', () => { expect(tracker.removedSince(0)).toEqual([]); expect(tracker.seqOf('a')).toBe(3); }); + + // A create/delete loop of 5,000 left tombstones, ownHashes and ownSeqs at + // 5,000 each for the life of the tab: nothing was ever evicted. + test('tombstones and the ownership records of forgotten ids are evicted oldest first past MAX_TOMBSTONES', () => { + const tracker = new AnnotationChangeTracker(); + const total = MAX_TOMBSTONES + 500; + for (let i = 0; i < total; i++) { + const id = `a-${i}`; + tracker.claimOwn({ id, text: 'x' }); + tracker.observe([{ id, text: 'x' }]); + tracker.observe([]); + } + const removed = tracker.removedSince(0); + expect(removed.length).toBe(MAX_TOMBSTONES); + // The oldest 500 are gone, the newest MAX_TOMBSTONES remain, in order. + expect(removed[0].id).toBe('a-500'); + expect(removed[removed.length - 1].id).toBe(`a-${total - 1}`); + expect(tracker.knows('a-0')).toBe(false); + expect(tracker.isOwn('a-0')).toBe(false); + expect(tracker.knows('a-500')).toBe(true); + expect(tracker.isOwn(`a-${total - 1}`)).toBe(true); + }); }); describe('ChangeTrackerSet', () => { diff --git a/packages/ui/webmcp/changes.ts b/packages/ui/webmcp/changes.ts index b915ff93e..97ee73738 100644 --- a/packages/ui/webmcp/changes.ts +++ b/packages/ui/webmcp/changes.ts @@ -57,6 +57,15 @@ export function hashAnnotation(annotation: TrackedAnnotation): string { const CURSOR_PREFIX = 'w:'; +/** + * Tombstones kept per tracker. A removal past this many is forgotten oldest + * first (FIFO by seq), together with the agent-ownership records of the + * forgotten id, so a create/delete loop cannot grow the tracker for the + * life of the tab. A tombstone older than 2,000 later removals is one no + * `since` in practice still asks about. + */ +export const MAX_TOMBSTONES = 2000; + export class AnnotationChangeTracker { private readonly entries = new Map(); private readonly tombstones = new Map(); @@ -151,12 +160,27 @@ export class AnnotationChangeTracker { this.tombstones.set(id, tombstone); delta.removed.push(tombstone); } + this.pruneTombstones(); // A re-added id is a fresh record: forget any agent-removal claim on it. for (const id of seen) this.agentRemoved.delete(id); if (touched) this.lastActivity = this.now(); return delta; } + /** Bounded eviction (MAX_TOMBSTONES): the oldest tombstones go, and with + * them the ownership and agent-removal records that only mattered while + * the id could still be asked about. Live entries keep theirs. */ + private pruneTombstones(): void { + while (this.tombstones.size > MAX_TOMBSTONES) { + const oldest = this.tombstones.keys().next().value; + if (oldest === undefined) break; + this.tombstones.delete(oldest); + this.ownHashes.delete(oldest); + this.ownSeqs.delete(oldest); + this.agentRemoved.delete(oldest); + } + } + seqOf(id: string): number | undefined { return this.entries.get(id)?.seq; } From 46b5baf2a8665d7a2566af0f52d3b4c4e0134e8a Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 27 Aug 2026 15:12:05 -0700 Subject: [PATCH 12/17] fix(webmcp): bound the add_comments requestId memory The idempotency map only ever grew. DocumentToolState.rememberRequest keeps the newest MAX_REMEMBERED_REQUESTS (500) requestIds, oldest evicted first; a retry of a recent call is still deduplicated, and a replay of an evicted id creates anew (tested). --- packages/editor/webmcp/documentTools.test.ts | 21 +++++++++++++++++++ packages/editor/webmcp/documentTools.ts | 22 ++++++++++++++++++-- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/packages/editor/webmcp/documentTools.test.ts b/packages/editor/webmcp/documentTools.test.ts index 7351b5d05..6e40622c5 100644 --- a/packages/editor/webmcp/documentTools.test.ts +++ b/packages/editor/webmcp/documentTools.test.ts @@ -29,6 +29,7 @@ import { type ToolSpec, } from '@plannotator/ui/webmcp'; import { + MAX_REMEMBERED_REQUESTS, buildDocumentHooks, buildDocumentTools, createDocumentToolState, @@ -436,6 +437,26 @@ describe('ownership', () => { }); }); +describe('requestId memory is bounded', () => { + // The idempotency map only ever grew: every add_comments call with a + // requestId added an entry for the life of the tab. + test('past MAX_REMEMBERED_REQUESTS the oldest requestId is forgotten and a replay of it creates anew', async () => { + const fx = fake(); + const first = dataOf(await fx.call('add_comments', { comments: [{ text: 'first', requestId: 'r-0' }] })); + const firstId = first.results[0].annotation.id; + for (let i = 1; i <= MAX_REMEMBERED_REQUESTS; i++) { + await fx.call('add_comments', { comments: [{ text: `note ${i}`, requestId: `r-${i}` }] }); + } + // The newest replay is still deduplicated... + const recent = dataOf(await fx.call('add_comments', { comments: [{ text: 'again', requestId: `r-${MAX_REMEMBERED_REQUESTS}` }] })); + expect(recent.created).toBe(0); + // ...while the evicted first one is treated as a new request. + const replay = dataOf(await fx.call('add_comments', { comments: [{ text: 'first again', requestId: 'r-0' }] })); + expect(replay.created).toBe(1); + expect(replay.results[0].annotation.id).not.toBe(firstId); + }); +}); + describe('requestId replay after the human removed the comment', () => { test('answers conflict instead of re-creating the comment', async () => { const fx = fake(); diff --git a/packages/editor/webmcp/documentTools.ts b/packages/editor/webmcp/documentTools.ts index f8fd80d3c..ff8d88779 100644 --- a/packages/editor/webmcp/documentTools.ts +++ b/packages/editor/webmcp/documentTools.ts @@ -154,6 +154,8 @@ export const NUDGE_USER_MAX_CHARS = 280; export const MAX_COMMENTS_PER_CALL = 20; export const MAX_REMOVALS_PER_CALL = 50; export const MAX_OTHER_DOCUMENTS = 10; +/** requestIds remembered for add_comments idempotency (oldest evicted first). */ +export const MAX_REMEMBERED_REQUESTS = 500; // --------------------------------------------------------------------------- // Persistent per-session state (survives catalog rebuilds) @@ -164,8 +166,24 @@ export class DocumentToolState { readonly siblings: ChangeTrackerSet; /** Per-sibling read watermark. */ readonly siblingRead = new Map(); - /** requestId -> what it created (idempotency). */ + /** requestId -> what it created (idempotency). Bounded: see rememberRequest. */ readonly requests = new Map(); + + /** + * Record a requestId, forgetting the oldest past MAX_REMEMBERED_REQUESTS + * (FIFO). Idempotency only has to hold across a retry of a recent call, so + * a bounded window is the whole contract; an unbounded map grew with every + * add_comments call for the life of the tab. + */ + rememberRequest(rid: string, created: { id: string; path: string | null; anchoredBy: AnchoredBy }): void { + this.requests.delete(rid); + this.requests.set(rid, created); + while (this.requests.size > MAX_REMEMBERED_REQUESTS) { + const oldest = this.requests.keys().next().value; + if (oldest === undefined) break; + this.requests.delete(oldest); + } + } lastPageUrl: string | null = null; lastOpenPath: string | null = null; responses = 0; @@ -636,7 +654,7 @@ export function buildDocumentTools(adapter: DocumentToolAdapter, state: Document // list) so the returned view carries the new seq and the response's // nudges reflect the mutation. syncTrackers(adapter, state); - if (rid) state.requests.set(rid, { id: annotation.id, path, anchoredBy }); + if (rid) state.rememberRequest(rid, { id: annotation.id, path, anchoredBy }); created += 1; const all = [...snapshot.annotations, annotation]; const result: AddCommentResult = { From e0c3a171fee25d835cbad4dbcf0e18c6ab534def Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 27 Aug 2026 15:12:05 -0700 Subject: [PATCH 13/17] fix(ui): scope minted HTML annotation ids per viewer instance mintedHtmlAnnIds was module-scoped and never cleared, so ids minted by one HtmlViewer leaked into unrelated later instances in the same tab (the Workspaces SPA). The set now lives in a ref per hook instance and is cleared on unmount; the unanchored union only ever consults it against the bridge of the instance that minted the ids. A DOM test mounts A, mints, unmounts, mounts B and asserts B starts empty. --- .../useHtmlAnnotation.mintedIds.test.tsx | 86 +++++++++++++++++++ .../html-viewer/useHtmlAnnotation.ts | 31 ++++--- 2 files changed, 106 insertions(+), 11 deletions(-) create mode 100644 packages/ui/components/html-viewer/useHtmlAnnotation.mintedIds.test.tsx diff --git a/packages/ui/components/html-viewer/useHtmlAnnotation.mintedIds.test.tsx b/packages/ui/components/html-viewer/useHtmlAnnotation.mintedIds.test.tsx new file mode 100644 index 000000000..0094b59b6 --- /dev/null +++ b/packages/ui/components/html-viewer/useHtmlAnnotation.mintedIds.test.tsx @@ -0,0 +1,86 @@ +/** + * Locally minted annotation ids are per viewer instance (DOM-gated). + * + * A module-wide set leaked every id ever minted into unrelated later + * HtmlViewer instances of a long-lived host page (the Workspaces SPA), so a + * fresh instance started with a populated `createdAnnotationIds`. Mount A, + * mint, unmount, mount B: B must start empty. + */ +import React from 'react'; +import { afterEach, describe, expect, test } from 'bun:test'; +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import type { Annotation } from '../../types'; + +const hasDom = typeof document !== 'undefined'; +const hookModule = hasDom ? await import('./useHtmlAnnotation') : null; + +let root: Root | null = null; +let host: HTMLDivElement | null = null; + +afterEach(() => { + act(() => root?.unmount()); + root = null; + host?.remove(); + host = null; +}); + +type HookResult = ReturnType['useHtmlAnnotation']>; + +function Harness({ onResult, onAdd }: { onResult: (r: HookResult, iframe: HTMLIFrameElement) => void; onAdd: (a: Annotation) => void }) { + const iframeRef = React.useRef(null); + const result = hookModule!.useHtmlAnnotation({ + iframeRef, + enabled: true, + annotations: [], + onAddAnnotation: onAdd, + onSelectAnnotation: () => {}, + selectedAnnotationId: null, + mode: 'comment', + }); + React.useEffect(() => { + if (iframeRef.current) onResult(result, iframeRef.current); + }); + return