mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
feat/opencode2-native-commands
133 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Michael Ramos
|
4465950f0c |
feat: add bounded annotation undo and redo (#1426)
* feat: add bounded undo and redo history * fix: address undo redo review feedback * chore: refresh guide viewer manifest * fix undo history review regressions * chore: refresh guide viewer manifest |
||
|
Michael Ramos
|
c2950e709f |
fix: pre-release QA findings for 0.27.9 (#1405)
Fixes from the 0.27.9 pre-release review. Servers: an unreadable rendered-HTML root falls back to the startup snapshot on both runtimes with a once-per-process warning instead of hanging (Pi) or answering 500 (Bun); the version diff is recomputed against current bytes on reload and carried through the in-app Refresh instead of being dropped, with no history write on a GET. Client: a Refresh action on the compact touch shell; HtmlSurfaceControls renders Refresh independently of the eye; the dead HtmlSurfaceActions removed. Threading: one linear, cycle-safe reply resolution shared by the annotations panel, its sort, and the export (5,000-chain tests), PATCH ingest on both runtimes rejects self-references and cycles, nothing is ever dropped from feedback. WebMCP and viewer hygiene: bounded tombstone and request memories, per-instance minted ids, nudge id caps, waiter cleanup on unmount, a shared retry epoch for diagram blocks. Docs: HTML Refresh documented, the WebMCP design pointer fixed, marketing pages updated. AI-assisted (Claude) under maintainer direction. |
||
|
Michael Ramos
|
8e0c51f5ff |
fix(ui): 0.33.0 adoption feedback and bump @plannotator/ui to 0.34.0 (#1402)
Follow-ups from the Workspaces adoption of 0.33.0: a math-slot module hosts can redirect Mermaid's own katex import to (importer-scoped resolveId recipe in HANDOFF) so a math document fetches one KaTeX chunk owned by the host; HtmlViewer bridgeErrorDisplay ('banner' default, 'none' lets a host own the failure banner while onBridgeUnavailable still fires); the documented bridge alias narrowed to relative sibling imports; resetMathRenderer keeps a registered loader and discards stale in-flight loads via an epoch, with setMathRendererLoader(null) and getMathRendererLoader added. Plannotator's own bundles unchanged (markers and sizes within noise of main). Bumps @plannotator/ui to 0.34.0; core stays 0.25.0.
AI-assisted (Claude) under maintainer direction.
|
||
|
Michael Ramos
|
e807e2b89c |
fix(ui): 0.32.0 adoption feedback from hosts (#1399)
Follow-ups from the Workspaces adoption of 0.32.0: the default KaTeX loader moves to its own module so a host that registers a mathRendererLoader can alias it away and drop the unused KaTeX chunk (a registered loader is never backfilled by the default); docs state when onUnanchoredChange first delivers, what projectHostThreads loses on markdown surfaces (export ordering, line labels, repeated-text disambiguation, no-flash restore) while still re-anchoring by text search, and that cap-dropped handling is a backstop once maxAdditionalTargets is enforced upstream; HANDOFF.md now ships in the tarball so the README references resolve. Plannotator's own behavior is unchanged (the eager entry fills the slot before first render; built-bundle markers and sizes match main). AI-assisted (Claude) under maintainer direction. |
||
|
Michael Ramos
|
0b167cc478 |
perf(ui): lazy diagram and math renderers with eager entries for Plannotator (#1394)
Bundle-weight optimization of @plannotator/ui for multi-chunk hosts, requested by Workspaces: the Mermaid runtime and Graphviz engine load inside the render effect, the username dictionary sits behind a synchronous identity generator slot, and KaTeX sits behind a math renderer slot with a loader seam on configurePlannotatorUI. Plannotator's own apps import eager entries (math, identity, and Mermaid for the plan editor) so their behavior is unchanged: single-file builds within noise of main, math typeset on first paint, identities from the full dictionary, and the share portal keeps Mermaid in its entry chunk so its failure surface matches main. Built-HTML registration markers guard the eager imports. Hosts that omit the eager entries get the lazy paths, a one-shot automatic re-attempt, and a Retry affordance on the diagram error panel; the module-map limitation of in-page retries is documented. AI-assisted (Claude) under maintainer direction. |
||
|
Michael Ramos
|
6903d7a3dd |
feat(webmcp): expose plan review and annotate as WebMCP tools for browser agents (#1393)
Phase 1 of WebMCP support: a zero-dependency, feature-detected engine in packages/ui/webmcp plus a read-and-comment tool catalog for plan review and annotate (read_document, add_comments, update_comment, remove_comments, reveal, nudge_user, list_documents). No decision tools; the human approves. Zero footprint in browsers without document.modelContext (DOM, network, console, timers, and cookies identical to main), idle until called where the API exists, and never registered inside the annotate iframes. Adds an optional inReplyTo field on annotations for threaded replies. Client-only; no server changes. AI-assisted (Claude) under maintainer direction. |
||
|
Michael Ramos
|
0ae40e73a4 |
feat(annotate): restore a restricted thumbs-up on comment-only HTML surfaces
The v0.27.5 comment-only ruling removed every label affordance from HTML and live-app annotate surfaces, leaving no one-click positive feedback: the only path was opening the composer and typing prose. Restore exactly ONE affordance, the hardcoded 'Looks good' thumbs-up, on both input routes: - selection toolbar: commentOnly + a provided onQuickLabel now renders only the thumbs-up (no Delete, no Zap picker, Alt+digit suppressed); HtmlViewer passes a handler that filters by label id as defense in depth - pinpoint: the composer gains an optional one-click 'Looks good' footer action (disabled once anything is typed, so it can never discard a draft), emitting the same isQuickLabel comment shape with the draft's multi-select targets The trust-boundary clamp is untouched: redline/quickLabel modes stay collapsed to selection, so a hostile page still cannot force a DELETION or an arbitrary label. THUMBS_UP_LABEL moves to utils/quickLabels as the canonical definition. |
||
|
Michael Ramos
|
67f47dbac1 |
fix(annotate): armed-mode interaction fixes from the v0.27.5 QA gate (#1363)
* fix(annotate): pre-release QA fixes for the armed-mode interaction seams
Six confirmed QA findings on the HTML/live annotate surface plus missing
pi-extension resync coverage:
1. Armed pinpoint drifted click (>4px, no selection) was swallowed AND
leaked to the page: the always-on drag work armed the trailing-click
suppression on drift alone. The mouseup arming site now requires the
drag to have actually produced a text selection; drifted clicks pin
normally and never reach the page. Bridge tests for armed drift,
armed real drag, and Interact drift.
2. Esc ladder: hover-clear is no longer its own rung; clearing the
pinpoint outline and posting annotate-exit happen on the same press
when no draft is open. Draft-close keeps its own press.
3. Compact touch layouts no longer apply a restored toolsHidden:true
chrome cookie (both header toggles are desktop-only, so applying it
stranded the user); the cookie value is preserved for desktop.
4. The live-app probe now announces the static-conversion downgrade on
stderr when a loopback probe fails, naming --app to force live mode.
5. Live-app export: page group headers are now '## Page:' with '### N.'
entries nested below them; exports without pageUrl stay byte-identical.
6. Shift+1-4 mode shortcuts no longer fire while the annotation
toolbar's type-to-comment listener owns printable keys, so typing
! @ # $ into a starting comment cannot silently switch modes.
Also adds the missing tests for the two resyncPhaseFromSession
executing->idle fallbacks that arm idleNoticePending (verified by
mutation: flipping either arm fails its test).
* fix(annotate): compact arm/disarm affordance, guarded shutdown, restored chrome guards
Follow-up scope from the forensics sweep, same surface:
- Compact touch layouts get Options-menu actions for the HTML/live
surface: 'Annotate page'/'Interact with page' (the desktop pen and
Mod+Shift+A were unreachable on touch, so every tap annotated with no
way out) and 'Show tools'/'Hide tools' (the desktop eye). With the
menu as the way back, the toolsHidden cookie now applies on compact
again (desktop parity) instead of being ignored.
- The annotate servers' stop() now guards every disposal step
individually (Bun: runGuardedShutdown, mirrored inline in Pi): a
throwing agent-terminal teardown (#1314-class) no longer skips
liveProxy.stop() and the other disposals after it. Unit-tested with a
throwing disposer.
- Re-added the two regression guards dropped in the htmlHideTools ->
htmlChrome test rename: the restore commit never writes stale
pre-restore chrome values to the cookie, and the sidebar stays
reachable via Mod+B while tools are hidden.
* fix(annotate): scope the Agent TUI display reset to display settings only
The Display popover's 'Reset terminal display settings' button also called
onSideChange('left'), durably overwriting a user's chosen right/hidden
placement in config.json with no disclosure — the label scopes the reset
to font/appearance. Position is a layout preference with its own explicit
segmented control right below, so the reset no longer touches it: the
button now resets exactly the display settings through the panel's one
sanitized update path, and the popover no longer has any code path from
reset to the side.
AgentTerminalDisplayPopover is now exported with a defaultOpen test seam
(the surrounding panel needs a live WebTUI session to render it); tests
assert reset restores the display defaults without firing onSideChange,
and that the Position control remains the explicit way to change
placement.
|
||
|
Leonardo Reis
|
81ecd67e75 |
feat(annotate): configurable Agent TUI placement with durable config and Hidden state (#1050)
* Allow annotate terminal to dock on either side * Allow annotate terminal to dock on either side * fix(annotate): persist Agent TUI preferences through the settings registry The Position control introduced in #1050 stored its choice in a cookie via hand-rolled helpers that bypassed the settings registry. Every annotate session runs on its own random port, so a cookie is scoped to one session: the placement silently reset on the next annotate. The sibling `plannotator-annotate-agent-terminal-default` cookie (preferred agent) had the same gap. Both now follow the `conventionalComments` precedent exactly: * `agentTerminalSide` and `agentTerminalDefaultAgent` join `SETTINGS` with serverKey/fromServer/toServer, reusing their existing cookie keys so a user who already picked a side keeps it across the upgrade. * `PlannotatorConfig` gains both as flat keys (only diffOptions, theme, reviewAnalysis and prompts deep-merge in saveConfig), emitted from `getServerConfig()` behind an `isAgentTerminalSide` guard so a hand-edited config.json cannot advertise a side that does not exist. * Both keys are added to the two /api/config allowlists: the Bun annotate server and the hand-mirrored Pi one. The side vocabulary moves to @plannotator/core/agent-terminal (widened to include the `hidden` state added next) so the registry can reach it without closing an import cycle through ConfigStore; the ui util keeps its public API by re-exporting. Regenerates the pinned guide viewer manifest, which shifts by 0.1 KB gz because the settings registry now reaches into core/agent-terminal. AI-assisted (Claude) under maintainer direction. * feat(annotate): add a Hidden Agent TUI position and extract its layout Builds on the Left/Right Position control from #1050. Hidden (third state of the Position control) Hidden is a durable preference that the Agent TUI is not part of this user's layout: nothing is docked, and choosing Hidden while the terminal is open closes it (from either surface that offers the control). It is a default, not a lock. The rail toggle, the Shift Shift shortcut and a message routed to the agent all still open the panel for the session, and none of them rewrites the preference, so explicit intent wins now without changing what happens next session. A `hidden` preference owns no dock edge, so a session open falls back to the historic left placement. Because the Position control lives inside the terminal's own popover, and Hidden closes that popover along with the terminal, the same control is now also in the Settings dialog (General tab, annotate mode). That is the way back from Hidden, and it also answers the review note that Position could not be preconfigured before the terminal was ever opened. It is gated on the terminal actually being available in the session so a remote or runtime-less annotate never offers a dead control. Both surfaces write the same `agentTerminalSide` config value and read it through ConfigStore, so they cannot drift. The existing transient hide affordances (header X, resize handle click and drag-snap, rail toggle, Shift Shift) are unchanged and stay session scoped. A running agent still stays mounted off-layout when collapsed, so hiding the panel never kills the PTY. Review fixes * Extract `getAgentTerminalLayout` from App.tsx into packages/editor/agentTerminalLayout.ts with a table test over {side including hidden} x {open} x {running} x {wideMode} x {belowBreakpoint} x {rightPanelOpen}, asserting the invariants that can actually regress: never docked on both edges, never visible below `lg` or in wide mode, a collapsed running terminal stays mounted zero-width on its own edge, and the right panel is suppressed exactly when a VISIBLE right-docked terminal holds the slot. * Fix `aiSurfaceOpen`, which still read `effectivePanelOpen && rightSidebarTab === 'ai'` after its siblings moved to `isRightPanelVisible`. A right-docked terminal visually suppresses the panel but left the Ask AI model-discovery effect firing for an invisible surface, which is exactly the eager provider work that gate exists to avoid. The layout computation is hoisted above the consumer so it can use the same fact the JSX does. * Document the right-slot invariant at both coordination sites. The asymmetry is deliberate: the panel evicts the terminal (which keeps running off-layout, so reopening resumes the same session), while the terminal only suppresses the panel visually so dismissing it restores the user's place. Symmetry would make every short terminal detour cost the reviewer their open surface. * Name the `useIsMobile(1024)` literal `AGENT_TERMINAL_LG_BREAKPOINT`, tied to the panel's own `hidden lg:flex`. * Restore `hideAgentTerminal()` in the resize hook instead of the raw setter, and point the handle at the resolved placement. AI-assisted (Claude) under maintainer direction. --------- Co-authored-by: Michael Ramos <backnotprop@gmail.com> |
||
|
Michael Ramos
|
2ca55c8332 |
feat(annotate): live local app annotation through a loopback reverse proxy (#1352)
* feat(bridge): additive live-mode gate + LIVE_BRIDGE_BOOTSTRAP Adds the config-gated live branch to BRIDGE_SCRIPT: frame gate, pinned parent origin, token-stamped postToParent, origin+token checks on both inbound handlers, pinpoint-only clamp, vim and resize off, pageUrl on ready, and coalesced page-change reporting for SPA history navigation. With no config present (srcdoc) every branch is inert and behavior is unchanged; the existing html-viewer suites pass unmodified as the regression proof. LIVE_BRIDGE_BOOTSTRAP installs the annotation CSS from the JSON config prelude before the IIFE runs. New package export exposes the string constants without the React barrel. * feat(ui): live-session parent side for proxied app annotation useHtmlAnnotation gains a live option (origin + token validated before parseBridgeMessage; token + concrete targetOrigin on every outbound post) and a validated page-change message with onPageChange. HtmlViewer gains src/liveSession/currentPageUrl/onPageChange: src-mode iframe with no sandbox and no srcdoc, ready pageUrl handling, per-page restore filtering with explicit clear-marks + re-sync on navigation, and one postToBridge choke point for its direct posts. Annotation.pageUrl is additive; exportAnnotations groups by page (with global numbering kept) only when a pageUrl is present, byte-identical otherwise. AnnotationPanel shows the page label; AnnotationToolstrip can hide the input switch. The editor app wires mode annotate-app: full-viewport live surface, forced pinpoint, vim off, diff/share hidden, pageUrl stamping. * feat(server): loopback reverse proxy for live app annotation Whole-origin mirror of a local dev server on a dedicated 127.0.0.1 port: streaming bridge injection (after the head open tag, before a bare </head>, or appended; exactly one per document; 8-byte holdback plus a state machine for tags split across chunks), header hygiene (upstream Host rewrite, X-Forwarded-*, identity Accept-Encoding on document intent only, hop-by-hop strip), CSP drop-and-replace with frame-ancestors listing the editor origins, X-Frame-Options removal, target-origin Location rewrite, byte-identical passthrough for assets and encoded HTML (no injection, once-per-session diagnostic), SSE streaming, and WebSocket passthrough with a bounded pending queue for HMR. Host header validation runs before any upstream contact; the bind is the literal loopback constant and the advertised-URL override is never applied. Tests boot a fake dev server and cover injection, hygiene, fidelity, WS echo, and the security posture. * feat(annotate): annotate-app server mode + CLI live probe with remote hard-off startAnnotateServer gains mode annotate-app and a liveApp option: it throws under PLANNOTATOR_REMOTE, generates the per-session token, composes the proxy-served bridge body (JSON config prelude with both editor origin forms, localhost first, plus bootstrap and bridge supplied by the caller so packages/server never imports @plannotator/ui), starts the loopback proxy after the annotate port is known, serves the live /api/plan payload (no rawHtml, no version fields, sharing off), and stops the proxy with the server. Version history and durable submission records stay excluded via the explicit mode gate. The CLI resolution probes loopback http URLs (3s, accept text/html) and defaults them to live mode when the probe returns HTML; --static forces conversion, --app forces live and fails loudly on non-loopback, https, unreachable, or non-HTML targets; both flags are mutually exclusive transport-shape flags never echoed in the tolerant handoff. A live resolution under PLANNOTATOR_REMOTE is a startup failure suggesting --static. OpenCode and Pi parsers are untouched this phase. * test(live-annotate): protocol, server, and probe suites + smoke script + docs htmlLiveProtocol.test.tsx covers the parent trust boundary (origin and token rejection before parseBridgeMessage, token + targetOrigin on every outbound post, validated page-change and ready pageUrl, per-page restore filtering with full-list numbering) and the bridge live gate, executed as the composed config + bootstrap + bridge body inside a dedicated harness iframe so the srcdoc suites keep running the same script uncontaminated in this process. annotate.test.ts gains annotate-app cases (live payload shape, composed bridge served by the proxy, no-history version endpoints, proxy stopped with the server, remote rejection); annotate-live-resolution.test.ts covers the probe matrix. The two post helpers now drop unmatched-targetOrigin posts silently, matching browser semantics where some DOM environments throw. Adds the manual Vite/Next smoke script and the AGENTS.md live app annotation section (phase gate, security posture, limitations). * test(annotate-cli): cover the CLI layer of the live app remote hard-off Spawns the real CLI entry (async, so the in-process fake app can answer the live probe) with PLANNOTATOR_REMOTE=1 against a loopback HTML server and asserts the startup-failure exit with the --static hint. Completes per-layer coverage of the three-layer hard-off (CLI exit, server throw, unconditional loopback proxy bind). * fix(live-annotate): harden the loopback trust boundary end to end - isLoopbackHostname (now canonical in live-proxy.ts, re-exported by the CLI resolution) requires localhost, ::1, or a LITERAL 127/8 IPv4 address: DNS names like 127.0.0.1.evil.example no longer classify as loopback, so neither the default probe nor --app can start a live proxy against an off-box origin. - The live-eligibility probe judges the FINAL response URL: a target that redirects off its loopback origin falls back to the static pipeline (or fails loudly under --app) instead of opening a live session whose iframe immediately leaves the proxy. - WS upgrades with a browser Origin not naming the proxy itself are refused, so a hostile page's cross-site connect is never laundered into the origin-less shape dev servers trust as a non-browser client (Vite CVE-2025-24010 class). - /__plannotator__/bridge.js refuses cross-site/same-site Sec-Fetch-Site fetches: the per-session token is no longer readable via an off-origin script include on modern browsers. - X-Frame-Options is stripped only on HTML responses (where frame-ancestors replaces it); non-HTML responses keep the app's own framing protection. - Redirect Locations are re-anchored by loopback-host + port equivalence instead of a string prefix: alternate loopback spellings are now caught and lookalike ports (5173 vs 51730) pass through untouched. - --app on a non-URL target fails loudly instead of being silently swallowed. * fix(live-annotate): session correctness for SPA restores, origins, and pathful targets - A live find-and-mark that resolves nothing keeps its record, seeded with unresolved placeholder targets from the durable anchor/text params, so the mutation-driven reconcile re-acquires the pin once a lazy route or data-dependent tree renders (SPA navigation no longer permanently drops pins). Srcdoc restores keep the fail-closed drop. - The bridge posts every outbound message once per listed editor origin; the browser delivers only the one matching the parent document, so an editor opened at 127.0.0.1 instead of localhost no longer silently loses ready and every subsequent message. - The advertised appUrl is the proxy under its localhost spelling with the target URL's own path and query: the framed app stays same-site with the editor, shares the dev app's host-only localhost cookies and storage, and a pathful target opens its page instead of the app root. The proxy still binds the 127.0.0.1 literal. * ci(live-annotate): run the live protocol DOM suite; document the hardened posture htmlLiveProtocol.test.tsx is DOM-gated and was absent from the workflow's DOM_TESTS file list, so none of its trust-boundary assertions ran in CI. Add it, and update the live-app section of the project docs: literal-loopback gate, probe redirect rule, WS Origin check, bridge.js delivery gate, localhost appUrl advertisement, live restore resilience, and the remote-mode behavior change (loopback URL annotate under PLANNOTATOR_REMOTE now exits asking for --static instead of silently converting). * fix(live-annotate): absorb the v0.27 mainline into the live session surface Post-rebase seam work after replaying the branch onto main (v0.27.4 era): - Route the bridge's unanchored-transparency report through postToParent so live sessions deliver it token-stamped to the listed editor origins; the raw '*' post main introduced for srcdoc would be dropped by the live parent's message authentication exactly where restores fail most. New live-harness test pins the contract. - Extend the live remote hard-off to --tailscale sessions (flag postdates the branch): CLI startup failure + startAnnotateServer throw keyed on tailnetPublished, matching how the annotate agent terminal treats tailnet publication. Covered in annotate.test.ts and documented in AGENTS.md. - Keep main's compact-touch input controls and effective mode/input values on the HTML surface while preserving the live pinpoint-only clamps. - Regenerate the pinned guide-viewer manifest (CSS hash moved with the new UI classes; JS unchanged). * feat(live-annotate): Interact/Annotate mode toggle for live app and raw HTML sessions A live app session used to be unusable: the pinpoint capture-phase click handler owned every click, so buttons, checkboxes, inputs, and links never fired. One boolean mode now governs the HTML/live viewer surface: - Interact: the bridge is fully passive. Pinpoint capture, hover outline, drag-selection toolbar, [data-annotate] clicks, and committed-highlight click interception are all gated behind annotateModeActive, so clicks, forms, text selection, and SPA navigation reach the page natively. Committed markers and highlights stay VISIBLE, and marker buttons keep their clicks (a marker click still opens its comment). - Annotate: classic behavior, unchanged. Live sessions annotate exclusively via pinpoint while armed. Control: a single bubble icon button in the editor header (icon never changes; armed = accent + visible border, idle = transparent border of the same width, so the box is pixel-identical in both states), plus a subtle inset accent ring floated over the viewer while armed (pointer-transparent, no layout shift). Keyboard: Mod+Shift+A through the shortcut registry (html-annotate scope; the bridge mirrors the chord inside the iframe and forwards it over the authenticated postToParent path). Esc gains a final ladder rung: draft closes first, then the hover outline clears, then Esc exits Annotate back to Interact (bridge posts annotate-exit; a parent-side listener covers Esc with editor focus). The parent owns the mode and pushes it with the same re-post-on-ready pattern as set-input-method, so it survives live page changes, HMR reloads, and bridge re-injection without ever reloading the iframe. Defaults: live app sessions START in Interact; static/raw HTML sessions START in Annotate (today's behavior preserved, and the srcdoc bridge default keeps behavior byte-identical when no set-annotate-mode ever arrives). Session-only state, no persistence. Vim navigation is available only while Annotate is armed. Covered by new bridge-harness and parent-side DOM tests in htmlLiveProtocol.test.tsx and htmlPinpointProtocol.test.tsx: Interact pass-through, armed capture, the Esc ladder order, mode survival across re-injection, marker clicks in Interact, and both defaults. * feat(live-annotate): pinpoint-armed default, always-on drag comments, comment-only HTML surfaces Simplifies the Interact/Annotate design after live review. The new contract replaces the previous one where they conflict: - BOTH surfaces (raw HTML and live app) now START ARMED with pinpoint; the live-session Interact default is gone. Esc keeps the ladder (close draft, clear hover, then exit to Interact) and the header toggle re-arms. The bridge also paints the pinpoint cursor at init instead of waiting for the parent's first round trip. - The header toggle is a PEN icon: the old bubble sat next to the annotations-panel bubble and the two were indistinguishable. Same box geometry (armed = accent + visible border, idle = transparent border of identical width), aria-pressed, Mod+Shift+A, and the armed ring over the viewer are all unchanged. - Text drag-selection commenting is ALWAYS live on HTML/live surfaces, in BOTH states: the selection pass is ungated from annotateModeActive and from the pinpoint input method. In armed pinpoint, click = pin an element and drag = select text, simultaneously; the >4px drag arming decides which one a gesture was, a completed drag's trailing click never re-pins (one-shot dragEndedClick), and a plain click is never swallowed (the pass only acts on a real selection and never preventDefaults). Esc in Interact still closes an open drag draft before yielding to the page. - HTML/live surfaces are COMMENT-ONLY: useHtmlAnnotation clamps redline/quickLabel (both the host mode and a bridge-posted modeOverride, so a hostile page cannot force a DELETION), the selection toolbar drops Delete and quick labels behind a new commentOnly seam on AnnotationToolbar, and the quick-label picker portal is gone from HtmlViewer. Markdown surfaces keep the full toolbar, and persisted DELETION annotations still restore. - The "Show tools"/"Hide tools" header button is removed. It hid the floating toolstrip (now gone from HTML surfaces entirely: with comment-only plus both input paths live there is nothing left to switch), the collapsed sidebar tab flags, and the viewer's floating action cluster (attachments + global comment + version-diff toggle), all of which are now always visible. htmlChrome persistence keeps only the sidebar/panel state; an old cookie's toolsHidden flag is read tolerantly and ignored, so a stale record cannot strand a user with hidden chrome and no way back. - HTML surfaces pin the viewer input method to pinpoint (the drag/ pinpoint switch is meaningless when both are live); the Alt input switch no-ops there. Vim stays armed-only, as built. No server, proxy, or protocol-security changes; the armed flag stays session-only. Tests: the live-bridge harness is reworked around the armed default (forged-DISARM posture, drag-selection passes in armed and Interact, the trailing-click guard), the pinpoint suite covers the comment-only toolbar and the redline/quickLabel clamp at the trust boundary, a new AnnotationToolbar.commentOnly seam test guards both surfaces' toolbars, App.htmlChrome.test.tsx replaces App.htmlHideTools.test.tsx (no tools button, stale-cookie tolerance, pen armed default), and the htmlChrome tests cover the narrowed persisted shape. * feat(live-annotate): collapsible floating controls cluster The simplification removed the Hide tools toggle, which left the floating comment/attachments cluster permanently over the page. Restore a hide affordance on the cluster itself: a collapse chevron shrinks it to a small expand pill in the same corner, so the page is never obstructed without a way back. Collapsed state persists with the rest of the HTML chrome cookie (sidebar/panel), tolerantly read. Hosts that do not wire the toggle (readOnly viewers, review-editor panels) are unchanged. * feat(live-annotate): header Show/Hide tools replaces the collapse pill The collapse pill was a half measure: it left its own artifact over the page and the sidebar tongue tabs stayed. Revert it and restore the real thing as a header control: an eye toggle immediately left of the pen that removes ALL floating chrome over the page from the DOM (sidebar tongue tabs + the comment/attachments cluster), leaving nothing behind. The toggle lives in the header, so a hidden state always has a way back, which also makes honoring a persisted (or pre-existing) toolsHidden cookie safe again. |
||
|
Michael Ramos
|
d2d2dba7fa |
feat(annotate): configurable extra markdown extensions (#1309)
* feat(annotate): configurable extra markdown extensions (#1307) Adds a config-only `markdownExtensions` key to ~/.plannotator/config.json, e.g. { "markdownExtensions": [".livemd"] } for Livebook notebooks. A listed extension is accepted everywhere .md is on the annotate path: CLI target resolution, folder discovery and the file browser, /api/doc plus relative and wiki-link navigation between sibling docs, the 2MB size cap, and per-file version history. Listed extensions render as markdown with frontmatter stripped, never as raw HTML, and they only widen the accepted set. Design: - packages/core/annotatable.ts stays browser-safe and zero-dep. Its regexes and predicates now take an optional, defaulted-empty list of extra extensions, plus a normalizer and regex builders. - packages/shared/markdown-extensions.ts is the node-side seam: it reads config.json once per process through the existing loadConfig() and threads the normalized list into those pure functions. resolve-file re-exports the config-aware predicates so both runtimes pick them up; the Bun server, the Pi mirror, the OpenCode plugin and the CLI all go through them. - The annotate /api/plan payload ships the resolved list so the renderer can linkify links to sibling documents (module-level UI registry, empty by default, so nothing changes without config). Validation: entries must be dot-led, lowercase-normalized, and free of path separators, globs and whitespace. Invalid entries are dropped silently, built-ins are deduplicated, and `.env` is denylisted so config can never register it (annotate copies file contents into the data dir). Deliberately unchanged: the Pi plan-write allowlist (ALLOWED_PLAN_EXTENSIONS in tool-scope.ts) and Edit Mode source save (SOURCE_SAVE_FILE_REGEX), which keep their own narrower allowlists. * fix(annotate): deny the dotenv family and sandbox config-aware tests Review follow-ups on #1309: - deny the whole dotenv family (.prod.env, .env.local, ...) in normalizeMarkdownExtensions, not just the exact .env name - resolve config.json path per call instead of at module scope so PLANNOTATOR_DATA_DIR sandboxing works in single-process test runs - stop resolve-file.test.ts reading the real user config: pure predicate imports plus pinned empty extras on every resolve call - add the config.json -> memo -> predicate integration test using resetMarkdownExtensionsCache under a temp data dir * test(call-flow): make the stale-read advert test self-sufficient The read-only GET only probes the node runtime while Call flow is enabled. The stale-read test relied on earlier tests' settings POSTs leaking callFlow=true through the process-frozen config path; with lazy config resolution each sandbox is genuinely isolated, so the test now enables Call flow in its own data dir. Locally the dependency was masked by an fnm-shimmed sem sidecar spawning node coincidentally. |
||
|
Michael Ramos
|
e181b824cc |
Mobile-safe plan and code comment composition (#1297)
* feat: harden mobile comment composition * fix(ui): keep mobile app inside Safari viewport * docs: record physical mobile triage * fix(ui): extend plan canvas behind Safari controls * fix(ui): let mobile plans drive Safari chrome * fix(ui): release Safari top edge on mobile plans * docs: triage mobile feedback and close phase 1b * fix(ui): harden compact touch behavior |
||
|
Michael Ramos
|
d3633c9c52 |
Mobile foundation and quieter first run (#1295)
* feat: establish mobile foundation and simplify onboarding * fix(ui): finish mobile foundation cleanup |
||
|
Michael Ramos
|
356b628b6f |
ci(security): add Semgrep CE and Trivy monitoring (#1294)
* ci(security): add Semgrep CE and Trivy monitoring * fix(ci): diagnose Trivy coverage assertions * fix(ci): accept Trivy repository scan metadata * fix(ci): harden scanner failure diagnostics |
||
|
Michael Ramos
|
1bf90a2357 |
feat(editor): focus-mode shortcut to toggle both sidebars (#1279)
* feat(editor): focus-mode shortcut to toggle both sidebars (#1276) Keyboard-first reviewers had to reach for the mouse to collapse the Contents sidebar and the right-hand panel every time they wanted to read a document. Bind that to one key. Mod+. now toggles the existing `focus` view mode from the keyboard, on both the plan review and annotate surfaces. The first press remembers whatever was open and closes both panels; the next press restores that exact arrangement, so a session with only one panel open comes back the same way. Any manual open (a sidebar tab, the annotation panel, the agent terminal) already exits the view mode and clears the memory, so the shortcut cannot leave the layout stuck. The binding was picked after auditing every plan-review and annotate binding: Mod+B / Mod+Shift+B are the sidebar toggles, Mod+S, Mod+P, Mod+Enter, Mod+C and Mod+Z are taken, and code review already uses Mod+. for its own "collapse the chrome" toggle. It is unshifted on every keyboard layout and claimed by no browser or OS default. The dispatcher gate is shared with the annotate sidebar shortcuts, so the key is inert while a dialog, an overlay, a submission, or a text field owns the keystroke. HTML surfaces are excluded because they own their own persisted chrome and never render the Focus control. * fix(editor): keep focus-mode exit reachable on HTML surfaces; note the HTML caveat in help |
||
|
Michael Ramos
|
8e7b5ce300 |
feat(review): refine Call Flow navigation and annotations (#1277)
* feat(review): refine Call Flow navigation and annotations * fix(review): align viewed controls with panel navigation * feat(review): add Call Flow path search controls * fix(ui): wrap long tooltip identifiers * feat(review): annotate raw Call Flow output * feat(review): refine call flow lens context * fix(review): keep call flow lens search accessible * fix(review): scope call flow find shortcuts |
||
|
Michael Ramos
|
7d7654fcda |
feat(annotate): minimal-by-default HTML sessions with stale-preference decay (#1260)
* feat(annotate): minimal-by-default HTML sessions with stale-preference decay Raw-HTML annotate sessions now open minimal by default: Pinpoint input, tools hidden, left sidebar closed, and the right annotations drawer closed (the drawer joins the persisted HTML chrome state, previously it always opened on desktop). Explicit user choices still persist between HTML sessions, but the records now carry a timestamp and expire after 7 days without a refresh, so users who have not changed anything or annotated HTML in a while come back to the product defaults. Explicit changes and annotation activity both re-stamp the records, so active users keep their setup. Legacy untimestamped cookies are treated as expired (a one-time reset to the new defaults). The markdown surface keeps its own preference with no TTL, unchanged. Mutation-verified: disabling the TTL fails 4 tests. * test(annotate): stamp seeded chrome cookies and compare semantic fields The App-level chrome suite seeded legacy untimestamped cookies, which the stale-preference decay now treats as expired by design, and one assertion compared cookie bytes that re-stamping legitimately changes. Seeds carry a fresh savedAt and the write-integrity assertion compares the chrome fields. |
||
|
Michael Ramos
|
f8951cd3c6 |
feat(annotate): shift-click multi-element selection for raw-HTML pinpoint (#1254)
* feat(annotate): shift-click multi-element selection for raw-HTML pinpoint drafts One comment covering multiple targets: shift-clicking elements while a pinpoint draft composer is open toggles them in/out of the SAME draft. - bridge: pendingMultiTargets registry with per-target pinned outline boxes, DOM-identity + anchor-equality toggle dedup, primary promotion on removal, draft cancel on last removal, shift-hover preview, rAF pointer relay for the composer yield, remove-target/flash-target parent messages, capped at 16 additional targets at the source - pins: registerPin now allows several elements per annotation id (deduped by (id, element)/(id, anchor)); badges number by first-seen id so all targets of one annotation share one number; find-and-mark restores additionalAnchors as same-numbered pins (anchor-only, fail-closed) - parent hook: multi-target-added/removed/pointer messages validated and capped at the trust boundary (key<=64, label<=64 truncated, text via the 10k surrogate-safe cap, anchors via parseHtmlElementAnchor, array cap 16); draftTargets state, chip removal with deterministic promotion mirrored on both sides, composerFocusToken for focus return - types: additive Annotation.htmlAdditionalTargets (label/text/anchor per extra target; anchor optional so fail-closed targets still export) - CommentPopover (all seams optional, default off): horizontally scrollable target chips with remove buttons and hover-to-flash, refocusToken, captureStrayKeys first-keystroke guard, yieldState fade/click-through with 180ms transition and prefers-reduced-motion fallback - HtmlViewer: composer-yield state machine (composerYield.ts) fed by parent mousemoves plus bridge-relayed pointer positions, with 48px/96px hysteresis - export: multi-target comments append an 'Also applies to N more elements' block (label + excerpt per target); single-target output byte-identical - share URLs: additional targets are dropped exactly like htmlAnchor (the compact tuple format never carried anchors); drafts carry them verbatim * test(annotate): cover shift-click multi-select across bridge, DTO, composer, export, drafts, sharing - srcdoc.test.ts (bridge DOM): shift-click add + toggle-off with echoed removals and per-target pinned boxes; create-mark commits all targets under one id with one badge number (second annotation numbers 2); primary promotion and last-removal cancel; parent remove-target mirrors without echo; flash-target; 16-target cap at the source; find-and-mark restores additionalAnchors as same-numbered pins with stale anchors failing closed - htmlPinpointProtocol.test.tsx: multi-target-added/removed/pointer DTO validation (key/label/text/anchor caps, hostile payloads), selection targetKey/targetLabel validation; mounted-composer flows — primary chip, shift-adds into ONE submitted comment carrying htmlAnchor + 2 additional targets, single-target submit shape unchanged, promotion onto the comment, last-removal closes the composer, chip removal, 16-cap at the trust boundary, drag selections never arm multi-select - CommentPopover.multiTarget.test.tsx: chips render primary-first with remove/hover handlers, refocusToken focus return, captureStrayKeys stray keydown routing (and non-interference when focused), yieldState classes + reduced-motion-aware 180ms style; default composer renders none of it - composerYield.test.ts: distance + hysteresis state machine (48px over-exit, 80/96px near enter/exit) - parser.test.ts: multi-target export block (labels, excerpt clipping, fail-closed targets) and byte-identical single-target output - useAnnotationDraft.seam.test.tsx: multi-target annotations round-trip the draft transport verbatim (save body + restoreDraft) - sharing.multiTarget.test.ts: share tuples drop anchors AND additional targets while the comment itself still shares * fix(annotate): keep pinpoint drafts alive when the pinned element scrolls out of view Found by the real-browser signoff harness: reaching a second element to shift-click often scrolls the pinned primary out of the viewport BEFORE any additional target exists, and the scroll-out teardown then cleared the bridge's pin state mid-compose — the shift-click landed on a dead draft and started a new one instead of adding to it (and even a single-target pinpoint draft silently lost its visual pin on commit after scrolling). Pinpoint drafts (pendingPinViaPinpoint) now survive scroll-out; drag selections keep the existing close-on-scroll-out behavior unchanged. * fix(annotate): address adversarial review of multi-select (arm handshake, label sanitization, iframe shift relay, removal resync) D1 (blocker): the bridge accepted shift-toggles for ANY pinpoint draft while the parent only mirrors targets when the comment composer owns it — in quickLabel mode the user could pin elements the saved annotation would never carry. Multi-select is now ARMED EXPLICITLY: the parent posts arm-multi-select (keyed to the primary, so a stale arm can never arm a new draft) only from the composer flow, and the bridge refuses the toggle — shift-click behaves as a plain click — until armed. D2: target labels derive from page-controlled attributes (aria-label), so newlines could smuggle real markdown structure (fake headings) into agent-read feedback. parseTargetLabel now collapses all whitespace at the trust boundary, and the exporter collapses again (defense in depth for persisted pre-fix drafts). D3: the composer yield armed Shift only from parent-window keydown/mousemove, but window blur (focus entering the iframe) cleared it and modifier keydowns don't reach the parent from the sandbox — from the second shift-click on, the composer never yielded. The bridge pointer relay now carries the observed shiftKey (validated strict boolean) and arms/disarms yield directly. D4: a forged multi-target-removed desynced parent (promotes) from bridge (keeps original). applyTargetRemoval now ALWAYS echoes remove-target — idempotent for legit bridge-side removals, forcing convergence after forgery. D5: the 'Also applies to N more elements' block gains a leading blank line so markdown lazy continuation cannot fold it into the preceding blockquote. D6: the stray-key guard registers in capture phase (a bubbling global shortcut can no longer both fire and have its character appended), inserts at the textarea's remembered caret instead of end-of-text, and refocusToken now preserves the caret rather than jumping to the end. D7: the expanded dialog no longer carries the dead yield class/style — its data-comment-popover wrapper spans the viewport, making proximity meaningless; the dialog deliberately does not yield. Also reverts the incidental bun.lock version-catch-up churn. Tests: unarmed/stale-arm refusal, quickLabel non-arming and non-mirroring, newline-label collapse at both layers, bridge-shift-driven yield, forged removal echo + bridge-side idempotent resync, caret-preserving stray keys, blockquote separation. Signoff harness re-run green (14/14) on rules-ui-signoff.html including the arm handshake. * fix(annotate): reset multi-select arm on every new pinpoint draft (D1-R) The re-review caught that multiSelectArmed was never cleared when annotateElement started a fresh draft — only clearPendingPin reset it. So a comment-mode draft (armed) followed by a mode switch the parent doesn't mirror (quick label posts no arm) and a new pinpoint click left the stale arm live: the bridge accepted shift-clicks and pinned elements the saved annotation would never carry. Reset the flag at the top of annotateElement alongside clearMultiTargets. Regression test reproduces the exact sequence (armed draft -> new unarmed draft -> shift-click must not add a target); mutation-verified that removing only this reset fails it. * docs(annotate): correct the first-keystroke guard comment reasoning The comment claimed capture phase prevents a global shortcut from also firing; preventDefault does not stop the dispatcher (it ignores defaultPrevented by design). Restate the actual invariant: the guard is safe only because no bare printable single-key binding exists on this surface, and flag that as a constraint for future bindings. Comment only. |
||
|
Mehdi
|
696bf1f24e | fix(vim): preserve plan position on refocus (#1252) | ||
|
Michael Ramos
|
9ab5392bad |
feat(annotate): pinpoint-first raw-HTML sessions with element anchors and a minimal-first render (#1243)
* feat(annotate): rebuild HTML pinpoint mode with element anchors, pin badges, and a minimal-first render Raw-HTML annotate sessions now open as just the page: first-ever run hides all annotation chrome (toolstrip, tongue tabs, action cluster, sidebar) and the user's last chrome state is restored on every later session via the plannotator-html-chrome cookie. Pinpoint becomes the default input method on the HTML surface, persisted separately from the markdown preference. Pinpoint mechanics are rebuilt in the sandbox bridge, modeled on app-notes-extension and agentation: a fixed-position outline box + label replaces class writes on author elements, hover is identity-gated and re-hit-tested on scroll/resize via a rAF-coalesced reconcile pass, clicks pin the element (crosshair cursor, pinned outline while composing) and go straight to the comment popover. Each pin serializes a verified-unique CSS selector anchor (id > identity attrs > meaningful classes > nth-of-type path) with a text-snapshot fingerprint; restoration is anchor-first with fail-closed validation and falls back to document-wide text search. Element pins that cannot take an inline mark (SVG etc.) get numbered pin badges that track their element and re-acquire it after re-renders. All DOM inspection stays inside the bridge; only validated, size-capped DTOs cross postMessage. Annotation model is extended additively (htmlAnchor), so exported feedback, drafts, and share links keep working. * fix(annotate): harden HTML element anchors per adversarial review Four fixes from the pre-merge adversarial review of the pinpoint round: - Stable identity is now only the element's own #id or data-* rung, re-derived from the resolved element and compared whole instead of parsed out of the selector string. Behavioral attributes (role, href, aria-label, name, alt) no longer exempt an anchor from the text check, so a regenerated page can no longer bind an annotation to the wrong element. A weak anchor with a missing or empty snapshot is rejected, not exempted. - Restoration order: the document-wide text search now runs before the pin-badge fallback, so text that moved elsewhere in a regenerated page is followed rather than badging the stale container. Resolved SVG anchors still pin directly. - Selection text is capped at 10k chars on both sides of the bridge (truncated, not rejected); one pinpoint click on a huge pre/table no longer ships an unbounded page-controlled string into drafts, feedback, or share URLs. - buildAnchorSelector returns null instead of a terminal fallback path already proven non-unique; no anchor beats a known-ambiguous one. Also moves the HTML chrome restore out of the one-shot mount effect into a surface-transition effect, so a linked .html doc opened from a markdown session gets the minimal-first chrome and persists its state, and a markdown surface's sidebar use never leaks into the HTML cookie. * fix(annotate): skip the chrome save in the restore commit itself The reviewer's residual finding: htmlChromeRestoredRef flips synchronously inside the restore effect, but the restored state lands a commit later, so the save effect's run in the restore commit wrote pre-restore values over the remembered state (self-corrected next flush, but a page ending between the two writes would freeze the inverted value). The restore now arms a skip-one flag the writer consumes, so the stale write never happens. A follow-up save fires from the changed deps when the restore changed anything; when it changed nothing the cookie already holds those values. Regression test instruments every chrome cookie write on a returning-user mount and asserts none ever differs from the remembered state (mutation tested: removing the skip fails it). |
||
|
Raúl
|
183cae2c44 |
fix(vim): keep j/k cursor clear of HUD bands when scrolling (#1154)
Document Vim navigation moved the cursor with
`Element.scrollIntoView({ block: 'nearest' })`, which parks the target
flush against the nearest viewport edge — exactly where the sticky action
bar (top) and the key HUD / status pill (bottom) float. Motion to the top
or bottom of a document then hid the caret behind an overlay, while a
mouse wheel (which the browser lets overshoot) kept the same line nearer
centre.
Add vimScroll.ts: a pure computeVimScrollDelta returning the signed
scrollTop delta needed to clear a HUD band at each edge (0 when the
target is already inside the safe band; a target taller than the band
aligns to its top edge so reading order wins), resolveVimScrollMargin
sizing the fallback band as clamp(20% of viewport height, 24px..160px),
and a scrollVimTargetIntoView wrapper.
Both bands are measured from live geometry instead of guessed constants:
the top band widens past the ratio margin to clear the sticky action
bar, and the bottom band derives from the portaled key HUD / mode pill
rects ([data-vim-key-hud] / [data-vim-mode-badge]). The opt-in key HUD
(fixed bottom: 150, height: 88 — a ~238px band, past the 160px clamp) is
actually cleared, while the default pill reserves ~49px instead of
over-reserving 160px. An expanded key HUD is a deliberate modal state
that can outgrow the viewport, so it keeps the ratio margin.
The scrolling element is the native-scroll host fed through
ScrollViewportContext, so the wrapper takes that element from the caller
(useScrollViewport() in Viewer, the same node the reticle measures
against) and falls back to the historical scrollIntoView when it is
absent, so behaviour never regresses.
Route every cursor/target move in useVimSelection through it, and add
vimScroll.test.ts to the DOM allowlist in test.yml so its integration
tests run in CI.
|
||
|
Michael Ramos
|
ffd49080ee | fix(skills): harden skill references before first release (#1235) | ||
|
Michael Ramos
|
2edad89dfe | fix(editor): stop the skill-content priming effect from re-render looping (#1234) | ||
|
Michael Ramos
|
7ad4d39ed9 |
feat(comments): reference agent skills with / or $ in plan review and annotate comments (#1229)
* feat(comments): reference agent skills with / or $ in plan and annotate comments
Typing / or $ at the start of a word in the document-UI comment composer
opens a picker of the user's global agent skills (~/.claude/skills,
~/.codex/skills, ~/.agents skills roots), served by a new GET /api/skills
on the plan and annotate servers in both runtimes (Bun + Pi mirror).
Multiple references per comment are supported; references live in the
comment text itself and are appended to exported feedback as a
'Skills referenced' block so the acting agent knows which skills to apply.
Human-invocation-only skills (disable-model-invocation: true frontmatter)
stay listed and selectable but render dimmed with a badge, warn in the
menu and composer, and are marked in the export so the agent is never
asked to invoke something it cannot.
Discovery reuses the review-skill loader (same roots, precedence, and
skip-and-log discipline), reads only an 8KB head per SKILL.md, caps the
catalog at 500 skills, takes no client input, and is never persisted;
any failure degrades to plain typing.
* fix(comments): harden skill references per review (trigger, IME, seam, fail-closed frontmatter)
Blockers:
- B1: a trigger now requires at least one query character. A bare / or $
no longer opens the catalog, so Enter stays a newline and Tab still
leaves the field ("This costs $" + Enter, "cd /" + Tab, bullets).
- B2: the menu ignores keys mid-IME-composition (nativeEvent.isComposing),
matching the 16 existing guards; Enter committing a Pinyin/Telex/Korean
candidate can no longer insert a skill.
- B3: the catalog request is a host seam (skillCatalogTransport via
configurePlannotatorUI), defaulting to the existing GET /api/skills.
- B4: resetSkillCatalogCache() invalidates outstanding requests
(generation counter), and a late-resolving stale request can no longer
overwrite a newer cached value or the export registry. The catalog
tests reset in beforeEach, so they hold in any file order.
Also:
- F1: skillReferences={false} is fully inert — the human-only notice memo
and the cache seed are gated on the prop.
- F4: frontmatter flag parsing no longer fails open: trailing YAML
comments are stripped, on/1 (and TRUE/yes etc.) read as true, the head
read is 64KB, and truncated unterminated frontmatter fails CLOSED on
disable-model-invocation.
- F5: extraction ignores markdown link destinations ([x](/name)), shell
redirects (cat /x > out), and /-triggered FHS root names (/run, /tmp);
menu insertion switches / to $ for those names so inserted references
always survive extraction.
- F6: the 500-skill cap slices after sorting, so which skills survive no
longer depends on readdir order.
- F3: /api/skills wiring guards for the Bun and Pi plan + annotate
servers (skills-endpoint.test.ts).
- Keyboard state machine tests against the real CommentPopover in
happy-dom (bare trigger, insertion, composition, Escape, highlight
bounding, opt-out inertness), added to the CI DOM step.
- The insertion path dismisses the trigger start so the menu close is
ordering-safe against React's select-plugin re-reading a stale caret.
* feat(comments): redesign the skill reference menu (bare triggers, no preselection, highlighted tokens)
Per maintainer direction, reversing the earlier bare-trigger opt-out
deliberately: typing a bare / or $ at the start of a word now opens the
full skill catalog immediately, and the safety story moves from the
trigger to the menu itself.
No preselection (the load-bearing rule): the menu opens with NO row
active, and while nothing is active every key behaves exactly as if the
menu were closed. "This costs $" + Enter is a newline; "cd /" + Tab
leaves the field (the proven regression that must never return). A row
activates only via ArrowDown/ArrowUp (Down from none lands on the first
row, Up on the last); only then do Enter/Tab insert. Pointer hover never
activates a row, because the menu floats exactly where the mouse rests
over the composer; a click inserts directly and never arms Enter.
Continuing to type re-filters and disarms any active row. Escape clears
the active row and dismisses when the user engaged (query typed or row
active); an unengaged bare-trigger menu passes Escape through so closing
the composer still costs one press.
Menu redesign to the reference look: icon, bold name, dimmed inline
description with ellipsis, right-aligned source column (Agents / Claude
/ Codex from the discovery roots), rounded generously padded rows, and a
subtle active-row background; human-only rows stay dimmed with their
badge and the warning now shows while such a row is ACTIVE.
Inserted references render highlighted in the composer via a mirrored
aria-hidden overlay behind a transparent-text textarea (identical font,
padding and wrapping metrics; scroll synced; tokens change color and
background only, drawn from the --primary theme token so every palette
works in light and dark). The caret keeps --foreground, selection uses a
translucent primary wash, and IME composition temporarily restores
native textarea text so composition underlines render normally.
skillReferences={false} still renders the plain pre-feature textarea.
Also, per review:
- extraction: dropped the over-broad shell-redirect exclusion (false
negatives on prose like "use /animate <- this one"; the motivating
case stays covered by the reserved-path rule)
- frontmatter: an unterminated frontmatter block now fails CLOSED on
disable-model-invocation even in complete (untruncated) files
- the reserved-path / to $ insertion switch stays: extraction still
reads /run as a path, and the new token highlight makes the switch
self-explanatory (an unhighlighted insert would look broken)
The composition guard, transport seam, catalog generation counter,
enabled gating, and export rules are unchanged and re-covered by the
rewritten DOM test matrix.
* fix(comments): give the skill reference menu adaptive, viewport-clamped placement
The menu rendered bottom-full with a fixed max-h-64: always upward, up to
256px, with no viewport awareness. With the comment popover near the top of
the viewport (annotating near the top of a document), typing a trigger ran
the menu off the top of the screen with its upper rows unreachable.
Placement now mirrors the popover's own computePosition idiom: measure the
space above and below the composer wrapper against window.innerHeight,
prefer above (the shipped direction; keeps the action row and human-only
notice visible), flip below when the list fits below but not above, and when
neither side fits pick the roomier side. The list's max height is clamped to
the available space (still capped at the former 256px), so the menu never
extends past a viewport edge. Recomputes on every commit (drag moves,
popover flips, filtering changing the item count, warning-footer toggles)
plus capture-phase scroll and resize listeners, matching the popover's
tracking. Visual design of the menu and rows is unchanged.
* feat(comments): inject human-only skill instructions into exported feedback
A human-only skill (disable-model-invocation: true) referenced in a review
comment used to export as a dead name the agent could do nothing with. A
human referencing a human-only skill IS the human invocation, so the export
now injects the skill's SKILL.md body verbatim (frontmatter stripped) inside
clearly delimited BEGIN/END SKILL INSTRUCTIONS markers, with the absolute
skill directory and the resolve-relative-paths pointer so references/,
scripts/, and assets/ stay actionable. Model-invocable skills keep exporting
as names the agent can invoke itself.
Transport is lazy: a new GET /api/skills/content?name= endpoint (Bun and Pi)
serves one discovered skill's body, capped at 20k chars with an explicit
truncation notice pointing at the file; the client fetches contents only for
the human-only skills actually referenced, keyed off comment state, and the
catalog now carries each skill's absolute dir so every failure path (deleted
skill, unreadable file, race with submit) degrades to naming the skill plus
its directory. Names are matched against discovery only and never used as
paths, so traversal cannot escape the skill roots. A per-export dedupe
injects each skill once even when several comments reference it, and
GLOBAL_COMMENT annotations run through the same block.
The referenced-skills header now says the reviewer is asking for the
invocation, and the human-only menu footer and composer notice explain that
the skill's instructions will be included with the feedback instead of
warning that the reference will not work.
* polish(comments): quiet, progressive human-only skill treatment
The human-only surfaces shipped with too much emphasis: a dimmed row plus
a bordered uppercase badge, an amber warning footer, and a persistent
amber notice in the composer after insertion. Human-only is a property of
a skill, not an error state, so the treatment is now quiet and
progressively disclosed:
- Menu rows render at full strength with a small muted 'human-only' pill
(bg-muted / muted-foreground tokens; no border, no dimming).
- The plain-language explanation (a model cannot invoke it, so its
instructions will be included with your feedback) appears as a muted
footer only while a human-only row is active (keyboard) or hovered
(pointer). Hover disclosure is purely visual state local to the menu;
it never touches activeIndex, so the no-preselection invariant and the
hover-never-arms-Enter rule are unchanged and re-asserted by a new test.
- When not disclosed, the same sentence stays in the DOM sr-only and
human-only rows point at it with aria-describedby, so the state reaches
assistive tech as text rather than as a purely visual badge (this does
not attempt the #1233 combobox semantics, and does not worsen them).
- After insertion, the highlighted token itself carries the quiet inline
marker (a dotted primary underline; text-decoration cannot move glyphs,
so overlay alignment is untouched) and the standing amber notice is
replaced by a native <details> disclosure: a single muted 'Includes
skill instructions' summary line that expands to the full accurate
sentence, operable by pointer, keyboard, and AT alike.
No amber remains; every color is a theme token (muted, muted-foreground,
border, primary, ring), so the treatment follows every palette in light
and dark. Copy is unchanged where it was accurate. Behavior is unchanged:
human-only skills stay selectable and injection still happens.
* fix(comments): harden human-only skill injection per adversarial review
Three findings on the injection path, each with tests that fail pre-fix:
1. Marker forgery: an injected SKILL.md body containing our own
`--- BEGIN/END SKILL INSTRUCTIONS ---` markers (or an
`[Instructions truncated:` notice) could close the block early — making
everything after it read as the reviewer's own words — forge a block for
a skill nobody referenced, or forge a truncation notice pointing at an
attacker-chosen path. Body lines matching the structural marker forms
(leading-whitespace and case variants included) are now visibly
neutralized before injection: kept verbatim but prefixed, never silently
deleted (neutralizeSkillMarkerLines).
2. Forged human invocation: POST /api/external-annotations is
unauthenticated on localhost, so any local process could submit a
comment referencing a human-only skill and cause its instructions to be
injected "at the reviewer's request". Annotations carrying a `source`
now still LIST their skill references but never cause verbatim
injection — human-only references fall back to naming the skill plus
its directory, with an honest reason. The content-prime effect skips
external texts for the same reason. A human referencing a human-only
skill IS the human invocation; a tool is not.
3. Unbounded read: readReferenceSkillContent read the whole SKILL.md
before slicing to the 20k cap, so an unauthenticated no-cors fetch loop
could balloon RSS by file size per request (measured +64.4MB for a 64MB
file). It now uses the same bounded readFileHead as the catalog,
reading only frontmatter allowance + 4 bytes per capped char + slack;
truncation detection is unchanged for any file whose frontmatter fits
the catalog bound, and frontmatter that overflows the read falls back
to null rather than serving raw YAML. Measured: 12 reads of a 64MB
SKILL.md now cost +5.1MB total.
Also: the fast-fail guard no longer rejects legitimately discovered names —
`name.includes("..")` 404'd a real `v1..2` skill dir forever (and `\` is
legal in POSIX names) while defending nothing, since the name is only ever
matched against discovery output and never joined into a path. It now
rejects exactly the names that can never be a readdir entry: empty, `.`,
`..`.
|
||
|
Michael Ramos
|
7ba4e3b3e4 |
fix(review): mint content-derived diff cache keys so single-file tabs render fully (#1219)
* fix(review): mint content-derived diff cache keys so single-file tabs render fully Single-file diff tabs have not rendered their full-content diff since v0.26.0: the expansion gap bars show no chevrons and clicking them does nothing, at every file size. @pierre/diffs 1.3.2 (the 1.2.8 -> 1.3.2 bump, upstream "Fix diff rerender in edit mode (#878)") added name-based cacheKey defaulting in FileDiff.render: an unset `fileDiff.cacheKey` becomes the file's name. `areDiffTargetsEqual` — the only identity check its render and highlight caches make — compares nothing but that key. DiffViewer renders each file twice on one surviving FileDiff instance (key={filePath}): first the PARTIAL diff from getSingularPatch, then the AUGMENTED full-content diff from processFile once /api/file-content lands. Neither set a cacheKey, so both defaulted to the filename and Pierre served the stale partial render forever. Only the augmented diff is expandable, hence the dead gap bars. Both diffs now mint content-derived keys (`<path>#<hash>` and `<path>#full#<hash>`), matching how AllFilesCodeView already keys its items — which is why the all-files view was never affected. The hash (not patch.length) matters because Pierre's worker highlight cache is a singleton that outlives remounts. The partial diff needs its own key too: with key={filePath} the instance also survives diff-type and base switches, where a same-named new patch would otherwise hit the same name-keyed stale cache. hashString moves from AllFilesCodeView to utils/hashString.ts so both surfaces mint keys the same way. Covered by a new DOM test that mounts DiffViewer against the real @pierre/diffs renderer, holds the /api/file-content response until the non-expandable partial baseline is asserted, then requires the expansion affordances to reach the pixels. It fails against the unfixed tree. * fix(review): explain why an oversized file's card has no diff Files over the 5 MB review limit are replaced by a contents-free stub (buildOversizedTrackedStub, plus the untracked equivalent), which renders as a header-only card with no counts and no explanation. Users read that as a broken diff. The stub now carries an explicit marker line in its extended header (OVERSIZED_REVIEW_STUB_MARKER). A marker rather than a client heuristic because the only other signal, `Binary files ... differ`, is exactly what a genuine binary file emits, so a heuristic would put a false size-cap explanation on every image in the diff. The marker lives in shared/diff-paths so the browser bundle can detect it without pulling in the node-facing review core; both server runtimes pick it up from review-core, which vendor.sh already copies to Pi. Git ignores unknown extended-header lines and @pierre/diffs parses the stub identically with or without it, so nothing else moves. Which files get stubbed is unchanged. Both review surfaces now render one line under the file header saying the file is over the limit and only a stub is shown. * test(review): make the diff-swap proof machine independent, not stopwatch based CI failed two tests that pass locally. Both were timing races, neither was an app bug. 1. DiffViewer.fullContentSwap: the swap assertion carried a 15s internal wall-clock budget, which a cold, contended CI runner blows and a warm laptop clears. Two changes, both aimed at the clock rather than the symptom: - The waits are now budgeted in SCHEDULER TURNS, not milliseconds. A slower box spends longer inside each turn but needs no more of them, so the budget never has to be retuned for CI hardware. - Pierre's shared Shiki highlighter is preloaded before mounting. It is a module singleton, and building it was the entire multi-second cost the old budget was accidentally measuring; warming it moves that work into an unbounded await OUTSIDE the observed window. Disposed in afterAll, because packages/ui/utils/codeHighlight.test.ts asserts the pre-attachment behaviour of that same singleton. Verified against an artificially stalled clock: forcing 20s of dead time into every wait (41s total, far past the old 15s budget) still passes, and with the cacheKey fix removed it still fails on the assertion (not as an opaque timeout) in ~12s. A 20-turn budget with the preload removed and every core saturated also passed 10/10, so 400 turns is a wide margin rather than a guess. 2. App.archiveReadOnly compared the fenced block's innerHTML before and after a click. Since #1218, applyHighlight writes plain text first and swaps in Shiki markup when the grammar attaches, so that MARKUP changes on its own schedule and the assertion was racing the swap. The test is checking that the click opened no mutation entry point, which textContent plus the absence of an annotation <mark> says exactly, and which no highlight swap can perturb. Latent on main; the branch's run happened to lose the race. Also fixed while confirming the above: codeHighlight.test.ts asserted a GLOBAL precondition ("no grammar attached yet") that any earlier file attaching a typescript fence invalidates, so `DOM_TESTS=1 bun test packages/ui packages/editor` failed by file order alone. It now resets the attachment cache through the existing __resetCodeHighlightCacheForTests seam and asserts the contract instead of the run order. Not currently reachable from CI (that file is not in the DOM list), but one list edit away. * test(review): drop the highlighter preload, harden the swap proof, report why a paint is missing The preload added in the previous commit made CI strictly worse, so it is gone. Before it, CI's partial diff painted and only the swap was missing; with it, CI never painted at all. It was an optimization for a theory the evidence has since killed, and it mutated a process-wide singleton to buy it, so it is not worth keeping while the real failure is unexplained. The afterAll dispose that existed only to undo the preload goes with it. What the CI log actually shows: - The "WorkerPoolManager: operation canceled because the pool terminated" error is inside discardRestoreRender.test.tsx's own group, ~0.3s BEFORE this file's group opens. It is that file's provider unmounting and terminating the pool singleton it created: end-of-file teardown, the same benign noise documented on #1209. It also prints on every local run, where the whole list passes. It is not a mid-test terminator, and nothing in this file uses the worker pool (no WorkerPoolContextProvider is mounted, so useWorkerPool() is undefined and rendering takes the main-thread path). - This file's group prints NOTHING for its whole 10.3s: no console.warn from the stale-content guard, no error. Pierre simply painted nothing. Not reproducible locally: the exact DOM list from test.yml, one bun process, forward and reverse order, 13 runs with every core saturated, all green. So the remaining difference is the environment, which cannot be reasoned out from here. Three changes make the next CI run answer it instead of costing another guess: - renderDiagnostics() dumps what Pierre actually painted (container / separator / chevron / line-number counts plus a markup fragment) when a wait gives up. Prints only on failure, so it is worth keeping. - The precondition is asserted rather than assumed: the REAL getSingularPatch and processFile must produce partial-then-full on these fixtures. Bun's mock.module is process global and an earlier file in this very list mocks '@pierre/diffs', so a leaked mock now fails in milliseconds with a clear message instead of as a render that never arrives. - The first paint is now REPORTED, not asserted. The verdict belongs to the swap; gating on the partial paint let a slow or absent first paint mask the result the test exists for. Removing the cacheKey fix still fails it (verified), because that tree paints no chevrons at any point. Also fixed a real trap in the fixture: the hunk header said @@ -61 while its context lines start at line 59 of both contents. Pierre realigns a misaligned header rather than rejecting it, so it was silently tolerated. * test(review): stop a leaked module mock from silently unrendering the diff tests Root cause, and it was never a timing problem. AllFilesCodeView.lifecycle.test.tsx calls `mock.module('@pierre/diffs', ...)` with a hunk-less `getSingularPatch` and `processFile: () => null`. Bun's module mocks are process global and are not unwound at file boundaries, and that file sits immediately before DiffViewer.fullContentSwap.test.tsx in the DOM step's list. On the Linux runner the stub reached this file; on macOS it did not, which is why 13 local runs of the exact list, both orders, cores saturated, stayed green. It explains both CI symptoms exactly, including the one that looked like a contradiction: `processFile: () => null` means the augmented diff never exists, so no chevrons ever (the failure before the preload); the stub `getSingularPatch` has `hunks: []`, so nothing paints at all (the failure after it). The "WorkerPoolManager: operation canceled because the pool terminated" line was a red herring throughout: it is inside discardRestoreRender's own group, ~0.3s BEFORE this file's group opens, is that file's provider unmounting the pool it created, and prints on every local run too. The precondition assertion added in the previous commit is what proved it, turning a 10.3s mystery into a 0.45ms verdict: 228 | expect(expected?.isPartial).toBe(false); error: expect(received).toBe(expected) Expected: false Received: undefined Fixed at both ends: - Source: the mocking file now captures the real modules before it stubs them and restores both library specifiers in afterAll, so no later file in any run inherits its stubs. This fixes the class for every future DOM test that needs the real renderer, which was the actual leak. - Consumer: the two tests that render against the real @pierre/diffs get their own CI step, the same isolation (and for the same kind of reason) this workflow already gives useFileBrowser.test.tsx. They are removed from the shared list so that step is their single source of truth. The restore above should make this unnecessary; it is not something to bet a green build on from a machine that cannot reproduce the platform behaviour. Verified with a CI-faithful harness: one bun process per step, the exact lists from test.yml, isolated + shared-forward + shared-reverse, 10 iterations with every core saturated, then 6 more after the final split. All green, plus the full suite and typecheck. * docs(test): point the diff-renderer DOM tests at the CI step that actually runs them |
||
|
Michael Ramos
|
c08b188812 |
perf(ui): single Shiki highlighter, palette-matched code blocks, drop highlight.js (#1218)
* perf(build): stub out the dead Oniguruma WASM in every bundle
@pierre/diffs picks its Shiki engine with a runtime ternary:
engine: preferredHighlighter === "shiki-wasm"
? createOnigurumaEngine(import("shiki/wasm"))
: createJavaScriptRegexEngine()
Plannotator pins `preferredHighlighter: 'shiki-js'` (and Pierre's own
default is 'shiki-js'), so the Oniguruma branch never executes. Because
the choice is a runtime ternary, bundlers keep the `import("shiki/wasm")`
edge anyway and inline `@shikijs/engine-oniguruma/wasm-inlined`, a
~622 KB base64 blob, into the single-file HTML builds. The review app
paid for it twice: once on the main thread (via
`highlighter/shared_highlighter.js`) and once inside the `?worker&inline`
Pierre worker.
Alias `shiki/wasm` to a stub that throws if it is ever reached. Wired via
`resolve.alias` rather than a plugin because `resolve.alias` is shared
with Vite's worker build and `plugins` are not.
Highlighting output is unchanged: the JS regex engine was already the one
doing the work. Opting back into 'shiki-wasm' now fails loudly instead of
silently costing every user a megabyte of dead bytes.
apps/review/dist/index.html 19,424,646 -> 18,180,545 (-1,244,101 raw / -463,348 gzip)
apps/hook/dist/index.html 23,032,467 -> 22,410,416 (-622,051 raw / -233,485 gzip)
* perf(ui): consolidate code highlighting onto Shiki, drop highlight.js
The app shipped two highlighters. Shiki already tokenised the code-review
diff pane (via @pierre/diffs, JavaScript regex engine); highlight.js
separately coloured markdown fences and review suggestion snippets at
~982 KB minified for a full build of ~190 grammars. That second
highlighter is now gone.
Every call site moves onto `packages/ui/utils/codeHighlight.ts`, a thin
wrapper over Pierre's SHARED Shiki instance:
CodeBlock, Viewer, PlanCleanDiffView markdown fences
InlineMarkdown code-file hover preview
HighlightedCode review suggestion snippets
Reusing Pierre's instance rather than standing up a second fine-grained
one is deliberate. Pierre imports Shiki's full bundle, so every grammar
and theme is ALREADY inlined in the single-file builds: a separate
highlighter with a curated language list would have duplicated a subset
of bytes that are already there. Sharing costs nothing, gives every
language Shiki bundles instead of a shortlist, and — the point of the
change — guarantees fences resolve the exact same theme the diff pane
resolves.
Theming. `SHIKI_THEME_MAP` / `resolveSyntaxTheme` move from
`packages/review-editor/hooks/usePierreTheme.ts` to
`packages/ui/utils/syntaxTheme.ts`; usePierreTheme re-exports them, so
the review editor's imports are unchanged. `useFenceTheme()` feeds the
components and re-highlights on palette or mode change. Code blocks now
follow the active palette across all ~52 themes in both light and dark,
instead of always rendering github-dark and relying on hand-written
`.hljs-*` override stacks to stay legible. Those stacks are deleted:
`packages/editor/index.css`'s light-mode token palette, and
`colorblind.css`'s hand-tuned tokens which existed to APPROXIMATE
@pierre/theme's protanopia-deuteranopia themes that are now simply used.
Behaviour held fixed:
- Language-less fences stay plain text (#1212). No auto-detection
anywhere, including the hover preview, which previously called
`hljs.highlightAuto`. `HighlightedCode` derives its language from
the caller's file path; an unknown extension renders plain.
- `applyHighlight(el, ...)` keeps the imperative `hljs.highlightElement`
DOM contract the annotation layer reaches into, and writes plain text
at final size first so async highlighting causes no layout shift.
Already-attached grammars highlight synchronously — no flicker on
cached highlights.
- It also verifies the rendered text is byte-identical to the source
and falls back to plain otherwise, because annotations address code
blocks by text offset.
- `@plannotator/ui`'s public API is unchanged: the highlighter is a
module-level default like the package's other seams, no new props.
The `hljs` class on fenced `<code>` becomes `pn-code` (it is a
structural hook for blockTargeting, vim navigation and print.css, and it
named a library we no longer ship). `language-*` stays.
apps/review/dist/index.html 18,180,545 -> 17,270,889 (-909,656 raw / -291,921 gzip)
apps/hook/dist/index.html 22,410,416 -> 21,704,434 (-705,982 raw / -238,096 gzip)
Verified the diff pane is untouched: the rendered Pierre shadow-DOM
markup is byte-for-byte identical between an origin/main build and this
one (SHA-256 aa1ee88a…).
* fix(ui): strip stray NUL bytes from the code-highlight source
Two U+0000 bytes slipped into comments in the previous commit, which made
git treat the file as binary. Replaced with spaces; no behaviour change.
* fix(ui): keep code-block annotation marks across highlight swaps
Fenced code is annotated by hand: one `<mark data-bind-id>` inside the
`<code>` element, which `applyHighlight` also owns. Every highlight swap
(palette change, dark/light toggle, or the first async grammar attach
after load) replaces that element's children, so the mark was silently
wiped and nothing put it back. Annotation state, the sidebar panel and
exports were unaffected; the loss was purely visual, and deterministic.
`applyHighlight` now publishes every write through `onCodeHighlightSwap`,
synchronously, immediately after it. `Viewer` subscribes and re-paints the
fence's mark, so a swapped block ends up with BOTH the new theme's tokens
and its annotation. The shared painter (`paintCodeBlockMark`) moves the
token spans into the mark instead of flattening them to text, so creating
an annotation no longer costs a block its colours either.
Being driven by the swap also fixes the cousin race by ordering rather
than timing: share/draft restore runs on a timer after load, and on a slow
machine the first async swap could land after it and wipe the restored
marks per block. A restore that painted before the swap is now
re-established in the same task the swap ran in, and one that runs after
finds the mark already there.
Removal tombstones the id before re-highlighting, because the host drops
the annotation from state a tick later — without it the swap listener
would paint the just-removed annotation back in, and a fence carrying a
second annotation would end up bare.
Also closes the named gap in the WASM coverage: entry-assets only grepped
source, so a future @pierre/diffs bump could reintroduce the inlined blob
through a different import specifier unnoticed. It now greps the built
`apps/{review,hook}/dist/index.html` for the base64 WASM magic, skipping
on an unbuilt checkout and running for real in the CI job that builds the
bundles.
|
||
|
Michael Ramos
|
2d65c65596 |
feat(ui): pair a light theme and a dark theme, switched by mode (#1217)
* feat(ui): pair a light theme and a dark theme, switched by mode
ThemeProvider stored one palette plus a mode, so picking a dark-only
palette pinned the mode and greyed out the Light/System buttons. Store a
pair instead: { mode, light, dark }, resolved as pair[preferredMode], so
System flips between the two choices as the OS scheme changes.
The Settings Theme tab now assigns one half at a time. A Light/Dark
switch decides which half the grid is filling, the grid lists only the
palettes that can render that half (from the registry's modeSupport), and
a summary line names both halves with each side clickable. Every mode
button is permanently enabled: a dark-only palette simply never occupies
the light slot, so no mode coercion is left to do.
The pair round-trips through the SETTINGS registry to the `theme` key in
~/.plannotator/config.json the way diffOptions does. A user upgrading
seeds both halves from their stored single palette, and the legacy
plannotator-color-theme key keeps tracking the active palette so a
downgrade never lands on an unstyled first frame.
Addresses part 1 of #1211.
* fix(ui): make the theme pair seed local, and keep the legacy API non-destructive
Review of #1217 found a data-loss path and three published-API regressions.
Seeding: ThemeProvider handed its resolved pair to the config store through
set(), which queues a debounced POST. configStore.init() applies the server
config but never cancelled that queued write, so a single cookie-less visit
(fresh profile, incognito, cleared cookies) flushed a default pair to
~/.plannotator/config.json AFTER the real one had arrived, and the next
session restored those defaults over the user's cookies. The provider now
uses a new configStore.seed(): memory plus cookie, never the server, and
never over a value init() already applied. init() additionally retracts
queued writes for the leaves the server just spoke for, which closes the
same race for every server-synced setting rather than this one key.
Deprecated APIs: isThemeModeAvailable() and normalizeThemeMode() are back as
one-line wrappers with @deprecated notes, since packages/ui exports utils/*.
setColorTheme: assigns exactly one half and nothing else. A both-mode palette
goes to the half on screen instead of clobbering both; a mode-restricted one
goes to its half without yanking a System user to an explicit mode (render
time already resolves that). It persists through configStore.setLocal(), so
it stays cookie-only as it was before the pair, unless a host installed its
own serverSync transport.
storageKey / colorThemeStorageKey are honored on the read path, so a host's
stored pre-pair preference is migrated rather than discarded. The two halves
have no pre-pair equivalent and stay on fixed keys, documented on the props.
Tests: a fresh-mount case that pins zero POSTs (the previous helper pre-seeded
cookies, which is why this was invisible), a case that pins a real choice
still reaching config.json, direct setColorTheme cases for all three
semantics, a host-storage-keys migration case, and configStore seed/retract
unit tests. All of them fail against the code they replace.
|
||
|
Michael Ramos
|
c4acec988c |
feat(ui): add colorblind theme (#1192)
* feat(ui): add colorblind and tritanopia themes Two new built-in themes tuned for color vision deficiency: - Colorblind: for red-green CVD (protanopia and deuteranopia). Diff additions are blue (#4f83d1 dark / #216cab light) and deletions are orange (#c96e12 dark / #a55c1e light), a pair on the blue-yellow axis that survives red-green dichromacy. - Colorblind Tritanopia: for blue-yellow CVD. Additions are teal (#2b939e dark / #1b7981 light) and deletions are vermilion (#db5f38 dark / #c14f2b light), a pair on the surviving red-green axis. Both pairs were verified computationally with Machado et al. (2009) severity-1.0 CVD simulation matrices: the simulated add/del line backgrounds keep CIEDE2000 >= 8.9 (red-green theme) and >= 18.6 (tritanopia theme) at every diffLineBgIntensity in both dark and light modes, and normal-vision foreground contrast stays >= 4.7:1 over the strongest tint. The +/- gutter signs and indicator bars reuse the same base colors and keep >= 4.2:1 contrast on the page background. The review editor's Pierre diff pipeline picks up the pair via the library's own override seam: the theme files set --diffs-addition-color-override / --diffs-deletion-color-override, which inherit into the diff shadow DOM, so no app code changes were needed for the line backgrounds. Syntax colors in diffs map to the @pierre/theme colorblind shiki themes (already in the dependency tree), and plan-editor highlight.js code blocks get theme-scoped token overrides mirroring the same palettes. * feat(ui): drop the tritanopia theme after adversarial review Independent re-simulation showed the shipped default diff pair (the pierre-dark/pierre-light gitDecoration colors, green #07c480 / red #ff2e3f in dark and #18a46c / #d52c36 in light) beats the teal/vermilion pair under tritanopia simulation, and a matched-pair grid search over green/yellow-green vs red/vermilion candidates confirms why: tritanopia preserves the red-green axis, so the stock green/red palette is already near-optimal for tritanopes. No candidate pair that also keeps 4.5:1 normal-vision text contrast over the strongest line tint can beat the default in all six mode/intensity configurations under both Machado severity-1.0 and Vienot 1999 (the best candidates converge on the default's own vivid green/red and still fall 3 to 6 dE00 short in the worst configuration). Shipping a theme named for tritanopia that at best ties the default would invite false confidence, so it is removed rather than renamed. The Colorblind theme (red-green CVD, the common ~95% of cases) is unaffected and also holds up under tritanopia simulation (Vienot dE00 >= 32.8 in every configuration). |
||
|
Raúl
|
9389a8c543 |
feat(ui): render markdown reference links (#1168)
* feat(ui): render markdown reference links The simplified markdown parser only understood inline links `[text](url)`, so CommonMark reference links rendered as raw text: `[text][id]` and the `[id]: url` definition both showed literally (#923). Add `resolveReferenceLinks`, a pure pass run at the top of `parseMarkdownToBlocks` that rewrites full (`[text][id]`), collapsed (`[text][]`), and shortcut (`[text]`) references, plus their image forms, into inline `[text](url)` links, so the existing inline renderer draws them. Link reference definitions are collected first (first definition wins, labels matched case-insensitively with collapsed whitespace, `<url>` and quoted-title forms supported) and then blanked in place, so a definition never renders and every block keeps its original source line number. Resolution is code-aware: references and definitions inside fenced code blocks and inline code spans are left verbatim, a shortcut is skipped when an inline `(...)` destination follows it or when it is a task-list checkbox marker at the start of a list item, and an unknown reference stays literal so bracketed prose like `[TODO]` or `[0]` never becomes a false link. A definition-shaped line is only collected when it can start a block (after a blank line, a code fence, another definition, or the document start), so a `[word]: token` line that continues a paragraph is left as text rather than deleted (CommonMark: a definition cannot interrupt a paragraph). A document with no definitions is returned unchanged. * fix(ui): protect code/HTML/footnotes and quadratic risk in reference-link resolution Owner review round for reference-style link resolution (#923): - Fence detection now mirrors the block parser's own naive rule exactly (full .trim() + startsWith('```'), any indentation, backtick-only — no ~~~ support) instead of a looser 0-3-space approximation, so indented and list-nested fences the block parser treats as code can never be rewritten. Aligns tilde-fence behavior the same way: since the block parser has no ~~~ support, the resolver no longer protects ~~~ blocks either. - Raw HTML blocks (<details>, <pre>, etc.) are now protected using the same HTML_BLOCK_TAGS/HTML_BLOCK_OPEN_RE/VOID_HTML_TAGS the block parser itself uses, with the same three termination rules (blank-line, void single-line, balanced-depth). - GFM footnote definitions ([^label]: ...) are excluded from collection entirely, so they and their [^label] references are never rewritten into inline links. - A definition-shaped line is now only blanked when its label was actually consumed by a resolved reference outside a protected region. Unused definitions, and definitions referenced only from inside code/HTML, stay visible. This also fixes a plan-diff bug: a URL-only edit to a definition line used to blank to nothing on both sides of the diff (a real change rendering as empty); now the isolated diff chunk keeps the definition visible and diffs normally. - CRLF lines are now recognized (definition regex tolerates a trailing \r) and preserved (a blanked line keeps its own \r). - Bound the label/text capture groups (999 chars, CommonMark's own label limit) and the code-span alternative (5000 chars) so a long run of unmatched brackets/backticks can no longer cause quadratic backtracking within the 2MB annotate cap; added a defense-in-depth cap on the number of definitions tracked per document. - Added coverage for nested brackets, backslash-escaped brackets, parenthesized destinations, idempotence, and confirmed dangerous destinations still flow through the existing sanitizeLinkUrl path unchanged. Added a migration-caveat note to the existing annotation-anchor section of packages/ui/HANDOFF.md: documents using reference-style links render differently now, which can shift position-based anchors captured before a host upgrades past this change. * fix(ui): bound HTML-block extent scan to kill quadratic unclosed-opener case markProtectedLines and parseMarkdownToBlocks each independently scanned line-by-line from a multi-line HTML opener until its balanced open/close depth returned to zero, giving up only at end-of-document. That scan never advanced the outer index on failure, so a document with many consecutive unclosed openers (e.g. thousands of bare <div> lines with no </div> anywhere) made every one of them re-run the same O(N) tail scan — O(N^2) total, a real hazard well within the 2MB annotate cap. Extract the scan into one shared helper, findHtmlBlockEnd, used by both call sites so they can't drift apart: - closeExistsFromLine lazily builds (once per tag name, cached per document) a suffix array answering whether a closing tag exists at or after a given line, so an opener that can never close is rejected in O(1) instead of scanning to EOF. - MAX_HTML_BLOCK_SCAN_LINES bounds the residual case (a closing tag exists far away but depth never actually reaches zero before it) to a constant amount of work per start position — a documented, safe degradation: a block whose true close sits beyond the cap is treated as unclosed, identically to today's 'no close ever found' case. Added a failing-before-fix perf test (many unclosed <div> lines took ~2.3-3.4s and blew a 800ms bound; now ~12-15ms) for both parseMarkdownToBlocks and resolveReferenceLinks, plus a parity test proving a real <details>...</details> block stays intact and identically protected/parsed among thousands of decoy unclosed <div> lines. * fix(ui): remove HTML-block scan cap that truncated valid long blocks MAX_HTML_BLOCK_SCAN_LINES (2000) fixed the O(N^2) unclosed-opener case but as a side effect also truncated genuinely valid, longer HTML blocks: a <details> or raw <table> block whose closing tag sits beyond 2000 lines got cut off mid-block, with its remaining content and the real closing tag falling through as separate, incorrect blocks. closeExistsFromLine already rejects an opener that can never close in O(1) (no closing tag anywhere in the document) without scanning a single line — that already eliminates the pathological 'many failing scans' case on its own. A cap on top of that only ever hurt the opposite case: a scan that DOES succeed, which happens once per document and costs O(L) for an L-line block exactly like reading any other block's content once. So the cap bought nothing further and could silently corrupt valid parsing for any block longer than it, however generous its value. Removed it; the scan now runs unbounded to its real end once closeExistsFromLine confirms a close exists at all. findHtmlBlockEnd is still the single shared implementation used by both markProtectedLines and parseMarkdownToBlocks, so both stay in parity. Added regression tests: a >2000-line <details> and a >2000-line raw <table> block each stay one whole html block (previously truncated); a link definition inside a >2000-line <details> block stays protected and never wins over a real definition outside it; a valid long <details> block survives even preceded by thousands of unclosed <div> decoys. Re-verified the 40k-unclosed-opener perf/parity case (already handled by closeExistsFromLine alone) stays fast and unaffected. * fix(ui): replace HTML-block close scan with a linear prefix-sum index Removing the fixed line-count cap fixed truncation of valid long HTML blocks, but reopened a closely related O(N^2) case: N unclosed <div> openers followed by a single trailing </div> all still pass the 'does a close exist anywhere' pre-check, so every one of them independently scanned forward (mostly to end-of-document) before giving up. Measured before this fix: 5000 openers ~1.0s, 10000 ~4.1s, 40000 timed out past 69s. Replaced the scan entirely with a per-tag-name prefix-sum index (buildTagCloseIndex): the running open-minus-close count for a tag name, plus a classic 'next element at or below this one' index over that prefix sum (an O(N) monotonic-stack construction, each position pushed/popped at most once). Finding where (if anywhere) a block starting at a given line closes is exactly that classic query, so it is now an O(1) lookup with zero scanning per opener, whether the block never closes, closes after 3 lines, or closes 3000 lines away. Both markProtectedLines and parseMarkdownToBlocks still share the single findHtmlBlockEnd implementation, so they stay in parity. 40000 unclosed <div> openers + one trailing </div> now resolve in ~30-45ms (parser and resolver both), with block-boundary parity between them. Re-verified: no truncating cap reintroduced (>2000-line <details>/<table> blocks still stay whole), nested same-tag blocks still balance on true depth (not just tag presence), and independent tag types (e.g. a <table> nested inside a <details>) don't cross-talk between their separate per-tag indices. |
||
|
Michael Ramos
|
463f6ed57c |
fix(ui): fall back to legacy copy in insecure browser contexts (#1174)
* fix(ui): fall back to legacy copy in insecure browser contexts navigator.clipboard only exists in secure contexts. Remote mode serves plain HTTP on a non-localhost host, so every bare navigator.clipboard.writeText call threw TypeError and copy buttons silently broke. Add copyTextToClipboard(text): Promise<boolean> to packages/ui/utils/clipboard.ts: it tries the async Clipboard API (guarded against synchronous throws), falls back to the existing copy-event plus execCommand path, reports success as a boolean, and never throws. copyTextWithFallback now returns whether the copy happened and accepts an optional focusOwner; copyTextPreservingFocus keeps its exported signature and behavior unchanged. Route all bare call sites through the helper, preserving each site's UX: Copied states only flip on success, error toasts and console errors remain for the failure case, fire-and-forget sites stay fire-and-forget. GoalSetupSurface gains the fallback and keeps its error surface for the all-strategies-failed case. Add DOM-gated unit tests for the helper and register them in CI. Closes #1173 * fix(ui): address clipboard fallback review findings Review follow-ups for the insecure-context clipboard fallback: 1. Tag the fallback textarea with data-clipboard-fallback and whitelist it in PopoutDialog's ANNOTATION_SELECTORS so the transient focus shift during a fallback copy no longer closes popout dialogs (TablePopout copy buttons, CodeFilePopout copy contents). 2. Widen AnnotationPanel's onQuickCopy prop to Promise<void | boolean>. A false resolution now suppresses the Copied flash; void resolution stays success so existing hosts keep today's behavior. The editor quick-copy site returns the helper's boolean. 3. In copyTextWithFallback, only flag the copy-event path as success when clipboardData was present and setData actually ran, and only when execCommand also reported success. A null clipboardData no longer calls preventDefault, so the textarea retry still runs. 4. Add tests pinning that the fallback runs synchronously when navigator.clipboard is absent (execCommand fires before the call returns, keeping it inside the user-gesture window), that a copy event without clipboardData is not treated as success, and that the fallback textarea carries the PopoutDialog focus-out marker and is removed after the copy resolves. 5. GoalSetupSurface surfaces the real writeText rejection message when the Clipboard API exists but fails and the fallback also fails; the generic unavailable message is reserved for the API-absent case. |
||
|
Michael Ramos
|
47157e7a55 |
feat(editor): add Vim keyboard annotation controls and live HUD (#1127)
* feat(editor): add Vim keyboard annotation controls * feat(ui): add optional live Vim HUD * feat(ui): finish Vim HUD experience * feat(ui): promote Vim to dedicated settings panel * feat(ui): make Vim document focus automatic * feat(ui): let Vim HUD hide its key panel * fix(ui): harden Vim selection UX |
||
|
Brad Beebe
|
ac3a84ba5a |
fix(opencode): Fix hardcoding default build OpenCode agent when sending responses (#1131)
* fix(opencode): default agent switching to disabled
* fix(opencode): keep plan-approval build handoff; default no-switch for review feedback only
The agent switch cookie is shared by plan approval and code review, so
flipping the stored default to `disabled` also removed OpenCode's
plan-approval hand-off for every user who never configured the setting.
Make the unset default surface-aware instead: `getAgentSwitchSettings('plan')`
keeps the historical build hand-off, `getAgentSwitchSettings('review')`
stays on the current agent. An explicit user choice still applies to both
surfaces. Settings and the agent warning resolve the default from the mode
they render in, and the OpenCode "agent not available" warning now names
plan approval on the plan path.
Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS
---------
Co-authored-by: Michael Ramos <mdramos8@gmail.com>
|
||
|
Ruaridh Williamson
|
6297358994 |
fix(ui): parse YAML block scalars in frontmatter (#1101)
* fix(ui): parse YAML block scalars in frontmatter Folded (>) and literal (|) values were stored as the bare indicator (e.g. ">-") with their body dropped, so multi-line skill descriptions rendered empty in the plan/annotate viewer. * fix: strip trailing CR from block scalar body lines (CRLF sources) --------- Co-authored-by: Michael Ramos <mdramos8@gmail.com> |
||
|
Michael Ramos
|
f9a6c1e39d |
feat: annotate accepts YAML, JSON, TOML and other plain-text files (#1099)
* feat(annotate): accept common plain-text config formats (.yaml, .json, .toml, …) Annotate previously rejected every file that wasn't .md/.mdx/.txt (or .html/.htm), even though the pipeline reads files as UTF-8 text and renders anything. Widen the accepted set to unambiguously plain-text config/data formats: .yaml .yml .json .jsonc .json5 .toml .ini .cfg .conf .properties .csv .tsv .log .xml .env.example. They render exactly the way .txt renders today. - New single source of truth: packages/core/annotatable.ts (ANNOTATABLE_TEXT_REGEX / ANNOTATABLE_DOC_REGEX + predicates), re-exported through @plannotator/shared/resolve-file and vendored into the Pi extension. - .env stays excluded (commonly holds secrets; annotate history copies file contents into the data dir). Source-code extensions stay with code review. - Single-file accept + bare-filename fuzzy search widen in resolveMarkdownFile; folder discovery and the file-browser listing widen in all three runtimes (hook CLI, OpenCode, Pi). - /api/doc gains a `doc=1` param set by the file browser so extensions that overlap CODE_FILE_REGEX (.yaml/.json/.toml/.ini/.xml) render as annotatable documents there while code-file links inside documents keep the syntax-highlighted popout. - Error messages now list the wider set; docs updated (AGENTS.md, marketing annotate page). Closes #1029 Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk * fix(annotate): frontmatter, size caps, edit-guard, and skill docs from review Review fixes for #1099: - Frontmatter: `--- … ---` stripping is a markdown convention; for non-markdown annotatable sources (multi-document YAML, .txt starting with ---) the delimiters are real content. parseMarkdownToBlocks gains a { frontmatter } option and the editor keys it off the active document's path via shouldStripFrontmatter() (strip for .md/.mdx and pathless/converted sources; keep raw for other annotatable text). - Size caps: new shared MAX_ANNOTATABLE_FILE_BYTES (2MB — same limit the code-file popout always had) now guards the annotate CLI single-file read in all three runtimes and the /api/doc document branches in both servers. Also applies to .md/.txt (behavior change for pathological inputs; previously unbounded). - Editing guard: mid-edit file opens gate on isSourceSaveFilePath (.md/.mdx/.txt) instead of the wider annotatable set — config files are view-only, so switching to one mid-edit no longer silently downgrades "Done editing" to feedback-only edits. - Skill docs: plannotator-annotate SKILL.md (core + Kiro) now mention the plain-text config formats. Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk |
||
|
Michael Ramos
|
6fbf0ef560 |
fix: align Codex model catalog and reasoning efforts with the current Codex CLI (#1096)
* fix(agents): align Codex model catalog and reasoning efforts with Codex CLI 0.144 - Remove gpt-5.3-codex from the Codex model picker: saved picks of it are already force-migrated to the fallback (ChatGPT-account Codex rejects it), so offering it was incoherent. The 5.2/5.1 family stays — those models remain available to API-key Codex users. - Per-model reasoning-effort gating: the catalog (now in packages/ui/utils/codexModels.ts, re-exported from AgentsTab) carries each model's supported efforts and default effort from the CLI catalog. The reasoning pickers only offer the selected model's efforts, including the new max/ultra tiers on the GPT-5.6 family, and useAgentSettings clamps an unsupported saved effort to the model's catalog default before it reaches the pickers or a launch payload. - Drop `minimal` everywhere: no current Codex model supports it. Saved perModel reasoning entries migrate minimal -> low, and the guide repair launch no longer hardcodes it. - Ask AI: bump the codex-app-server static fallback model and DEFAULT_MODEL from gpt-5.4 to gpt-5.6-sol (runtime model/list discovery still replaces the fallback and already carries per-model efforts; added Max/Ultra labels). Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk * fix(agents): prune API-shut-down Codex models per OpenAI deprecations OpenAI's API deprecations page schedules an API-level shutdown on 2026-07-23 for gpt-5.2-codex (-> gpt-5.5), gpt-5.1-codex-max (-> gpt-5.5), and gpt-5.1-codex-mini (-> gpt-5.4-mini) — dead for ALL auth modes, not just ChatGPT accounts — so they leave the picker and saved picks migrate: the first two to the surface fallback (already gpt-5.5), codex-mini to gpt-5.4-mini with its perModel preference carried under the new key (same pattern as the gpt-5.6 -> gpt-5.6-sol rename). gpt-5.2 stays selectable with no migration: it was retired from the ChatGPT product (steered to 5.5) but the API still serves it, so API-key Codex users retain it. Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk |
||
|
Vladyslav Hrabovyi
|
92c311ab1d |
fix(review): show System theme coherently across UI (#1015)
Use one shared Light/Dark/System registry across review, plan, annotate, Settings, and published UI consumers. Preserve the public Mode contract, scope wider menu geometry to theme pickers, and keep constrained palettes coherent in rendering and previews. Co-authored-by: Vladyslav Hrabovyi <vladyslav.hrabovyii@gmail.com> Co-authored-by: sasha <kredich05@gmail.com> |
||
|
Kushida
|
ee28f1a428 | fix: handle Windows install and review edge cases (#1021) | ||
|
Michael Ramos
|
e20fd97be7 |
Consumer enablement for @plannotator/ui (0.24.0) (#1017)
* feat(ui): AnnotationPanel renderCardFooter + readOnly host props Per-card footer slot for host reply/resolve UI (clicks inside don't select the card); readOnly hides delete/edit on all card kinds. Both optional, both no-op by default — Plannotator unchanged. * feat(ui): bless 6 exports into the strict-consumer surface TableOfContents, ResizeHandle, useResizablePanel, useActiveSection, useScrollViewport, utils/annotationHelpers — verified strict-clean and backend-free (useResizablePanel persists via the storageBackend seam); added to the gate and the HANDOFF supported-imports table. * feat(ui): Viewer allowImages + readOnly host props allowImages threads to both CommentPopover sites (popover already gated its attach affordance; Viewer never exposed the knob). readOnly suppresses every composer entry point — selection toolbar (highlighter 'enabled'), pinpoint, global comment, attachments, checkbox toggles — while existing annotations still render and select. Both default to today's behavior. * fix(ui): lazy-import Viewer/CommentPopover in the consumer test (DOM-less bun test crashed on web-highlighter) * feat(ui): strict-consumer gate gains verbatimModuleSyntax + noUnusedLocals/Parameters Fixed the 24 violations the flags surfaced across the supported-import graph: type-only imports (verbatimModuleSyntax) and dead imports/locals. Consumers with stricter tsconfigs no longer have to relax them. * feat(ui): opt-in content-verifying annotation restore verifyRestoredContent on useAnnotationHighlighter: after a meta-based fromStore restore, the painted text is checked against originalText (whitespace-normalized). Mismatch -> highlight removed, text-search fallback re-anchors; if that fails too, onRestoreMismatch(annotation, restoredText) fires and nothing is painted. Default off — today's trust-the-positions behavior. Workspaces hit this live after document drift; correctness upgrade for every consumer. * chore(ui): 0.24.0 — HANDOFF consumer-enablement notes + version bump |
||
|
Michael Ramos
|
070d9a5f6d |
Make the document UI reusable as published building blocks (#957)
* docs(adr): revert failed document-ui cutover, add ADR 004 with corrected reuse plan The document-ui extraction/cutover (ADRs 002/003) was an AI-driven rewrite that broke the app; the code was reverted. Add ADR 004 as the source of truth: share @plannotator/ui as published building blocks for the Workspaces app, keep Plannotator's app unchanged, gate on human-verified parity. Banner the reverted ADRs and point AGENTS.md/CLAUDE.md at 004 so future agents don't rebuild the mess. * docs(adr): add verified document-ui extraction plan, supersede draft inventory 36-agent verification of the reuse inventory: confirmed the /api coupling but found the draft missed Viewer's transitive backend call, the cookie settings layer, 3 React contexts + identity singleton, SSE transports, and harder packaging blockers. Adds the verified per-subsystem extraction plan with a parity guardrail on every step; flags the draft inventory as superseded. * docs(adr): add document-ui extraction roadmap + parity checklist Phase 0-7 execution roadmap (safety net -> packaging -> foundation seams -> rendering -> navigation -> comments -> extras -> publish) and the reusable 'did it break?' parity checklist run after every step. Both enforce the law: move + decouple, never rewrite; Plannotator's experience cannot change. * build(ui): packaging unblock for external install (Phase 1) — no runtime change Phase 0: captured parity baseline (typecheck/test/build + shipped-bundle hashes). Phase 1 packaging fixes to packages/ui, metadata only: - add phantom dompurify ^3.3.3 dep (imported in sanitizeHtml/aiChatFormat, was undeclared) - align diff ^8.0.3 -> ^8.0.4 with root - add peerDependencies (react, react-dom, tailwindcss, tailwindcss-animate); keep as devDeps - add files allowlist (excludes tests); remove dead tsconfig @plannotator/shared alias Verified byte-identical: typecheck pass, 1620 tests pass/0 fail, all 3 builds OK, shipped plan+review bundle hashes unchanged from baseline. Remaining Phase 1 blocker (@plannotator/ai + @plannotator/shared workspace:* deps) deferred pending a publish-vs-inline decision; logged in worklog. * feat(ui): make image URL resolution host-overridable (Phase 2, seam 1) getImageSrc now delegates to a module-level resolver defaulting to the verbatim Plannotator /api/image logic; add setImageSrcResolver/resetImageSrcResolver so a host (Workspaces) can resolve images via its own backend. All 5 consumers and the signature unchanged. Verified: default URLs byte-identical, typecheck pass, 1620 tests pass/0 fail, builds OK. No Plannotator behavior change. * feat(ui): make settings storage backend host-overridable (Phase 2, seam 2) storage.ts cookie impl is now the default 'cookieBackend'; add setStorageBackend/ resetStorageBackend so a host (Workspaces) can persist settings via its own storage. getItem/setItem/removeItem delegate to the active backend; the ~24 consumers and literal plannotator-* keys are unchanged. Verified: swap works, typecheck pass, 1620 tests pass/0 fail, builds OK, theme persists across reload. * feat(ui): make MarkdownEditor theme mode host-supplyable (Phase 3) Add optional mode? prop; mode now mode ?? resolvedMode. Plannotator passes no mode (App.tsx:4261) so it keeps using ThemeProvider's resolvedMode unchanged. A host without ThemeProvider can supply mode directly. Verified: typecheck pass, 1620 tests/0 fail, builds OK, App.tsx untouched. * feat(ui): allow hosts to opt out of code-path validation (Phase 3) Viewer gains optional disableCodePathValidation? threaded to a new disabled? arg on useValidatedCodePaths; when set, the /api/doc/exists probe is skipped. Default undefined for Plannotator => validation stays on, /api/doc/exists fires exactly as today. Verified: typecheck pass, 1620 tests/0 fail, builds OK, App.tsx untouched. Also logs Phase 3 workflow outcome + remaining scroll/docfetch pieces. * feat(ui): make code-file hover preview fetch host-overridable (Phase 3) Add DocPreviewFetcher seam (default = verbatim /api/doc fetch) + setDocPreviewFetcher/resetDocPreviewFetcher; route handleMouseEnter through it, useCallback deps unchanged. No caller overrides it => Plannotator fetches /api/doc identically. typecheck pass, 1620 tests/0 fail, builds OK. * feat(ui): ship ScrollViewportProvider with the library (Phase 3 scroll) Add render-transparent ScrollViewportProvider (createElement, keeps .ts) so the scroll-viewport context travels with @plannotator/ui instead of living only in App.tsx. Rewire App.tsx provider tags (3-line delta); identical tree/value/ position, sidebar TOC still reads the MAIN viewport. Fix stale OverlayScrollbars doc-comment. typecheck pass, 1620 tests/0 fail, builds OK, eyeball: TOC tracks. * fix(ui): disabled code-path validation should keep links clickable (self-review) The Phase-3 disabled branch set ready=true with an empty map, which makes gateCodePath demote every code link to plain text. Leave ready=false so the no-validation fallback renders links optimistically. No Plannotator impact (never disables). Logs Phase 3 completion + reusability note. typecheck pass, 1620 tests/0 fail, builds OK. * feat(ui): make file-tree backend host-overridable (Phase 4) Lift useFileBrowser's three backend wires (load-dir fetch, obsidian-vault fetch, and the SSE live-watch effect moved VERBATIM) into an injectable FileTreeBackend with default + setFileTreeBackend/resetFileTreeBackend, same pattern as the image /storage seams. useFileBrowser() stays zero-arg; default fetch/SSE URLs identical. Sidebar confirmed noop (zero backend wires, already reused by review-editor). Verified: useFileBrowser.test.tsx passes 6/0 UNMODIFIED (DOM_TESTS=1), typecheck pass, 1620 tests/0 fail, builds OK, App.tsx untouched, manual eyeball (annotate adr/: tree loads, file-switch works, new file appears live via SSE). Plannotator byte-unchanged. Logs two pre-existing bugs found during testing (not regressions). * docs(adr): research + synthesis + spec for Phase 5 (comments/annotations/drafts) Five-probe code research of the comment system. Key finding: most comment UI is already portable (panel/popover/toolbar/highlighter prop-driven; review-editor already reuses the hooks). Phase 5 narrows to 3 seams — draft transport (+ the 3-party generation protocol), external-annotation transport (SSE->polling, move verbatim), and identity/authorship — plus 2 non-extraction items: renderer coupling (document as a contract) and replies/threading (defer as a new feature). * docs(adr): accept ADR 005 — make comments/annotations/drafts host-overridable (Phase 5) Three seams (identity, draft transport, external-annotation transport), each defaulting to today's behavior; renderer coupling documented as a contract; replies/threading deferred as a new feature. Locks in the recommended choices from the Phase 5 spec/synthesis. * feat(ui): make annotation identity host-overridable (Phase 5 seam 1) Add IdentityProvider + setIdentityProvider/resetIdentityProvider in identity.ts; getIdentity/isCurrentUser now delegate to a module-level provider defaulting to today's ConfigStore tater behavior. The ~9 author-stamp sites and 2 (me)-badge sites delegate with zero call-site edits. No caller overrides => Plannotator byte-unchanged. typecheck pass, 1620 tests/0 fail, builds OK. * feat(ui): make draft persistence transport host-overridable (Phase 5 seam 2) Add DraftTransport (load/save/remove) + getDraftTransport/setDraftTransport/ resetDraftTransport in useAnnotationDraft.ts, default = today's /api/draft fetches verbatim. useCodeAnnotationDraft reads getDraftTransport() live. The generation pre-increment, 500ms debounce, keepalive retry-gate, and pagehide/visibilitychange flush stay in the hooks; getDraftGeneration() still escapes to the host. save rejects-on-failure so the gated retry is preserved. No caller overrides => Plannotator byte-unchanged. shared/draft.test.ts 10/0, annotationDraftPersistence 13/0, typecheck pass, 1620 tests/0 fail, builds OK. * feat(ui): make external-annotation transport host-overridable (Phase 5 seam 3) Add ExternalAnnotationTransport<T> (subscribe/getSnapshot/CRUD) + setters in useExternalAnnotations.ts; default = today's SSE->polling wire moved verbatim into createDefaultTransport. The reducer (applyEvent), fallback-once gate, 500ms poll, versionRef scoping, optimistic-before-await, and [enabled] gate stay in the hook. A host (Workspaces) can implement the same event contract over Durable Objects. No override caller => Plannotator byte-unchanged. external-annotations test green, typecheck pass, 1620 tests/0 fail, builds OK. Logs Phase 5 completion. * docs(adr): research + synthesis + spec for Phase 6 (versions, settings, sharing, AI) Five-probe code research. Most of the four subsystems is already portable; the real work is 5 seams (version fetchers + vscode-diff, config write-back, obsidian detect, save-to-notes, AI transport) + 1 CSS move (block/raw diff classes from the app shell into the package's theme.css). Fragile do-not-touch: the AI SSE reader loop + epoch guards, and configStore debounce/deepMerge. Five Plannotator-only pieces (OpenInApp, HooksTab, useUpdateCheck, useAgents/useAgentJobs) stay home. * docs(adr): accept ADR 006 — make extras (versions/settings/sharing/AI) host-overridable (Phase 6) Five seams + one CSS move, each defaulting to today's behavior. AI reader loop + epoch guards and configStore debounce/deepMerge stay verbatim. Five Plannotator- only pieces stay home. Locks the recommended choices from the Phase 6 spec. * feat(ui): make version fetchers + vscode-diff host-overridable; move diff CSS into package (Phase 6 versions) usePlanDiff gains optional fetchers (default /api/plan/version(s), error asymmetry kept: selectBaseVersion alerts, fetchVersions silent). PlanDiffViewer gains optional onOpenVscodeDiff (default /api/plan/vscode-diff). Relocate .annotation-highlight* + .plan-diff-* block/raw CSS from editor/index.css into ui/theme.css (next to .plan-diff-word-*) so the diff/highlights are self-styling from the package. Verified: relocated CSS gone from index.css, present in shipped bundle (33x), diff renders identical; typecheck pass, 1620 tests/0 fail, builds OK, App.tsx untouched. * feat(ui): make config write-back + obsidian-detect host-overridable (Phase 6 settings) configStore.setServerSync(fn) injects only the terminal POST /api/config; the 300ms debounce, deepMerge batching, singleton, and eager cookie reads stay verbatim. Settings gains optional onDetectObsidianVaults (default /api/obsidian/vaults), with the [obsidian.enabled] effect dep + auto-select-first-vault verbatim. No override caller => Plannotator unchanged. typecheck pass, 1620 tests/0 fail, builds OK. * feat(ui): make save-to-notes host-overridable (Phase 6 sharing) ExportModal gains optional onSaveToNotes (default = verbatim POST /api/save-notes); showNotesTab = isApiMode && !!markdown kept byte-for-byte. Sharing utils already parameterized (noop). No override caller => Plannotator unchanged. typecheck pass, 1620 tests/0 fail, builds OK. * feat(ui): make Ask AI transport host-overridable (Phase 6 ai) useAIChat gains a module-level AITransport (session/query/abort/permission) + setAITransport/resetAITransport, default = the five /api/ai/* fetches verbatim. The SSE reader loop, epoch/createRequest guards, and the supersede-abort position inside createSession stay untouched. Capabilities + provider-resolution stay host-owned in App.tsx. No override caller => Plannotator unchanged. ai.test.ts 97/0, typecheck pass, 1620 tests/0 fail, builds OK. * docs(adr): log Phase 6 completion (4 seams + diff CSS move) * docs(adr): research + synthesis + spec for Phase 7 (carve @plannotator/core + publish) Carve a browser-safe @plannotator/core: move the ~15 pure shared modules in, extract types from the 3-4 node-bound ones (config/storage/workspace-status) so nothing duplicates, shim @plannotator/shared so Plannotator's 99 import sites stay unchanged, re-point @plannotator/ui to depend only on core, move wideMode.ts, then publish core+ui (source-only). shared + ai stay private. Open: registry, versions, CI job. Publish is the one outward-facing step — confirm before pushing. * docs(adr): fold configurePlannotatorUI() front door + precompiled CSS into Phase 7 spec Add the single typed configure() facade over the 9 global host-override setters (zero-risk, additive) and an optional precompiled CSS bundle (smooths the Tailwind-in-shared-lib wrinkle) to the Phase 7 publish scope. Both make the published surface nicer to consume; neither touches Plannotator. * docs(adr): lock Phase 7 publish decisions + carry over review fixes Decided: ship JS as source (single internal consumer on controlled stack, no build to maintain, no dist drift); precompiled CSS now REQUIRED (the @source glob is fragile under pnpm symlinks); core CI typecheck node-free; pin ui->core exact. Recorded the interrogation's carried-over Phase-5 code fixes (useExternalAnnotations split-transport + fallbackRef reset, per-seam override tests, configStore loadFromBackend) to do before publish. * docs(adr): ADR 007 — carve @plannotator/core, complete settings provider, publish Locks Phase 7 decisions: public npm; lockstep version at repo 0.21.0 (ui->core pinned exact); JS ships as source + required precompiled CSS; core CI node-free; ai stays unpublished-to-npm. Settings provider completed (loadFromBackend, prefetch +sync) is now IN SCOPE — Workspaces uses the same UI settings stored in its own backend. CI publish job wired but artifacts validated on-branch (pack + dry-run) before merge; first publish gated. Carries the 2 override-path bug fixes + per-seam override tests as pre-publish work. * fix(ui): make external-annotation transport reads consistent + reset fallback on re-enable Two override-path bugs found by the interrogation pass (both unreachable on Plannotator's path; harden the host-override path for a real consumer): 1. Split-transport: the effect captured the transport at mount for subscribe/poll while the CRUD callbacks read the module global live, so a host swapping the transport after mount would split reads and writes across two backends. Capture once in a ref and use it in all four spots. 2. fallbackRef/receivedSnapshotRef were not reset on effect re-run, so an enabled false->true toggle inherited a stale 'already fell back' flag and silently stopped updating. Reset both at the top of the effect. Plannotator unchanged: it never overrides the transport (same default singleton captured) and enabled never toggles (reset is a no-op). typecheck clean; full test suite shows zero delta (1605 pass / 45 pre-existing env failures, identical with and without this change). * docs(adr): align Phase 7 spec with ADR 007 (version 0.21.0 lockstep, CSS required, scope completeness) * feat(core): carve @plannotator/core — move pure modules, extract node-bound types, shim shared (Phase 7 step 1) * feat(ui): depend only on @plannotator/core — re-point all shared/ai imports (Phase 7 step 2) * refactor(ui): relocate wideMode helper to @plannotator/ui/utils (Phase 7 step 3) * feat(ui): add loadFromBackend settings rehydration + configurePlannotatorUI front door (Phase 7 step 4) * build(ui): precompiled styles.css CSS build + madge circular-dep check (Phase 7 step 5) * test(ui): per-seam override tests + configure routing test (Phase 7 step 6) Add one override test per seam (setX(fake)→drive→assert→resetX()) for all 9 seams + loadFromBackend, modeled after the existing seam test pattern. Fix configure.test.ts to defer mock.module() into beforeAll and restore with captured real function references in afterAll so sibling seam test files are not poisoned by spy replacements in the shared Bun worker module registry. * fix(ui): apply Phase 7 review findings — version lockstep + seam consistency - Bump @plannotator/ui to 0.21.0 (lockstep with @plannotator/core + repo, per ADR 007) [was the 1 critical review finding] - useAnnotationDraft: route persistNow/dismissDraft save+remove through getDraftTransport() so all paths read the transport consistently (matches the load path; makes the single-global invariant explicit) - configStore.loadFromBackend: document it must be called BEFORE init() or server values get overwritten - packages/core/tsconfig: add explicit types:[] so the node-free invariant is first-class (verified: planted node:fs still fails TS2882) * docs(adr): Phase 7 implementation plan (workflow-generated, durable artifact) * fix(ui): reconcile #948 with the draft-transport seam + lockstep 0.21.1 Rebased onto origin/main (picks up #948 draft-deletion fix, the 0.21.1 bump, and the #949/#950 editor fix). The rebase auto-merged #948's code-draft logic (hasHadAnnotationsRef, empty-state tombstone, clearTimeout in restore/dismiss) with the Phase-5 transport refactor cleanly — except the empty-state tombstone delete was left as a raw fetch('/api/draft', DELETE). Route it through getDraftTransport().remove() so a host backend tombstones its own stored draft on clear (the #948 guarantee, for hosts). Plannotator unchanged (default transport hits the same endpoint). Bump @plannotator/core + @plannotator/ui 0.21.0 -> 0.21.1 to match main's version (lockstep per ADR 007). Verified: typecheck clean, madge no-cycles, plain suite 1637 pass / 0 fail, #948 draft-clear test 3/0. (The 45 DOM_TESTS failures are the known server/network integration tests that need a real OS env — same set on main, not regressions.) * fix(ui): address review nits — host-path robustness + cleanups - PlanDiffViewer: wrap onOpenVscodeDiff in try/finally so a host opener that throws can't wedge the VS Code button in a permanent loading state (default unaffected) - useExternalAnnotations: (re-)capture the transport inside the effect on enable so a host that installs a transport before enabling annotations is honored, not the stale default — keeps the split-transport fix (effect + CRUD share one ref) - configure.ts: import ServerSyncFn from configStore instead of duplicating the type - repoint the 2 remaining @plannotator/shared test imports to @plannotator/core - AGENTS.md/CLAUDE.md: document the new packages/core package All host-path only — Plannotator behavior unchanged. typecheck clean, no cycles, full suite green. Skipped (not simple/over-engineering): usePlanDiff prop->module-level (design change), Obsidian late-bind, getSnapshot guard (inert), transport <any> (variance). * docs: collapse 29 ADR process docs into one packages/ui/README.md The branch had accumulated ~6,200 lines of ADR scaffolding (6 decisions, 7 specs, 10 research spikes/synthesis, 6 worklogs/roadmaps/plans) for this one effort. Replace all of it with a single concise README that ships with the published package: what @plannotator/ui + @plannotator/core are, why they exist (commercial reuse), how the host-override seams work (configurePlannotatorUI), how a consumer installs/builds, and the one rule (don't reimplement from scratch — add a seam). Repoint the CLAUDE.md banner at the README. No code references the deleted docs; main's pre-existing adr/ docs untouched. * docs(ui): add packages/ui/AGENTS.md guardrail + CLAUDE.md symlink Directory-scoped agent guidance for anyone editing @plannotator/ui: don't rewrite from scratch, add a seam (default = today's behavior, Plannotator byte-for-byte unchanged), core stays node-free, never delete working code until human parity. Points to README.md for the architecture. CLAUDE.md -> AGENTS.md symlink mirrors the repo root convention. * build: remove madge circular-dep check (unmaintained) madge is unmaintained (~3 years stale) and the check was never wired into CI, so it was a dormant script + devDependency on a load-bearing path. Drop it: remove the check:cycles script, the madge devDependency, and .madgerc. The no-cycle invariant still holds by construction — @plannotator/core imports nothing (zero @plannotator deps in its package.json), so any accidental core->shared/ui import fails at publish-time bun pm pack (and review). No automated tripwire, but no stale unmaintained tooling either. * fix(ui): address review — TDZ guard, html-viewer export, doc corrections - useExternalAnnotations: declare unsubscribe as let (not const) + guard calls, so a host transport that fires onError synchronously during subscribe falls back to polling instead of throwing a TDZ ReferenceError (Plannotator's EventSource fires async, never hit) - package.json: add explicit ./components/html-viewer export (dir has index.ts; the ./components/* -> *.tsx wildcard can't resolve it, so external installers would fail) - README: fix configurePlannotatorUI sample keys to the real option names (storageBackend/identityProvider/imageSrcResolver/externalAnnotationTransport) - AGENTS.md: point the Ask-AI mapping at packages/core/agents.ts (shared/agents.ts is a shim now) All publish/host-path/doc only — Plannotator unchanged. (#1 CSS-build font collision deferred to publish-prep — it needs the asset pipeline + files allowlist, not a one-liner.) * build(ui): don't bundle fonts in published styles.css — app loads fonts (review #1) Industry standard for a shared UI package: ship theme + component CSS, let the consuming app load fonts. Drop the @fontsource imports from styles-entry.css (the publish CSS entry); the theme still defines --font-sans/--font-mono, and the app provides those families. Fixes the asset-name collision (every emitted .woff2 was renamed styles.css) and shrinks the published stylesheet 555kB -> 185kB. README documents the two-line @fontsource install. Plannotator unaffected: its apps (editor/review-editor index.css) load fonts via their own entry CSS — styles-entry.css is consumed ONLY by the publish CSS build. * fix(ui): build styles.css on prepack, not prepublishOnly (review #4) prepublishOnly doesn't run for npm pack / bun pm pack / git / file: installs, so the package exported ./styles.css without shipping it. prepack runs on any pack, so the stylesheet is always present. Verified: bun pm pack now emits styles.css. * chore(ui): post-rebase reconciliation — version lockstep 0.21.3, awaitable AI abort seam Rebased onto main (0.21.3). Bump @plannotator/core + @plannotator/ui to 0.21.3 to stay in lockstep with the repo version. Resolve the useAIChat conflict: main added postServerAbort (an awaitable abort that prevents session-busy races) using a raw fetch. Route it through the AITransport seam by making AITransport.abort return Promise<unknown> instead of void, so the host override is honored AND main's await-the-abort behavior is preserved. Update the abort mocks in the seam/configure tests accordingly. * fix(ui): make postServerAbort never reject regardless of AI transport The await site in ask() relies on postServerAbort resolving so a superseding query can proceed. main's original guaranteed this with its own .catch on the fetch; routing through the AITransport seam delegated that guarantee to the transport. Restore it at the call site (Promise.resolve(...).catch) so a host override that rejects — or returns void at runtime — can't throw out of ask(). * fix(ui): address review — core import, abort sync-throw, snapshot guards - useAIProviderConfig: import Origin from @plannotator/core/agents (was the only ui file still importing @plannotator/shared); drop the masking shared/* path alias from ui/tsconfig.json so a stray shared import now fails typecheck. The hook is part of the published surface — a standalone install had no @plannotator/shared to resolve. - useAIChat.postServerAbort: defer the transport call into .then so a host abort that throws *synchronously* also can't reject (the .catch only caught async). - useExternalAnnotations: default getSnapshot returns null (skip) on a malformed 200 instead of coercing to []/0, so it can't clear annotations or reset the version cursor — restoring the pre-seam behavior. * feat(ui): add upload + identity-editable seams for host backends Two override points the Workspaces app needs that had no seam: - UploadTransport (utils/upload.ts): image attachments hardcoded POST /api/upload with no override. Add a setX/resetX/getX seam (default = today's /api/upload, verbatim) and route AttachmentsButton through it. Workspaces sends bytes to its R2 asset API and returns the content-addressed URL. - IdentityProvider.isEditable() (utils/identity.ts): the Settings rename/regenerate controls wrote to the cookie store, bypassing a host identity provider — so a host with server-owned identity could split one user across two author names. Add an optional isEditable() (default true) and hide the rename controls when a host returns false. Plannotator's cookie identity stays editable — unchanged. Both wired into configurePlannotatorUI(); seam tests added; configure routing test covers uploadTransport. HANDOFF.md updated with the Workspaces seam mapping from the repo research (asset layer, identity, realtime, no-AI-infra, the Me display-name backend follow-up). README publish command corrected to bun pm pack + npm publish. * refactor(ui): capture sessionId synchronously in postServerAbort Self-review: the deferred .then read sessionIdRef.current a microtask after the guard checked it. Capture the id synchronously so the abort always targets the session current at call time and there's no double-read. * fix(ui): address review — seed host store, browser-safe timer type, harden abort - configStore.loadFromBackend: seed the host StorageBackend with resolved defaults for keys it lacks. The constructor runs at module load (before a host installs its backend), so its default-seeding writes went to the cookie backend; without this a fresh host store was never populated and generated defaults (e.g. displayName) regenerated every reload. [P1, host path] - Viewer.tsx: replace NodeJS.Timeout with ReturnType<typeof setTimeout> (2 refs) so a browser-only consumer compiling the published source doesn't need @types/node. Matches the pattern already used in configStore. [P1, published path] - useAIChat: harden the create-session supersede abort the same way as postServerAbort, so a host transport that throws can't surface an unhandled rejection. No impact on Plannotator (default self-catches). [nit] - .gitignore: correct stale 'prepublishOnly' comment to 'prepack'. [nit] Plannotator behavior unchanged (it never calls loadFromBackend; the timer/abort changes are behavior-preserving). Strengthened configStore seam test to assert first-run seeding. typecheck clean, 1773 pass / 0 fail. * refactor(ui): single-source the never-reject abort via safeAbort helper Self-review: the hardened abort pattern (defer into .then + .catch so a host transport that throws can't reject) was duplicated across postServerAbort and the create-session supersede site — the exact drift the review flagged. Extract a module-level safeAbort(sessionId) so both call sites share one hardened implementation and can't diverge again. Behavior unchanged; reads aiTransport at call time so a late override is honored. * chore(ui): post-rebase version lockstep to 0.21.4 Rebased onto main (0.21.4, adds markdown math #878 + parser hardening). Bump @plannotator/core + @plannotator/ui to 0.21.4 to stay in lockstep with the repo. katex (main's math dep) merged into ui; typecheck clean, 1810 pass / 0 fail. * docs(ui): consumer-lens handoff hardening + ADR 005 - HANDOFF.md: add supported-imports allowlist vs unsupported (hardcoded /api/*) list; document the annotation anchor schema, reattachment order, and untested stale-anchor degradation; state that the markdown editor cannot take CM6/Yjs extensions yet and the plan of record; note AI avoidability re-verified post-rebase; fix stale 0.21.3 ref. - adr/decisions/005: record the publish-as-packages decision (packages over copy/vendor, core/ui split, seam-singleton pattern + SSR revisit condition, the law, lockstep publish model). * fix(ui): make shipped source strict-TS clean for consumers + seam type barrel Consumers compile the published TS source with their own compiler options, and strict mode failed with 35 errors inside the package: - settings.ts: satisfies SettingDef<unknown> is contravariantly illegal under strictFunctionTypes (33 errors) — use SettingDef<any> - useDismissOnOutsideAndEscape: RefObject<HTMLElement> rejects React 19's useRef<T>(null) refs — widen to HTMLElement | null - globals.d.ts: declare *.png / *.webp modules, referenced from each asset-importing component so any consumer program that includes one gets the ambient declarations Also unscatter the seam contract types: configure.ts re-exports every seam type next to configurePlannotatorUI, and ServerSyncFn is now exported from config/index.ts (it was unreachable through the exports map). Verified: standalone Vite consumer importing the full supported surface passes tsc --noEmit under full strict (was 35 errors). * fix(ui): keep KaTeX fonts out of published styles.css (back to ~187KB, was 1.6MB) Main's math PR imports katex/dist/katex.min.css in theme.css; the publish build (Vite lib mode) force-inlines all 60 KaTeX math fonts as data URIs, ballooning styles.css to 1.6MB (977KB gzip) and breaking the package's consumer-owns-fonts policy. Alias the katex stylesheet to an empty stub in vite.css.config.ts only — theme.css stays untouched (no rebase surface) and Plannotator's own apps, which import theme.css directly, still bundle KaTeX as before. Hosts that render math load katex.min.css themselves (bundler import, CDN tag, or self-hosted copy per HANDOFF.md), which also gets them lazy font loading. Verified: fresh build is 186.9KB / 30.8KB gzip with zero @font-face data URIs; consumer vite build CSS drops 1.66MB -> 200KB. * docs(ui): HANDOFF corrections from adversarial consumer review - Math rendering section: KaTeX css/fonts excluded from styles.css by design; three one-time host setup options (self-hosted recommended, CDN tag, bundler import) - styles.css size claim corrected (~187KB / ~31KB gzip) + strict-TS guarantee documented (verified against a standalone consumer) - AI-avoidability claim made precise: configure.ts statically imports useAIChat for its setter; unused AI code tree-shakes to zero (bundle- verified) — the runtime claim holds, the static wording was wrong - Loud warning on the loadSettingsFromBackend ordering footgun: configuring before hydration seeds generated defaults into the host backend and nothing re-runs hydration - DraftTransport.load() tombstone-generation contract spelled out - Seam-type barrel documented on the configure row; 'everything is importable' softened (some components/*.ts don't resolve via the *.tsx wildcard); stale diff stats refreshed * docs(ui): math setup pointer in README + pnpm caveat on the katex bundler-import option * fix(ui): lazy settings resolution — zero cookies on a configured host The configStore resolved all settings eagerly in its constructor, at module import — before a host's configurePlannotatorUI() could install its StorageBackend — writing 17 plannotator-* cookies (including a generated identity) onto the host origin. Resolution now runs lazily on first settings access (get/set/init/loadFromBackend): by then the host backend is live, so the initial reads AND default-seeding writes route through it. A configured host gets zero cookies, ever. Plannotator unchanged: same resolution, same cookie seeding, same values — on first settings read (same page load) instead of at import. New configStore.lazyInit.seam.test.ts proves the contract from a fresh module graph; full suite + consumer strict tsc green. * chore(ui): post-rebase version lockstep to 0.22.0 * fix(ui): round-2 review batch — dedupe asset declarations, CI seam tests, strict consumer gate, doc corrections - components/types.d.ts: drop the *.png/*.webp declarations that globals.d.ts now owns — both shipping was a duplicate-identifier error for any consumer with skipLibCheck: false - untrack packages/ui/styles.css (generated by prepack, gitignored; got scooped into the carve commit during the rebase by git add -A before the ignore entry existed in the replay) - CI: the DOM test step now runs ALL packages/ui tests, so the seam contract tests (AI/draft/external-annotations/file-tree/inline- markdown) actually execute in CI instead of skipping - new packages/ui/tsconfig.strict-consumer.json wired into root typecheck: type-checks the supported-import surface under full strict, so the consumer strict-TS guarantee can't silently rot - HANDOFF: rot-proofed the diff stat, strict guarantee now cites the CI gate, CDN katex pinned-version wording, theme-vs-styles.css caveats (theme still imports KaTeX + needs Tailwind), Viewer required props, Yjs plan-of-record updated to the atomic-editor fork - README: @source fallback wording (build entry isn't shipped) * test(ui): make the lazy-resolution seam test deterministic The test asserted lazy resolution on the module singleton and relied on its test file getting a fresh module graph — an isolation assumption that doesn't hold under all bun test orderings (CI failed with zero observed reads because another file had already resolved the store). Test the contract on a fresh instance instead: ConfigStore is exported as @internal ConfigStoreForTest, the spy backend is installed before construction, and the test asserts construction reads nothing while the first get() resolves and seeds through the live backend. Deterministic by construction. * test(ui): poll for the debounced reconnect refetch instead of a fixed sleep The reconnect-refresh assertion waited a fixed 150ms against the SSE watcher's 120ms debounce — a 30ms margin that slower CI runners lose, flaking 'refreshes after an SSE ready event from reconnect'. The watched logic is unchanged (verified byte-identical to main's inline version — the seam only relocated it into the default watchTrees and added the onChange indirection). Poll for calls.length===2 up to 1s so the pass/fail is hardware-independent. * test(ui): poll the committed tree state, not the fetch call count Prior fix polled calls.length===2, but the fetch call is counted one tick before its result commits to React state — so the poll exited early and the next assertion (dirs[0].tree === reconnectedTree) lost the race on slow CI (toEqual failure). Poll on the committed tree itself, which is exactly what the assertion checks: now the only way to fail is a genuine no-refresh, not a timing margin. * test(ui): give the reconnect-refetch poll a 10s ceiling + 20s test timeout A CI runner was measured at 6x normal speed (1676ms for a ~275ms test), blowing through the 1.5s poll ceiling before the 120ms debounce fired — same commit passed on a faster runner. Raise the poll to ~10s and set an explicit 20s test timeout (bun's 5s default would otherwise kill the poll). Root cause is load, not logic: this timing-sensitive test only started flaking when the CI DOM step was broadened to run the whole ui suite in one process. * ci: run the file-browser DOM test isolated; scope the DOM step to DOM files Root-causes the intermittent 'refreshes after an SSE ready event from reconnect' failure. The round-2 change ran the ENTIRE ui suite under DOM_TESTS=1 to catch the seam contracts; that load intermittently starved the test's 120ms real-timer debounce so the reconnect refetch never fired (observed failing after a full 10s poll — not a margin issue). The hook logic is byte-identical to main, and main runs this test in its own process (green for months). Fix at the CI layer, not the test: run useFileBrowser.test.tsx isolated (matching main), and run the seam contracts + remaining DOM-gated tests as an explicitly-scoped light batch. The test file is reverted to main verbatim (today's timing-poll experiments dropped). Follow-up issue to file: the underlying re-subscription race the load exposed. |
||
|
Michael Ramos
|
795f381ebe |
feat(review): "All changes" git-status review view (#990)
* feat(review): "Since main" git-status review view + correctness fixes
Adds a composite `since-base` diff — merge-base(origin/main, HEAD) vs the
working tree, plus untracked — as the default code-review view, rendered as a
three-section "git status" panel (Committed / Changes / Untracked) with a
Sections|Tree toggle, a first-run setup chooser, and a "baseline behind GitHub"
fetch banner. Everything normalizes to one git patch, so the diff viewer,
annotations, and feedback path are unchanged.
New diff type wired through runGitDiff / fingerprint / file-content / context /
staging / agent-context in packages/shared + both server runtimes (Bun +
Pi mirror). Default flipped from `unstaged` to `since-base`; the old
DiffTypeSetupDialog is replaced by ReviewSetupDialog (view + default-diff
chooser, screenshots, Settings access).
Includes a reviewed batch of correctness fixes:
- P0: baseBehindRemote was permanently true (rev-parse missing --verify)
- fingerprint blind to quoted/unicode untracked paths (unquote)
- sections parser: record both sides of a rename; staged-delete wins over
untracked (rm --cached collision)
- graceful degrade when merge-base can't resolve (trunk/no-remote repos)
- /api/fetch-base re-queries remote (narrow-refspec honesty)
- /api/diff/switch concurrency guard (diffSwitchEpoch) + draft rekey
- decouple remote-staleness probe from initialBase (Pi parity)
- keyboard file nav in the sections view; banner gated to base-relative modes
See adr/decisions/005-since-base-github-view-default-20260701-223706.md
* fix(review): since-base review-round hardening (11 fixes, both runtimes)
Addresses the multi-agent + PR-990 review findings:
- Sections rename parser split ` -> ` on the raw quoted porcelain token, so a
filename containing ` -> ` tore into garbage and a dirty file could render as
"Committed". Now quote-aware (splitPorcelainRename + unit tests).
- /api/diff/switch captured the epoch AFTER await req.json(), letting a
slow-body older switch overwrite a newer confirmed one. Epoch now captured
before any await; hideWhitespace committed only on win. Both runtimes.
- Unresolvable base (trunk / no origin/HEAD) no longer auto-defaults to a
degraded since-base that hides committed work — getGitContext only offers
since-base when the base ref resolves, so the default falls through to
uncommitted. Fingerprint + file-content degrade to HEAD to match the diff.
- git rm --cached no longer yields two diff entries for one path (untracked
files already in the tracked patch are dropped) — fixes wrong-file-open and
j/k nav looping in the Sections panel; also fixes latent uncommitted/unstaged.
- Settings' "Default Diff View" list now preserves the sections<->since-base
coupling (can't leave an invalid sections+classic pair).
- "Behind GitHub" banner only treats origin/* as fetchable; a bare local base
("main") is upgraded to its tracking ref at startup so Fetch can clear it.
- hashUntracked resolves untracked paths against the repo toplevel, not cwd, so
a review launched from a subdirectory isn't blind to untracked edits.
- Agent review instruction now tells agents to enumerate/inspect untracked files.
- Terminology: one "Committed changes" label; the live "Since <base>" label is
dynamic (matches the header); "Since main" kept only as product copy.
- Docs: CLAUDE.md endpoints/fields + since-base view section; ADR recap.
Verified: typecheck (all projects), bun test 1793 pass, and empirical repro of
the rm --cached dedup, trunk->uncommitted fallback, and rename parser.
* fix(review): restore "(PR view)" on the Committed-changes label (unify toward it, not away)
* fix(review): close since-base coverage gaps from PR-990 review round 2
- First-run setup no longer forces since-base on repos where it isn't available
(base ref can't resolve). getGitContext omits since-base there; the first-run
block now checks the same availability before resetting the default or showing
the chooser, so committed work isn't silently hidden on trunk/no-origin repos.
- reviewBase now includes 'since-base', so changing the base while in the
git-status view passes the selected base to /api/file-content — expandable
context is fetched from the right merge-base (was falling back to default).
- Settings now mirror ReviewSetupDialog's coupling exactly: Sections ⇒ force
since-base; Tree ⇒ leave the diff (Tree + since-base is valid and now
saveable); classic diff ⇒ Tree; since-base diff ⇒ leave the view. Removes both
coercions that previously discarded a supported preference.
* fix(review): PR-990 review round 3 — fingerprint/dedup/base-canonicalization
- Fingerprint now uses `git status --porcelain -uall`, so editing a file inside
a brand-new untracked directory changes the fingerprint and the "Diff out of
date" banner fires (was collapsed to `?? dir/` and hashed as unreadable).
- extractTrackedPatchPaths pairs `---`/`+++` and excludes only the file's KEYED
path (new side, or old side for a pure deletion) — no longer drops a recreated
rename-source file (`git mv a b && touch a`). Verified rm --cached still
dedups to one entry and the recreated file still shows.
- resolveReviewBase canonicalizes a bare local default name ("main") to its
tracking ref ("origin/main") on every call, so a client that loaded before the
startup upgrade resolved can't revert the server to the stale local base on the
next refresh/switch. Both runtimes (new Pi resolveReviewBase helper).
- Removed the dead activeDiffLabel prop chain (FileTree -> DiffTypePicker).
- ADR recap: documented the staleness-banner scope limitation (default base only).
Verified: typecheck (all projects), bun test 23/23 review-core+fingerprint pass,
empirical repro of the untracked-dir fingerprint, rm --cached dedup, and
rename-recreate cases.
* fix(review): PR-990 review round 4 — dedup content, staging gate, banner timing
- Dedup rewrite: instead of dropping the untracked side of a same-path collision
(which hid content), drop the tracked DELETION block and keep the untracked
working-tree content. `git rm --cached f` + edit now shows the new content, not
a phantom deletion — still one entry per path (no dock/nav collision). New
stripHeaderPath also strips git's trailing-tab metadata, so space-named files
dedup correctly. Verified: rm --cached+modify shows content (1 entry),
space-named (1 entry), rename+recreate still shows both.
- All-files (and the `a` shortcut on the focused file) now gate staging per-file:
committed files in since-base mode are not stageable, matching SectionsPanel /
FileTreeNode. Shared isPathStageable helper threaded via ReviewStateContext →
AllFilesCodeView. Stops the confusing `git add` no-op that still flipped local
staged/viewed state.
- /api/diff/switch now awaits recomputeBaseBehindRemote before building the
response, so the "Baseline behind GitHub" banner reflects the new base
immediately (no ~5s lag switching in, no stale banner switching away). Both
runtimes.
- ReviewSetupDialog re-applies the recommended default only on first-run dismiss,
not when reopened from the header menu (was snapping a mid-session diff back).
- SectionsPanel "N added" header now counts sidecar-staged files too, so it
matches the staged dots on rows.
Verified: typecheck (all projects), bun test 1830 pass, empirical dedup repro,
both bundles build.
* refactor(review): reuse parsePatchPathToken in removeTrackedDeletions (self-review)
Drop the duplicated stripHeaderPath helper — the shared diff-paths
parsePatchPathToken already strips a/|b/ prefix + C-quoting + git trailing-tab,
and verifies the prefix instead of blindly slicing two chars. Documents the
binary-deletion edge (no --- line, so a binary rm --cached is not deduped).
* fix(review): PR-990 review round 5 — mixed-base + staging/sort/flicker nits
- Atomic base upgrade: the startup origin/* canonicalization swapped currentBase
without rebuilding the patch, so /api/diff could advertise origin/main while
the served hunks came from local main (mixed-base review on origin/HEAD-absent
repos). Now rebuilds the diff for the new base and commits base+patch+ref+
fingerprint together (only if no user switch happened); the fingerprint change
makes the client's freshness poll pick it up. Both runtimes.
- isPathStageable: gate staging OFF when since-base is active but the sidecar
hasn't loaded (was falling through to true, allowing a git-add no-op on a
committed file).
- SectionsPanel: session-staged files now float to the top of Changes — the sort
key was `false ?? stagedFiles.has(...)` which short-circuits; now ORs them.
- /api/diff/fresh: early returns now carry baseBehindRemote, so a snapshot change
mid-probe no longer clears the "behind GitHub" banner for one poll. Both runtimes.
- Removed the now-dead activeLabelFallback prop from DiffTypePicker (FileTree
stopped forwarding it in round 3).
Skipped (agreed): the fetch-base input-validation nit (no realistic attacker)
and the diff-switch TOCTOU (concurrent switches aren't UI-reachable).
Verified: typecheck (all projects), bun test 1830 pass, both bundles build.
* refactor(review): drop redundant staged-OR in SectionsRow (self-review)
item.staged (the grouping key) already ORs in stagedFiles as of the round-5
sort fix, so the row-level `|| stagedFiles.has(...)` is dead weight.
* fix(review): surface the startup base upgrade to already-loaded clients
Review round 6 fixes:
- The startup main -> origin/main upgrade re-baselined the freshness
fingerprint, so a client that fetched /api/diff before the rebuild kept
the old patch and every /api/diff/fresh probe reported fresh (the probe
compares server state to itself, never to what the client renders). Now
the fingerprint is only re-baselined when no client has loaded the
pre-upgrade snapshot; otherwise the stale baseline trips the normal
"Diff out of date - Refresh" banner. Bun + Pi.
- preserveFile refreshes (staleness Refresh, post-Fetch) now adopt the
server's returned base — exactly the paths where the server may have
canonicalized main -> origin/main; keeping the old name sent
/api/file-content and Ask AI context against the wrong base.
- ReviewSetupDialog: clamp the fixed 800px height to the viewport so the
dialog fits on small laptop screens.
- Docs: /api/diff/fresh response also carries baseBehindRemote/agentCwd.
- Documented the committed-deletion + untracked-recreation dedupe edge as
accepted (code comment + ADR) — fixing it needs two same-path diff
entries, which the path-keyed UI cannot represent.
* fix(review): stop the Git-status default from silently reverting to tree
Users with reviewPanelView=sections could keep opening in the tree view on
a stale diff type despite their cookie saying Git status. Three causes:
- The header Sections toggle persisted the view but not defaultDiffType,
creating a conflicted pair (sections + non-since-base default) that every
UI writer is supposed to prevent. It now couples the diff default like
the setup dialog and Settings do.
- configStore's debounced POST /api/config could be lost when a session
closed within 300ms of a change, leaving cookie and config.json split.
Pending writes now flush on pagehide with a keepalive fetch.
- configStore.init() applies config.json over the cookie without the UI
coupling, so a stale server value re-corrupted the pair on every load.
The app now self-heals at load: if the view says sections but the diff
default isn't since-base, it repairs the default (cookie + config.json)
and switches the live session to since-base.
* fix(ui): keep the stage (+) button border visible on the active file row
The button's --border border has no contrast against the active row's 30%
primary tint, so it vanished on the selected row until hovered. Tint the
border with the row's primary color on active rows (both sections and tree
views); the button's own hover border still applies.
* ui(review): spell out the panel view toggle — 'Git status | Tree' text instead of icons
* fix(review): PR-990 review round 7 — export label, diff snapshot race, unicode paths
- exportFeedback describeDiff(): add the missing since-base case — every
feedback export in the new default mode read "**Diff:** since-base".
- /api/diff GET: snapshot patch/base/ref/error BEFORE the sections-sidecar
await and pass the pinned base into buildSectionsSidecar. The startup base
upgrade landing mid-await could pair a rebuilt patch with sections grouped
against the old base, with initialDiffServed still false so no refresh
banner ever came. Bun + Pi.
- SectionsPanel: a session-staged untracked file now moves to the Changes
section immediately (anticipating the server's next sidecar) instead of
sitting in Untracked with a staged dot until refresh.
- unquoteGitPath: real C-style unquoting with octal (UTF-8 byte) escape
decoding. JSON.parse rejects octal escapes, so non-ASCII names kept their
literal \303\251 form — an untracked "café.txt" was silently ABSENT from
the review (git diff --no-index could not access the quoted name) and the
deletion dedupe Set lookup could never match. getUntrackedFileDiffs now
unquotes ls-files output. Unit tests: octal decoding + end-to-end unicode
untracked file in a real repo.
- review-core.test: worktree subtype round-trip now covers since-base and
all (was 6 of 8).
Parked (deliberate): SectionsPanel/FileTree keyboard-nav dedup refactor.
* fix(shared): unquoteGitPath keeps literal unicode intact (self-review)
The byte-collector treated literal non-ASCII code units as single bytes,
which would mojibake headers synthesized by our own quoteGitPath
(JSON.stringify leaves unicode unescaped inside quotes — workspace-mode
prefixed headers round-trip through the same parser). Literal chars now
append as string code units (surrogate-safe); only octal escapes go
through the UTF-8 byte decoder.
* fix(review): PR-990 review round 8 — pre-staged toggles, explicit base, AI context race
- useGitAdd: session Set replaced with a tri-state override map folded over
the sections sidecar; stagedFiles is now the EFFECTIVE staged set (sidecar
+ session stages - session unstages). Fixes pre-staged files: the first
`a` press actually unstages (was a git-add no-op), the sidebar dot clears
after a real unstage (was stuck via sidecar OR), and the All-files header
agrees with the sidebar on load. Overrides reset whenever a fresh sidecar
arrives (switch, preserveFile refresh, PR response) so stale session
intent can't fight new porcelain truth. SectionsPanel drops its own
sidecar OR; stagedCount = effective size.
- Explicit base picks are honored verbatim: the picker sends explicitBase,
and the server permanently disables local-name -> origin/* canonicalization
once set (the local/remote groups are distinct choices). The behind-GitHub
banner also exempts an explicitly-picked local name — Fetch advances
origin/*, so the banner would be un-clearable nagging. Bun + Pi.
- buildCurrentAiReviewContext(patch, base): GET /api/diff builds Ask AI
context from the same served snapshot as the patch — the startup base
upgrade could hand Ask AI a different changeset than the screen. Bun + Pi.
- recomputeBaseBehindRemote: capture remoteDefaultInfo once — a concurrent
refresh nulling it mid-await threw. Bun + Pi.
- Revert stray "Status Update / Updated!" edit to adr/0001 (test debris
swept into the round-2 commit).
- splitPorcelainRename comment corrected (porcelain v1 does NOT quote plain
spaces; a name containing " -> " is ambiguous without -z) + ADR notes for
the index-only-changes semantics and the rename edge.
* fix(review): FileTreeNode uses the effective staged set — round 9
Round 8 made stagedFiles the effective set (sidecar + session overrides)
and removed SectionsPanel's sidecar OR, but missed the same OR in
FileTreeNode's since-base row (sectionStaged). In the Tree fallback a
pre-staged file unstaged this session kept its dot and the next toggle
re-staged it. Grep-swept: this was the last surviving sidecar-staged OR.
* refactor(review): make the staged-display invariant unrepresentable
The round-8/9 bug class (sidecar staged flag ORed over the effective set)
existed because surfaces had a second staging source to reach for. Remove
it: stagedFiles is now a REQUIRED prop on SectionsPanel/FileTree/
FileTreeNode and the optional-prop fallback branches reading
sectionEntry.staged for display are deleted — a future surface cannot
reintroduce the OR because the pattern no longer exists to copy. The
sidecar type's staged field and AGENTS.md now document the invariant at
the point of temptation. Grep for display reads of .staged now hits only
useGitAdd (owner) and the sidecar builder (producer).
* refactor(review): self-review cleanups on the staged-invariant hardening
- orderFilesBySections: document why its snapshot .staged read is safe
(only called at sidecar-fresh moments where snapshot = effective) and
that it must not be reused mid-session — the one remaining display-side
snapshot read the invariant sweep surfaced.
- FileTreeNode: drop the now-pure sectionStaged alias; use isStaged.
* fix(review): PR-990 review round 10 — header staging gate, fingerprint cap, single-writer coupling
- Single-file diff header now uses the per-path staging gate (canStagePath),
closing the last ungated staging trigger: committed-only files in
since-base offered a no-op Git Add that flipped local staged/viewed state.
Full trigger inventory swept: App `a` shortcut, all-files `a` + header,
SectionsPanel rows, FileTreeNode rows, single-file header — all six now
per-path gated or group-gated; no context-menu staging exists.
- Fingerprint circuit-breaker: the freshness poll's `git status --porcelain
-uall` degrades permanently (per cwd, per process) to collapsed -unormal
once its output exceeds 2MB — a forgotten node_modules/ no longer burns
CPU every 5s for the whole session. Costs untracked-dir edit sensitivity
only on such repos; one possibly-spurious staleness banner at the switch.
- The sections ⟺ since-base coupling now has a single writer:
setReviewPanelView/setReviewDefaultDiffType in @plannotator/ui/config.
All five call sites (setup dialog x2, Settings x2, header toggle,
first-run reset, self-heal) converted; grep for direct writes of either
setting now hits only reviewView.ts.
- /api/diff/switch responses pass snapshot args to the AI context builder
(both branches, Bun + Pi) — correct today, now robust against future
awaits between the epoch check and the response.
- Docs: explicitBase in the /api/diff/switch body.
Parked per discussion: SectionsPanel/FileTree nav+search+footer dedup
(extract when the commit-list view adds a third panel), querySelector row
measurement, fetch-base stderr passthrough (standing decision), Pi
hasAgentLocalAccess (pre-existing follow-up list).
* fix(review): PR-990 review round 11 — subdirectory launches, escape decode, settings note
- Repo-root-relative patch paths now resolve against the git toplevel
everywhere they meet a filesystem path or pathspec, via a shared
resolveRepoToplevel helper: file-content working-tree reads (hunk
expansion returned null from a subdirectory launch) and gitAddFile/
gitResetFile (stage/unstage failed with pathspec errors). Both bugs
pre-existed for uncommitted/unstaged; since-base made them the default
experience. The two existing inline toplevel resolutions (untracked
diffs, fingerprint) now use the same helper. Shared code — Pi inherits
via vendoring. Real-repo subdirectory tests for both.
- unquoteGitPath decodes \uXXXX (JSON.stringify emits it for control
chars without a short escape; our synthesized workspace headers
round-trip through this decoder). Malformed \u stays literal. Tests.
- Pi explicitBase guard matches Bun byte-for-byte on empty-string base.
- Settings Git tab notes when the CURRENT repo can't serve the Git-status
view (base ref unresolvable) instead of letting the preference look
silently broken — it's a global preference, so the options stay.
Same-class items left parked (pre-existing, untouched by this PR):
open-in root resolution and code-nav file reads from subdirectory
launches — on the follow-up list with the Pi divergences.
* fix(review): PR-990 review round 12 — per-client freshness, keyboard-operable row controls
- Freshness is now judged PER CLIENT: every patch-carrying response
(/api/diff, /api/diff/switch, pr-switch, pr-diff-scope) includes
snapshotId (the server's draftKey), and the client echoes it on
/api/diff/fresh probes. A mismatch reports stale for THAT client
regardless of the VCS fingerprint. This fixes the round-12 finding —
reloads/second tabs after the startup base upgrade got a permanently
bogus staleness banner from the shared pre-upgrade baseline — and
deletes the round-6 conditional re-baseline hack entirely (the
fingerprint recaptures unconditionally again; the old client's banner
now comes from its snapshot mismatch, not a deliberately stale
baseline). Also gives unfingerprintable modes (P4, PR layer) snapshot-
level staleness for free. Bun + Pi + client hook.
- StageControl and ViewedControl (which had the identical gap) are
keyboard-operable: tabIndex + Enter/Space activation + focus outline.
They're spans inside the row <button> (nested real buttons are invalid
HTML), so they need their own focus stop; the a/v shortcuts remain the
power path.
- Docs: snapshotId on /api/diff, ?snapshot= on /api/diff/fresh.
* fix(review): PR-990 review round 13 — re-key snapshots on scope switch, PR-tab refresh
- The PR scope switch now re-keys draftKey (= snapshotId + draft storage
key) at BOTH commit points, unconditionally — matching every other
snapshot commit site. The full-stack branch previously kept the layer
patch's key: stale layer tabs never got the banner after a cross-tab
scope switch (the exact case snapshotId exists for), and full-stack
drafts collided with layer drafts (pre-existing). The layer branch's
!layerPatchIncomplete conditional is gone — it only stayed consistent
because full-stack never re-keyed. Invariant now: every currentPatch
commit is followed by a re-key. Bun + Pi.
- Refresh works for any stale PR tab: re-selects the CURRENT scope
instead of no-opping for layer (only full-stack could go stale in the
fingerprint-only world; snapshot mismatch changed that). Accepted
residual (documented in code): after a cross-tab PR switch, refresh
updates the patch but not prMetadata — the scope endpoint doesn't
carry it, and the full-rehydrate refactor isn't worth the two-tab edge.
* fix(review): stale incomplete-layer Refresh uses the non-blocking upgrade path (self-review)
Round 13 made Refresh re-select the current scope for stale PR tabs; for
an INCOMPLETE layer patch that POST triggers the server's local recompute,
which can park for minutes behind checkout warmup — and
handlePRDiffScopeSelect renders the full-screen switch overlay the whole
time. Route that case through handleLoadFullDiff (same POST, progress
notice instead of modal), which exists for exactly this slow path.
* fix(review): round 14 — composite snapshot id, fully-pinned /api/diff, "All changes" label
- snapshotId is now content hash + diff type (+ PR scope), built by a
single currentSnapshotId() helper used at every response site and the
freshness compare. A cross-tab MODE switch with a byte-identical patch
(layer vs full-stack on a single-PR stack) now flags old tabs; the base
is deliberately excluded so a same-commit main -> origin/main
canonicalization stays banner-silent (round-12 noise-avoidance kept).
draftKey stays a pure content hash — drafts survive content-identical
round-trips. Bun + Pi.
- GET /api/diff pins ALL served fields (diffType, hideWhitespace,
prDiffScope join patch/base/ref/error/snapshotId) and the sidecar + AI
context builders take the pinned type instead of reading globals — a
concurrent tab switch during the sidecar await can no longer produce a
since-base patch labeled with another mode. Bun + Pi.
- First-user feedback: the since-base label is now plain English —
"All changes since origin/main" (dynamic, follows the picked base);
dialog/Settings short form "All changes"; feedback exports match.
uncommitted reverts to "Uncommitted" where it had borrowed "All
changes", so the two stay distinguishable side by side.
* docs(review): finish the Since-main -> All-changes terminology sweep in comments (self-review)
* fix(review): PR-990 review round 15 — base-revert race, freshness reset, unstage regroup
- resolveReviewBase gains a second, probe-independent rule: a non-explicit
echo of the bare local name of the CURRENT origin/* base stays on the
tracking ref. The existing rule keys off remoteDefaultInfo, which comes
from a second network probe that can lag the startup upgrade by seconds;
in that window a diff-type/whitespace switch echoing "main" committed
the session back onto the stale local branch and set baseEverSwitched,
permanently blocking the upgrade. Bun + Pi.
- useDiffFreshness resets stale/dismissed state on snapshotId too — since
round 14 a new snapshot can reuse identical patch text with a different
id, and the old banner state wrongly carried over until the next poll.
- SectionsPanel: unstaging a PRE-staged add moves it back to Untracked
(mirror of the round-7 stage regroup). Detection deliberately uses the
sidecar's snapshot flag to recognize "was pre-staged"; staged
modifications stay in Changes, staged renames remain a refresh-heals
edge.
* fix(review): PR-990 review round 16 — rename staged count, guarded fetch-base replay
- "N added" no longer double-counts staged renames: the sidecar truthfully
marks BOTH porcelain sides staged, but an above-threshold rename renders
as ONE file — the hidden old path inflated the count and left a phantom
effective-staged entry that unstaging the visible row couldn't clear.
The client now filters sidecar-staged paths to rendered files, which is
correct in both patch shapes (below-threshold renames render delete+add
as two rows and both sides pass the filter). Client-only; the sidecar
stays faithful to porcelain.
- The fetch-base completion only replays the diff refresh if the user's
diff type/base selection is unchanged since the click — a slow fetch no
longer yanks the review back to the view captured at click time.
Declined with reasons (in review thread): first-run persist-before-confirm
(approved forced default, second flagging), explicit-base flag ordering
(early-set preserves user intent; commit-after-win would leave picker and
server disagreeing), base-picker revert target (near-unreachable compound
race, deferred).
* fix(review): PR-990 review round 17 — find origin/main on feature-only clones
getDefaultBranch's chain (origin/HEAD -> local main -> blind "master")
skipped the fetched remote-tracking ref entirely. On checkouts with no
origin/HEAD symref and no local main/master — CI checkouts, `clone
--branch feature`, extra worktrees — it guessed "master", the base
didn't resolve, getGitContext suppressed since-base, and the flagship
"All changes" view silently disabled itself for the whole session even
though origin/main was fetched and diffable. The startup upgrade can't
rescue it either (its guard reads the bogus "master" as a deliberate
non-default base).
Chain is now: origin/HEAD (verified) -> origin/main -> local main ->
origin/master -> "master" — remote-tracking refs preferred, matching the
function's stated prefer-upstream intent. Shared core (Pi inherits);
real-repo test reproducing the exact clone shape.
|
||
|
Michael Ramos
|
16db952000 |
fix(ui): stop unclosed math from pairing with a stray delimiter in a later code fence
The unclosed-$$/\[ guard scanned ahead to EOF for the closing delimiter, so an unterminated opener could match a $$ (or \]) that appears far below - e.g. inside a code fence - and swallow every heading/paragraph in between into one broken math block. The scan now stops at a blank line: real display math has no blank line before its close, so a blank both confirms "unclosed" and keeps the search from reaching distant delimiters. Adds a regression test and a manual LaTeX + media test fixture. |
||
|
Michael Ramos
|
cd934e6de2 |
fix(ui): stop stray math delimiters from mangling plan/PR text
Two math-parser regressions from #878, both the same "greedy delimiter" class
as the <video> fix in
|
||
|
Michael Ramos
|
ec551c83af |
fix(ui): stop unclosed <video>/<picture> from swallowing the document
An unclosed or self-closing <video>/<picture> ran the HTML-block scanner off the end of the document, funneling every following heading/table/code block into one opaque html block (gone from the TOC, unannotatable). The scanner now only extends the block when it actually finds the matching close tag; otherwise it keeps the block to the opening line. Also fixes multi-line <img> (was truncated to a bare "<img" fragment) and allowlists srcset/media/sizes so <picture>/<source> and responsive <img> render instead of rendering inert. Reachable via GitHub PR description/comment media (#981) and plannotator annotate on any .md file. |
||
|
Devin
|
d39bc871b7 |
feat(ui): render markdown math (#878)
* feat(ui): render markdown math * feat(ui): support more markdown math delimiters * feat(ui): renders spaced dollar-delimited TeX support * chore(deps): update katex * Update bun.lock chore(deps): update chore(deps): update chore(deps): update chore(deps): update chore(deps): update * feat(highlight): highlight math * feat(ui): support formule & text highlight together * fix(ui): make rendered math drag-selectable for annotation Drag-selecting an inline formula did nothing: the browser normalizes the selection focus to offset 0 of the following text node, so the strict anchor/focus check in selectionHasNonMathContent treated every inline drag as "mixed prose+math" and bailed without claiming the event. Control fell to web-highlighter, which excludes .katex/.math-annotatable, so no annotation was created and the selection just collapsed. Drop the anchor/focus check and rely on the existing clone-and-strip logic (leftover text => genuinely mixed), plus a guard for selections wholly inside one formula. Add a regression test that reproduces the real spilled-endpoint drag, which the prior empty-selection tests missed. Also paint a preview highlight the moment a formula's toolbar/popover opens (stripped on cancel, committed on submit) and show cursor: pointer on hover so formulas read as annotation targets. * chore(ui): reconcile lockfile and math styles after rebase onto main Rebase reconciliation, not new behavior: - Re-add katex to bun.lock. The lockfile conflicted on every replayed commit and was resolved to main's copy throughout; regenerate it here so package.json (katex) and the lockfile agree again. - Move the math annotation styles (.math-inline-annotation / .math-block-annotation) into packages/ui/theme.css. main relocated all annotation-highlight rules there to share them with the code-review description annotations; the PR had added the math variants to the old editor/index.css location. * test(ui): lazy-load highlighter hook so DOM-less bun test skips cleanly The math-annotation test statically imported useAnnotationHighlighter, which pulls in @plannotator/web-highlighter — a UMD bundle that reads `window` at module-eval time. Under the default `bun test` (no DOM), that threw a ReferenceError on import, failing CI before the skipIf(!hasDom) guards ever ran. Import the hook lazily, gated on hasDom, so the file loads and its tests skip cleanly in CI while DOM_TESTS=1 still exercises them. * fix(ui): stop display math from swallowing the document The `$$`/`\[` block parser only recognized the closing delimiter when it was the last non-space text on its line. A line like `$$E=mc^2$$.` (trailing period) or `$$E=mc^2$$ where E is energy.` therefore looked like an unterminated opener and consumed every following line until the next fence or EOF — silently hiding the rest of the plan. Detect the closing delimiter anywhere on the line and re-process any trailing text as its own line, so the equation renders and the following content is preserved instead of dropped. Same fix for the multi-line closing path and the `\[ ... \]` branch. Adds parser regression tests (these run in CI; no DOM). --------- Co-authored-by: ishowman <ishowman@users.noreply.github.com> Co-authored-by: Michael Ramos <mdramos8@gmail.com> |
||
|
Michael Ramos
|
e3de938914 |
feat(review): PR Overview panel + description/comment annotations + media (#981)
Combine the PR Summary/Comments/Checks tabs into one PR Overview panel, then make the description and comments annotatable and render their media. - PR Overview panel (one sidebar entry) + comment UI (avatars, filters, hide bots, live context, responsive stacking). - Annotate the PR description (select → comment) and PR comments (Annotate button), with Ask AI; notes show in the Annotations sidebar and ship to the agent. - Split/Unified diff toggle relocated into the dock tab strip. - Render images + video in descriptions and comments (raw HTML + markdown), capped to the card so nothing bleeds. - Review-flow fixes: copy-all feedback, prose-only feedback preamble, no image control on prose notes, GitHub review-body seeding; stronger review trailer. - Add Claude Sonnet 5 as the default Ask AI model. No server, endpoint, or Pi-runtime changes. |
||
|
Michael Ramos
|
ce5afdf94a |
feat(review): add "Copy agent instructions" for external review comments (#983)
Mirrors plan mode's agent-instructions feature in code review. Adds buildReviewAgentInstructions() — the clipboard contract teaching an external agent to read the diff via /api/diff and POST line/file/general scoped comments to /api/external-annotations (accurate against transformReviewInput). Wires a gated "Agent Instructions" item into the review header menu (shown only in a live session). Also drops the now-confusing cross-reference to code review from the plan instructions module. |
||
|
Michael Ramos
|
cb6667e991 |
fix(ai): drive Codex Ask AI via codex app-server (#971)
* fix(ai): drive Codex Ask AI via app-server + answer-first review prompts (#971) Codex Ask AI previously ran via @openai/codex-sdk (codex exec), which forces approval_policy=never and breaks in enterprise-managed Codex environments that ban it (#971). Replace the transport with a long-lived 'codex app-server' process over JSON-RPC. - New provider packages/ai/providers/codex-app-server.ts (registered as 'codex-sdk' to preserve cookie/agents.ts/UI-gate); omits approvalPolicy so Codex resolves the user's + managed policy, pins read-only sandbox, and surfaces interactive approvals through the existing PermissionCard. - Delete codex-sdk.ts and drop the @openai/codex-sdk dependency (and its 6 prebuilt platform binaries); gate registration on 'which codex'. - SessionManager: additive, optional dispose?() hook to kill the spawned process on evict/remove — a no-op for Claude/OpenCode/Pi (they don't implement it). Also rework the Ask AI prompts (all providers, separate from the transport): - Every mode now instructs the agent to answer the user's message directly and not launch an unprompted review of the context. - Code review stops pasting the whole diff for git-reproducible diff types and instead tells the agent how to inspect it (git diff <base>..HEAD, three-dot for merge-base); non-git/PR/workspace types still paste. - Claude gains the Bash tool so it can run git (still gated by approvals). - The UI passes diffType/base (session) and what the user is viewing (per question) into the context. Verified: full typecheck, full test suite (101 ai tests), and a live end-to-end smoke against codex app-server. * fix(review): pin AI approval card above the input/model bar Render pending approval cards just above the input + provider/model bar in both the document chat (DocumentAIChatPanel) and code-review AI tab, instead of at the top of the scroll, so the user sees them where they act. * fix(ai): harden Codex abort/cancel + add Ask AI Stop button Addresses code-review findings on the codex app-server provider: - turn/interrupt was sent as a notification (no id) so Codex ignored it and abort never took effect. It's now a proper JSON-RPC request. - Filter turn events/approvals by turnId and reject an aborted turn's stragglers, so a stopped turn can no longer leak output into — or prematurely finish — the next turn (ask-stop-ask race). - Guard listeners by query generation and end the drain loop on the abort signal, so a superseded/stopped turn can't touch the live one and abort returns promptly instead of waiting for turn/completed. - Handle abort during startup: once the turn id is known, interrupt it instead of running it in the background. - Add a sendAndWait timeout so a stalled (alive-but-unresponsive) process errors instead of hanging forever. - Drain stderr (stdio 'ignore') to avoid a pipe-buffer deadlock. Also add a Stop button to both Ask AI surfaces (plan/annotate DocumentAIChatPanel and code-review AITab via ReviewSidebar). It replaces Send while streaming and calls the hook's abort -> /api/ai/abort -> session.abort(); the hook already exposed abort but nothing surfaced it. * feat(ai): drive Codex models + reasoning levels from model/list - Codex provider fetches model/list at startup (throwaway app-server, like Pi/OpenCode) and populates the real models plus each model's actual supportedReasoningEfforts + defaultReasoningEffort. Replaces the hardcoded model list and the static AI_REASONING_EFFORTS (which mislabeled xhigh as 'Max' and omitted minimal). - AIProviderBar + AIConfigBar now show the selected model's real efforts and hide the control when a model reports none. xhigh is shown verbatim. - Fix: the Stop button now also appears in the populated code-review chat state (a prior edit missed the second GeneralInput due to indentation). * fix(ai): scope Claude Ask AI Bash to read-only git; clear stale approval cards on Stop - Claude Ask AI no longer auto-allows bare Bash (which ran arbitrary shell with no Allow/Deny prompt). Replace it with scoped read-only git rules (Bash(git diff:*), show, log, status, rev-parse, merge-base, ls-files). git reads auto-run so the agent can inspect large diffs itself; any other command (write git, arbitrary, or injected compound) falls through to the permission card. Keeps the git-inspect approach (large diffs don't fit in the prompt) while closing the auto-exec hole. - useAIChat.abort() now drops still-undecided permission cards: abort cancels them server-side, so leaving them visible was a dead Allow/Deny. * feat(ai): code-review Ask AI shares the agent-review prompt machine, delivered as user messages Code-review Ask AI built its own diff description (gitInspectInstruction) in the system prompt from just diffType+base, which was wrong for full-stack, hide-whitespace, untracked files, and PR worktrees. Replace it with the same machine the review jobs use, delivered on the user's messages. - Server: buildCurrentAiReviewContext() reuses buildAgentReviewUserMessageForTarget (contextOnly) for the current view and ships it as aiReviewContext in every diff payload (/api/diff + switch/PR/scope). Mirrored in the Pi server. - Client: review-editor latches aiReviewContext onto each question via the pure buildReviewContextPreamble (packages/ui/utils/aiPrompt.ts) and buildDefaultPrompt — full block on the first message / when the view changes (incl. after a provider switch via the !sessionId fresh-session check), a short reminder otherwise (never re-pastes a large diff). - context.ts: delete the duplicate gitInspectInstruction; code-review system prompt is now role-only. Providers untouched (provider-agnostic user message). - Tests: machine scenario gaps (plain PR, full-stack default, PR-worktree origin/<base> + stale-main warning, untracked mention, jj-evolog, workspace lines); composition (first/reminder, command/pasted, preamble ordering). * fix(ai): agentic remote-PR context, UTF-8 stream decode, real Stop on supersede/disconnect Addresses review findings on the Ask AI prompt work: - Remote PR without a confirmed local checkout no longer gets URL-only. The agent is told it's in a PR worktree that's being prepared, to verify the PR files exist before relying on them, and to diff with git diff origin/<base>... HEAD (URL fallback). Inform + trust the agent rather than pasting. Shared machine, so review jobs get the same framing. - Decode Codex stdout with a streaming TextDecoder instead of per-chunk toString(), so multi-byte UTF-8 split across chunks no longer corrupts into U+FFFD (matches the Pi provider). - Stop now actually stops the server turn: ask() awaits /api/ai/abort when a new question supersedes a streaming one (awaiting avoids racing the new query into session_busy), and the /api/ai/query SSE stream gains a cancel handler so tab-close/navigation aborts the turn too. Both reuse the existing per-provider session.abort(); factored a shared postServerAbort helper. * fix(ai): per-model reasoning effort, shared PR-checkout readiness, Stop-then-ask race, type hole - Reasoning effort is now tracked per model (a map keyed by model) instead of one global value, so switching to a model that doesn't support the prior level (e.g. xhigh) no longer posts a stale/unsupported effort that Codex rejects. Each model keeps its own level; nothing leaks across. - Extract resolvePoolCwd into packages/shared/worktree-pool.ts (ready/pending/ absent) and use it from both servers' resolvePRLocalCwd so the readiness rule can't drift. Fix the Pi Ask AI helper to ready-check like Bun, so a warming PR checkout no longer claims 'checked out at PR head' and misdirects the diff. - Stop-then-ask no longer races into session_busy: the abort promise (from Stop or a superseding question) is stashed and the next ask() awaits it before sending. Stop still kills the turn instantly; a follow-up just waits the one abort round-trip. - Declare aiReviewContext on the initial /api/diff response type. * fix(ai): surface real Codex error messages, skip transient retries, auto-deny permission escalations Validated against codex-rs: - Codex's ErrorNotification nests the text at params.error.message (TurnError.message); we read params.message → always 'Unknown error'. Read the nested field (top-level fallback) so auth/usage-limit/stream failures show their real, actionable text. - Skip transient error notifications (willRetry=true): Codex retries on its own and the turn continues, so surfacing them flashed a spurious failure before the real answer. - Handle item/permissions/requestApproval: respond {permissions:{}, scope:'turn'} — byte-for-byte Codex's own cancel response (codex_delegate.rs) — instead of 'Unsupported request'. Ask AI is read-only, so denying the escalation lets the turn continue sandboxed rather than failing. Interactive grant deferred. * refactor(ai): share AI provider/model config in one hook; aggregate paginated model/list - Extract useAIProviderConfig (packages/ui/hooks): one home for provider/model/ reasoning-effort selection — initial state, auto-resolve on capabilities load, per-model effort (no leak across models), and persistence. Both the plan and code-review apps now call it and only compose the session reset (the hook can't own reset without a cycle through useAIChat). Plan editor adapted to the effect-based resolve (adds aiDefaultProvider state). This fixes the plan editor's stale-effort-on-model-switch bug by construction and stops the two apps' copies from drifting again. - fetchModels now follows model/list's nextCursor and aggregates every page into one list, so larger model catalogs aren't silently truncated (with a page guard against a misbehaving cursor). Skipped per discussion: legacy v1 approval methods (we're a v2 client). * feat(agents): per-agent review default; bound Codex model discovery so it can't stall the AI panel - The selected review profile is now tracked per review engine (claude/codex/ cursor/opencode) instead of one flat global value, so each agent keeps its own review default. Public hook API (reviewProfileId/setReviewProfileId) is unchanged — getter derives the current engine's value, setter writes it — so AgentsTab needs no changes. One-shot migration seeds every engine with any existing flat pick. Adds parseReviewProfileByEngine + tests. - fetchModels (Codex model discovery) now uses a short 6s timeout for its initialize + model/list RPCs instead of the 30s default. /api/ai/capabilities awaits model discovery, so an installed-but-unauthenticated codex could otherwise block the AI panel for ~30s; it now falls back to the static model list fast. Authed codex is unaffected (discovery completes in tens of ms). |
||
|
Michael Ramos
|
740d6fb2eb |
Add WebTUI agent panel to annotate mode (#941)
* feat(annotate): add WebTUI agent terminal * feat(annotate): wire WebTUI agent into annotate UI * docs: recap annotate agent terminal work * fix(annotate): harden agent terminal runtime * docs: add annotate agent terminal runtime ADRs * fix(annotate): polish agent terminal integration * fix(ui): preserve comment draft on Ask AI failure * fix(annotate): address terminal review findings * fix(annotate): harden agent terminal runtime fallback |
||
|
Michael Ramos
|
2a6fe5c457 |
Open files in external apps + code-review UX pass (#942)
Adds a server-side "open the current file in an external app" control to the code-review and annotate surfaces (split-button with host-detected apps; last-used becomes the default; cross-platform launch mirrored across the Bun and Pi runtimes), plus a code-review UX pass: file-header change letters and line counts, the diff-settings cog grouped with Split/Unified, an all-files collapse/expand-all toggle, and the semantic diff moved to a resizable sidebar accordion. Also fixes Cmd+click code navigation (worker token transformer). Hardened over three adversarial review rounds: shared, tested path-containment for open-in (Bun + Pi), annotate scoping aligned to /api/doc reference roots, strict PR-checkout rooting (never the launch repo), deduped app-catalog and semantic-diff fetches, macOS bundle-only availability, Windows launch fixes (reveal + terminal off cmd's parser), and PR-checkout tracking across switch and pool warmup. |
||
|
Michael Ramos
|
be2d06a7c2 |
Make HTML annotations render HTML by default
* feat(annotate): render html files by default * fix(annotate): support raw html assets and sharing * fix(annotate): address html first review followups * fix(editor): avoid raw html sidebar init crash * fix(annotate): support portable html shares * fix(annotate): harden html share support * fix(share): clear attachments when loading shared payloads * fix(share): warn on remote share link failures * perf(annotate): lazy-build html share payloads * test(annotate): guard lazy html share generation * test(annotate): drop flaky html share server test |