fix: 0.27.9 pre-release QA findings (HTML root fallback, refresh version diff, compact refresh, threading cycles) - #1405
Merged
Merged
Conversation
…s unreadable readRootHtml guarded only existence and the 2 MB cap, so a root replaced by a directory or stripped of read permission made the read throw: the Pi server left /api/plan unanswered (the tab hung on reload) and the Bun server answered 500. Both runtimes now treat an unreadable root exactly like a missing one and serve the startup snapshot with its version-diff fields. Tests on both sides cover the directory swap and, where the runner is not root, a chmod 000 file.
Once the served root bytes differed from the startup snapshot, both servers omitted the version-diff fields and applyRefreshedHtml nulled them client-side, so the "Show changes vs previous version" toggle vanished for the rest of the session the moment a reviewer refreshed: exactly when an agent had just edited the file. previousPlan/versionInfo describe the saved baseline, which stays the correct previous version, so both runtimes now keep them and recompute diffCurrent/diffHtml against the served bytes (htmlDiff is pure; a GET never writes history). /api/doc carries the same recomputed fields when it serves the root document (rootHtmlVersionDiff, root only; linked docs unchanged), the published useHtmlRefresh passes the whole snapshot to onSnapshot, and applyRefreshedHtml sets the fields instead of nulling them while still resetting isPlanDiffActive. Server tests on both runtimes assert the recomputed diff (and its absence for a sibling document); a DOM test asserts the toggle survives a refresh. CLAUDE.md documents the /api/plan contract.
HtmlSurfaceControls renders nothing on compact and the Options menu had equivalents for the pen and the eye but none for Refresh, while the header comment claimed the compact shell offered the same actions. The menu now has a "Refresh from disk" entry gated on the same canRefresh as the header button (local HTML files only; never URL or live-app sessions) and disabled while a refresh is in flight, and the comment names all three actions. A DOM test at a coarse-pointer compact viewport asserts the action and that the refreshed bytes reach the viewer. HtmlSurfaceActions.tsx was dead (only its own test consumed it; the header renders the published HtmlSurfaceControls directly and the parity sign-off happened in the browser), so it and its test are removed and PLANNOTATOR_HTML_REFRESH_LABELS now lives in AppHeader, the only importer. The CI DOM list drops the deleted test file.
The Refresh button only rendered inside the onToggleTools branch, so a host that passed canRefresh and onRefresh without a tools toggle got no Refresh, contradicting the documented props. The refresh and the eye still share one group left of the pen, but each now renders on its own terms; the test covers refresh without the eye, refresh alone, and the all-absent case.
…es at ingest exportAnnotations dropped every annotation that formed an inReplyTo cycle (reachable through PATCH /api/external-annotations, which merges arbitrary fields) while the header count still included it. The threading rule now lives once in @plannotator/core/annotation-threads (resolveReplyParents): an annotation is a reply only when its target is a different annotation in the list and the parent chain never returns to it; roots, orphans, self-references and every cycle member are emitted as roots in original order, so nothing is dropped and the count equals what is emitted. The export and AnnotationPanel.threadReplies both apply it. Both runtimes' PATCH handlers additionally validate inReplyTo (validateReplyTarget): it must name an existing, different annotation and must not close a cycle, otherwise 400, so the invalid state cannot be created there. Tests cover the helper, the export, the panel, and ingest on Bun and Pi; vendor.sh vendors the new core module.
…n pointer CLAUDE.md/AGENTS.md gains an HTML Refresh paragraph under the Annotation System (what Refresh does, which anchors survive, the Unanchored chip, the recomputed version diff, the compact-shell menu action, and that URL and live-app sessions have no Refresh), notes the shared inReplyTo threading rule and the PATCH validation, and stops pointing the WebMCP section at an untracked design file. The marketing annotate page documents Refresh in its HTML section, and the WebMCP reference lists the comment_only_surface nudge that packages/ui/webmcp/nudges.ts already emits.
resolveReplyParents walked every chain to its root without memoization, so a 5,000-deep inReplyTo chain cost 12.5 million steps (431 ms; 20,000 took 9.8 s), and the panel re-ran it on every render. Each id is now classified once with path compression, cycle members are still roots and the run-in into a cycle stays replies, and the new resolveThreadRootTimestamps gives the panel its thread ordering from one linear pass. A 5,000-chain timing test guards both.
threadTs walked the reply chain with all.find per hop inside the sort comparator, O(n^2 log n): 2,000 threaded comments took 4.5 s to sort per render and 5,000 over a minute, and POST /api/external-annotations has no depth or count limit. Thread timestamps are now resolved once per render through the shared linear helper and the comparator reads the map; threadReplies emits iteratively so a deep chain costs no stack. A 5,000-chain test asserts the ordering completes well under 100 ms.
replyBlock re-filtered and re-sorted the whole annotation list at every nesting level and indented without bound, so 10,000 replies produced 100 MiB of markdown on /api/feedback. Children are grouped once, the thread is emitted from an explicit stack into an array of parts, and the nesting indent is capped at eight levels, so a 5,000-reply chain exports in linear size and time (tested).
The readRootHtml fallback was silent on both runtimes and its unreadable reason was never consumed, so a genuine read bug could hide behind the startup snapshot. Both servers now console.warn once per process with the path and the error message, keeping the fallback itself. The Bun directory-swap test comment is corrected: Bun.file(dir).exists() is false, so that case guards the Pi mirror while chmod 000 is the Bun guard; both tests assert the single warning.
tombstones, ownHashes and ownSeqs were never evicted, so a create/delete loop of 5,000 left all three at 5,000 for the life of the tab. Tombstones are now capped at MAX_TOMBSTONES (2,000, oldest first) and the ownership and agent-removal records of a forgotten id go with it; live entries keep theirs. Tested past the cap.
The idempotency map only ever grew. DocumentToolState.rememberRequest keeps the newest MAX_REMEMBERED_REQUESTS (500) requestIds, oldest evicted first; a retry of a recent call is still deduplicated, and a replay of an evicted id creates anew (tested).
mintedHtmlAnnIds was module-scoped and never cleared, so ids minted by one HtmlViewer leaked into unrelated later instances in the same tab (the Workspaces SPA). The set now lives in a ref per hook instance and is cleared on unmount; the unanchored union only ever consults it against the bridge of the instance that minted the ids. A DOM test mounts A, mints, unmounts, mounts B and asserts B starts empty.
…eport is empty Verifies the sweep claim that a chip from refresh 1 survives refresh 2: it does not. The viewer asks the bridge for a complete post-restore report and the bridge answers empty set included (pinned in htmlPinpointProtocol), and the App replaces its chip set with each report. The DOM test restores a drafted anchor, refreshes with a report naming it (chip), refreshes again with an empty report, and asserts the chip is gone. No fix was needed.
A 10,000-annotation burst yielded an 89 KB annotations_new nudge. The annotations_new, replies_new and annotations_removed nudges now list at most MAX_NUDGE_IDS (100) ids and the message says how many more were left out, the way other_document_active is capped.
reveal { path } waiters and their commit timeouts had no unmount cleanup,
so a timeout could fire into a dead hook. Unmount now fails every pending
waiter and clears its timer.
Mermaid and Graphviz memoize one runtime per module, so a chunk import failure fails every block together, but each Retry bumped only the clicked block's own token and its siblings stayed on their error panels after the runtime recovered. A module-level retry epoch (utils/runtimeRetry) notifies every subscribed block whose failure was the runtime import; healthy blocks and syntax errors are left alone. Tested with two instances per runtime.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the confirmed findings from the 0.27.9 pre-release QA. One commit per item; both server runtimes (Bun
packages/serverand the hand-maintained Pi mirror inapps/pi-extension/server) are updated together.1. HTML root unreadable
readRootHtmlguarded only existence and the 2 MB cap, so a root replaced by a directory or stripped of read permission made the read throw: on Pi the rejection was unhandled and/api/plannever answered (the tab hung on reload), on Bun/api/planreturned 500. Both runtimes now catch the read error and fall back to the startup snapshot exactly like the missing-file path, with the same version-diff fields. Tests on both sides cover a root replaced by a directory and, where the runner is not root, a chmod 000 file.2. Version diff lost after Refresh and reload
Once the served root bytes differed from the startup snapshot, both servers omitted the version-diff fields and
applyRefreshedHtmlnulled them client-side, so the "Show changes vs previous version" toggle vanished for the rest of the session the moment a reviewer refreshed. Per the maintainer ruling the diff is recomputed rather than omitted:previousPlanandversionInfokeep describing the saved baseline, anddiffCurrentanddiffHtmlare recomputed against the served bytes (htmlDiffis pure; a GET never writes history). For the in-app Refresh,/api/doccarries the same recomputed fields when it serves the root document only (rootHtmlVersionDiffoption on both doc handlers; linked docs unchanged), the publisheduseHtmlRefreshpasses the whole snapshot toonSnapshotas a second argument (existing callers are unaffected), andapplyRefreshedHtmlsets the fields instead of nulling them while still resettingisPlanDiffActive. The #1232 server tests on both runtimes now assert the recomputed diff and its absence for a sibling document; a DOM test asserts the toggle survives a refresh and renders the recomputed diff. CLAUDE.md's annotate/api/planrow documents the contract.3. Compact touch shell has no Refresh
The Options menu had equivalents for the pen and the eye but none for Refresh, while the header comment claimed parity. The menu now offers "Refresh from disk", gated on the same
canRefreshas the header button and disabled while a refresh is in flight, and the comment names all three actions. A DOM test at a coarse-pointer compact viewport asserts the action is present and that the refreshed bytes reach the viewer.HtmlSurfaceActions.tsxwas dead (only its own test consumed it), so it and its test are deleted,PLANNOTATOR_HTML_REFRESH_LABELSmoved intoAppHeader.tsx, and the CI DOM list drops the removed test file.4. HtmlSurfaceControls contract
The Refresh button rendered only inside the
onToggleToolsbranch, so a host passingcanRefreshandonRefreshwithout the eye got no Refresh. The refresh and the eye still share one group left of the pen, but each renders on its own terms. Tests cover refresh without the eye, refresh alone, and the all-absent case.5. inReplyTo export cycles
exportAnnotationsdropped annotations forming aninReplyTocycle while the header count still included them. The threading rule now lives once in@plannotator/core/annotation-threads(resolveReplyParents): an annotation is a reply only when its target is a different annotation in the list and the parent chain never returns to it; roots, orphans, self-references and every cycle member are emitted as roots in original order, so nothing is dropped and the count equals what is emitted. The export andAnnotationPanel.threadRepliesboth apply it. Both runtimes'PATCH /api/external-annotationsadditionally validateinReplyTo(validateReplyTarget: must name an existing, different annotation and must not close a cycle, else 400), so the invalid state cannot be created there. Tests cover the helper, the export, the panel, and ingest on Bun and Pi;vendor.shvendors the new core module andpackages/core/package.jsonexports it.6. Docs
CLAUDE.md/AGENTS.md gains an HTML Refresh paragraph under the Annotation System, notes the shared threading rule and the PATCH validation, and no longer points the WebMCP section at an untracked design file. The marketing annotate page documents Refresh in its HTML section, and the WebMCP reference lists the
comment_only_surfacenudge thatnudges.tsalready emits.Verification
bun run typecheck;bun test packages/ui packages/editor packages/core packages/server/annotate.test.ts(1087 pass); the Pi annotate suites plus the new Pi external-annotations test (31 pass);DOM_TESTS=1on the HTML-surface suites (srcdoc, htmlPinpointProtocol, htmlLiveProtocol, App.htmlChrome, HtmlSurfaceControls, useHtmlRefresh, useSharing.contentRevision, the AnnotationPanel tests, App.archiveReadOnly; 216 pass);build:reviewandbuild:hook; guides.showbuild:viewer,check:manifest(in sync, no manifest change) andcheck:budgets. All server runs were sandboxed under a scratchPLANNOTATOR_DATA_DIR.AI-assisted (Claude) under maintainer direction.