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/AGENTS.md b/AGENTS.md index 91b4c913d..16c342f4a 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) | @@ -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. 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/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 b5983c045..bd1215848 100644 --- a/apps/pi-extension/server/serverAnnotate-root-html.test.ts +++ b/apps/pi-extension/server/serverAnnotate-root-html.test.ts @@ -5,16 +5,16 @@ * 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. */ 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 { 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(" { 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, + }); + const warnings: string[] = []; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { warnings.push(args.map(String).join(" ")); }; + 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"); + // 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(); + } + }); + + // 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..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 @@ -453,16 +463,45 @@ 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 (err) { + warnRootHtmlUnreadable(rootHtmlSourcePath, err); + return { kind: "snapshot", reason: "unreadable" }; } - return { kind: "current", html: readFileSync(rootHtmlSourcePath, "utf-8") }; } + // The in-app Refresh re-reads the root through /api/doc. For the ROOT + // document only (linked docs are unchanged), the response also carries the + // version-diff fields /api/plan serves, recomputed against the bytes just + // read, so a refresh keeps the "Show changes" toggle exactly like a reload. + const rootHistory = annotateHistory; + const rootHtmlVersionDiff = + rootHtmlSourcePath && rootHistory + ? { + path: rootHtmlSourcePath, + compute: (currentHtml: string) => ({ + 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); @@ -657,14 +696,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) @@ -673,7 +714,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(); @@ -694,11 +735,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, @@ -886,6 +927,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/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..1d85b560f --- /dev/null +++ b/packages/core/annotation-threads.test.ts @@ -0,0 +1,112 @@ +/** + * 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, resolveThreadRootTimestamps, 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"); + }); + + 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", () => { + 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..82891a03f --- /dev/null +++ b/packages/core/annotation-threads.ts @@ -0,0 +1,161 @@ +/** + * 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. + * + * 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 { + id: string; + 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[], +): Map { + const byId = new Map(); + for (const item of items) byId.set(item.id, item); + const parents = new Map(); + for (const item of items) { + 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 (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; + } + 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); + } + } + 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 + * 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/editor/App.htmlChrome.test.tsx b/packages/editor/App.htmlChrome.test.tsx index 3d09f5f12..108777c28 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; @@ -165,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"); @@ -313,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 @@ -383,6 +443,115 @@ 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("an Unanchored chip from one refresh clears when the next refresh's restore report is empty", async () => { + // The bridge answers the parent's post-restore report request with the + // complete set, empty included (pinned in htmlPinpointProtocol.test); + // the App replaces its chip set with each report, so a chip from + // refresh 1 must not survive a refresh 2 on which everything anchors. + setStorageBackend(memoryBackend); + seedAnnouncementsSeen(); + const draftedFetch: typeof fetch = async (input) => { + const rawUrl = input instanceof Request ? input.url : String(input); + const url = new URL(rawUrl, "http://localhost"); + if (url.pathname === "/api/draft" && (!(input instanceof Request) || input.method === "GET")) { + return Response.json({ + annotations: [{ + id: "X", + blockId: "", + startOffset: 0, + endOffset: 0, + type: "COMMENT", + text: "pinned note", + originalText: "Body copy.", + createdA: 1, + htmlAnchor: { selector: "p", tagName: "p", text: "Body copy." }, + }], + codeAnnotations: [], + globalAttachments: [], + }); + } + return versionedFetch(input); + }; + await mountHtmlAnnotate(draftedFetch); + await settle(); + // The draft arrives behind the "Draft Recovered" dialog; restore it. + for (let attempt = 0; attempt < 20 && !findButtonByText("Restore"); attempt += 1) { + await settle(); + } + const restore = findButtonByText("Restore"); + if (!restore) throw new Error("draft restore dialog missing"); + await act(async () => restore.click()); + await settle(); + // The chip lives on the panel card; the HTML surface opens with the + // panel collapsed, so open it. + const showPanel = document.querySelector('button[title="Show annotations"]'); + if (showPanel) await act(async () => showPanel.click()); + await settle(); + expect(document.querySelector('[data-annotation-id="X"]')).not.toBeNull(); + const chip = () => document.querySelector('[data-annotation-unanchored]'); + const frame = () => document.querySelector("iframe[srcdoc]"); + const report = async (ids: string[]) => { + const iframe = frame(); + if (!iframe?.contentWindow) throw new Error("HTML iframe missing"); + await act(async () => { + window.dispatchEvent(new MessageEvent("message", { + source: iframe.contentWindow, + data: { type: "plannotator-bridge-unanchored", ids }, + })); + }); + }; + const refresh = async () => { + const button = refreshButton(); + if (!button) throw new Error("refresh button missing"); + await act(async () => button.click()); + for (let attempt = 0; attempt < 20 && refreshButton()?.getAttribute("aria-disabled") !== "false"; attempt += 1) { + await settle(); + } + }; + expect(chip()).toBeNull(); + + // Refresh 1: the remounted viewer's bridge reports X unanchored. + await refresh(); + await report(["X"]); + expect(chip()).not.toBeNull(); + + // Refresh 2: the page anchors everything; the report is the empty set. + await refresh(); + await report([]); + expect(chip()).toBeNull(); + }); + 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..49fc6d041 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, @@ -4840,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/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 { }); }); +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 = { diff --git a/packages/editor/webmcp/useDocumentWebMcp.ts b/packages/editor/webmcp/useDocumentWebMcp.ts index 48869c1ff..c7a4103d5 100644 --- a/packages/editor/webmcp/useDocumentWebMcp.ts +++ b/packages/editor/webmcp/useDocumentWebMcp.ts @@ -375,6 +375,15 @@ export function useDocumentWebMcp(inputs: DocumentWebMcpInputs): { available: bo useEffect(() => { if (openError) settleWaiter(null, false); }, [openError]); + // Unmount: no commit can ever settle a waiter again, so fail them now and + // clear their timers instead of leaving timeouts firing into a dead hook. + useEffect(() => () => { + for (const waiter of navigationWaitersRef.current) { + clearTimeout(waiter.timer); + waiter.resolve(false); + } + navigationWaitersRef.current = []; + }, []); return result; } diff --git a/packages/server/annotate.test.ts b/packages/server/annotate.test.ts index 9b3e86b57..59926495a 100644 --- a/packages/server/annotate.test.ts +++ b/packages/server/annotate.test.ts @@ -15,10 +15,10 @@ */ 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"; +import { dirname, join, resolve } from "path"; import { liveAppDraftIdentity, runGuardedShutdown, startAnnotateServer } from "./annotate"; import { getServerConfig, loadConfig } from "./config"; import { deriveAnnotateHistorySlug } from "@plannotator/shared/annotate-history"; @@ -278,9 +278,12 @@ describe("annotate server: local rendered-HTML root freshness", () => { }); // 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(" { server.stop(); } }); + + // 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); + 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, + }); + 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); + 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(); + // 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(); + } + }); }); describe("annotate server: source save", () => { diff --git a/packages/server/annotate.ts b/packages/server/annotate.ts index 4eec35961..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 @@ -414,15 +424,43 @@ 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 (err) { + warnRootHtmlUnreadable(rootHtmlSourcePath, err); + return { kind: "snapshot", reason: "unreadable" }; + } } + // The in-app Refresh re-reads the root through /api/doc. For the ROOT + // document only (linked docs are unchanged), the response also carries the + // version-diff fields /api/plan serves, recomputed against the bytes just + // read, so a refresh keeps the "Show changes" toggle exactly like a reload. + const rootHistory = annotateHistory; + const rootHtmlVersionDiff = + rootHtmlSourcePath && rootHistory + ? { + path: rootHtmlSourcePath, + compute: (currentHtml: string) => ({ + 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 }); @@ -638,21 +676,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(); @@ -673,11 +713,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, @@ -858,6 +898,7 @@ export async function startAnnotateServer( mode === "annotate-folder" && annotateHistoryEnabled ? { compute: computeFolderAnnotateHistory } : undefined, + rootHtmlVersionDiff, }); } 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/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/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.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 b4e94cd79..f51275af2 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, 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 @@ -40,50 +41,47 @@ 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); } + // 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 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); + 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 (a.inReplyTo && ids.has(a.inReplyTo) && a.inReplyTo !== a.id) continue; - emit(a, false); + if (parents.get(a.id)) continue; + out.push({ annotation: a, isReply: false }); + pushReplies(a); + while (stack.length > 0) { + const next = stack.pop()!; + out.push(next); + pushReplies(next.annotation); + } } - for (const a of sorted) emit(a, false); 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); diff --git a/packages/ui/components/DiagramBlock.lazyRetry.test.tsx b/packages/ui/components/DiagramBlock.lazyRetry.test.tsx index a920b8d8c..7900f9070 100644 --- a/packages/ui/components/DiagramBlock.lazyRetry.test.tsx +++ b/packages/ui/components/DiagramBlock.lazyRetry.test.tsx @@ -145,6 +145,43 @@ describe.each(cases)('$name lazy runtime', ({ install, runtime, element, source, expect(el.textContent).not.toContain(errorTitle); }); + test.skipIf(!hasDom)('one Retry re-attempts every sibling block that failed on the shared runtime', async () => { + // The runtime is memoized per module, so all blocks fail together; a + // Retry that bumped only its own block left the siblings on stale error + // panels after the runtime had recovered. + let healthy = false; + let calls = 0; + install(() => { + calls += 1; + return healthy ? Promise.resolve(runtime) : Promise.reject(new Error('down')); + }); + const el = await mount( + <> +
{element}
+
{element}
+ , + ); + await settle(RETRY_DELAY_MS * 5); + const a = el.querySelector('#a')!; + const b = el.querySelector('#b')!; + expect(a.textContent).toContain(errorTitle); + expect(b.textContent).toContain(errorTitle); + + healthy = true; + const callsBeforeRetry = calls; + const retry = a.querySelector('button[title="Retry loading the diagram renderer"]'); + await act(async () => { + retry!.click(); + }); + await settle(RETRY_DELAY_MS * 5); + + expect(a.innerHTML).toContain('data-sentinel="diagram"'); + expect(b.innerHTML).toContain('data-sentinel="diagram"'); + expect(b.textContent).not.toContain(errorTitle); + // Both blocks re-attempt in the same commit and share one memoized load. + expect(calls).toBe(callsBeforeRetry + 1); + }); + test.skipIf(!hasDom)('a diagram syntax error keeps the existing panel without a Retry button', async () => { const broken = { initialize() {}, render: async () => { throw new Error('Parse error'); }, renderString: async () => { throw new Error('Parse error'); } } as never; let calls = 0; diff --git a/packages/ui/components/GraphvizBlock.tsx b/packages/ui/components/GraphvizBlock.tsx index 073365c7f..7d0044f42 100644 --- a/packages/ui/components/GraphvizBlock.tsx +++ b/packages/ui/components/GraphvizBlock.tsx @@ -2,6 +2,7 @@ import React, { useRef, useState, useEffect, useCallback } from 'react'; import { createPortal } from 'react-dom'; import type { Viz } from '@viz-js/viz'; import type { Block } from '../types'; +import { createRuntimeRetryEpoch } from '../utils/runtimeRetry'; interface ViewBox { x: number; @@ -60,6 +61,9 @@ export function __setVizLoaderForTests( const wait = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); +/** One Retry re-attempts every block whose engine import failed (see utils/runtimeRetry). */ +const vizRetryEpoch = createRuntimeRetryEpoch(); + function parseViewBox(svgEl: SVGSVGElement): ViewBox | null { const raw = svgEl.getAttribute('viewBox'); if (!raw) return null; @@ -153,6 +157,16 @@ export const GraphvizBlock: React.FC<{ block: Block }> = ({ block }) => { const [retryToken, setRetryToken] = useState(0); const [showSource, setShowSource] = useState(false); const [isExpanded, setIsExpanded] = useState(false); + // A sibling's Retry re-attempts this block too, but only while its own + // failure was the shared engine import; a healthy block or a dot syntax + // error is left alone. + const runtimeUnavailableRef = useRef(runtimeUnavailable); + runtimeUnavailableRef.current = runtimeUnavailable; + useEffect(() => vizRetryEpoch.subscribe(() => { + if (!runtimeUnavailableRef.current) return; + setError(null); + setRetryToken((token) => token + 1); + }), []); const zoomLevelRef = useRef(1); const isDraggingRef = useRef(false); @@ -431,10 +445,7 @@ export const GraphvizBlock: React.FC<{ block: Block }> = ({ block }) => { {runtimeUnavailable && ( )} + {onToggleTools && ( + )} )} diff --git a/packages/ui/components/MermaidBlock.tsx b/packages/ui/components/MermaidBlock.tsx index 2cd30bb04..835c43dd9 100644 --- a/packages/ui/components/MermaidBlock.tsx +++ b/packages/ui/components/MermaidBlock.tsx @@ -11,6 +11,10 @@ import { } from '../utils/mermaid'; import { loadMathRenderer } from '../utils/math'; import { hasMermaidMath } from '../utils/mermaid-math-slot'; +import { createRuntimeRetryEpoch } from '../utils/runtimeRetry'; + +/** One Retry re-attempts every block whose runtime import failed (see utils/runtimeRetry). */ +const mermaidRetryEpoch = createRuntimeRetryEpoch(); // Re-exported: the config pin test and the lazy-retry test import them from here. export { MERMAID_CONFIG, __setMermaidRuntimeLoaderForTests }; @@ -135,6 +139,16 @@ const MermaidBlockImpl: React.FC<{ block: Block }> = ({ block }) => { const [retryToken, setRetryToken] = useState(0); const [showSource, setShowSource] = useState(false); const [isExpanded, setIsExpanded] = useState(false); + // A sibling's Retry re-attempts this block too, but only while its own + // failure was the shared runtime import; a healthy block or a diagram + // syntax error is left alone. + const runtimeUnavailableRef = useRef(runtimeUnavailable); + runtimeUnavailableRef.current = runtimeUnavailable; + useEffect(() => mermaidRetryEpoch.subscribe(() => { + if (!runtimeUnavailableRef.current) return; + setError(null); + setRetryToken((token) => token + 1); + }), []); // All zoom/pan state as refs to avoid re-renders const zoomLevelRef = useRef(1); @@ -459,10 +473,7 @@ const MermaidBlockImpl: React.FC<{ block: Block }> = ({ block }) => { {runtimeUnavailable && (