Skip to content

fix: three release-QA findings — PATCH validation, dark-theme printing, and the root vendor/ exclusion - #1564

Merged
backnotprop merged 5 commits into
mainfrom
fix/qa-028-highs
Sep 18, 2026
Merged

backnotprop merged 5 commits into
mainfrom
fix/qa-028-highs

Conversation

@backnotprop

Copy link
Copy Markdown
Owner

Three release-QA findings from round 028, one PR.

1 — HIGH: PATCH /api/external-annotations could blank the page (new in #1560)

Finding. PATCH merged its body into the stored annotation verbatim in BOTH
runtimes (packages/server/external-annotations.ts, the Pi mirror); only id
and source were pinned, by the store. So any local process could store a
value POST refuses, and {"diagramAnchor": null} was one: DiagramBlock read
.family off it during render and the whole document went blank.

Repro, before (pre-fix build, real server + browser):

PATCH {"diagramAnchor": null}  -> 200, stored as null
page:  TypeError: Cannot read properties of null (reading 'family')
       document.body.innerText.length === 0, #root childElementCount === 0

After, same script:

PATCH {"diagramAnchor": null}  -> 400 {"error":"invalid \"diagramAnchor\": must not be null"}
page:  textLength 911 (unchanged), 3 diagrams, zero page errors, stored anchor intact

"nope", 7, {}, {v: 2, …}, an unknown family, htmlAnchor: null,
elementContext without a tag, a bad type and a non-string text all
answer 400 too; a legitimate {"text": …} edit still works and unknown keys
are dropped.

Fix. Two layers.

  • validateAnnotationPatch in @plannotator/core/external-annotation — the
    module both handlers already import — allowlists and field-validates the
    patch with the SAME validators POST applies: diagramAnchor through
    parseDiagramAnchor, htmlAnchor / elementContext / the target arrays
    through their own fail-closed parsers, inReplyTo through the existing
    validateReplyTarget, images / type / text / originalText /
    pageUrl by type and cap. Unknown keys are dropped (the wire shape is
    additive), id and source stay immutable, and null clears an optional
    field but is refused on an anchor or a structural one. Review mode keeps its
    own field set. Wired into the Bun handler and the Pi mirror; html-anchor is
    now vendored to Pi.
  • Every renderer read of diagramAnchor is nullish-safe (!= null, ?.family)
    in DiagramBlock and Viewer, so no ingest — API, draft or share link — can
    make a render throw.

Tests. HTTP-level validator tests in both runtimes' external-annotations
test files (null, string, number, {}, wrong v, unknown family, oversized
label capped by the parser, negative source line, the inReplyTo cycle that
was already covered, unknown-key drop, non-object body, review-mode fields),
and a DOM test —
packages/ui/components/Viewer.diagramAnchorHostile.test.tsx, added to the CI
allowlist — that a stored row with diagramAnchor: null (plus five other
malformed shapes) renders the document and lists in the panel instead of
throwing. It fails on the pre-fix component with the reported TypeError.

2 — HIGH: dark-palette diagrams printed illegibly (from #1556/#1557 theming)

Finding. print.css assumes white paper (body, div, span, p, … { color: #1a1a1a !important }), but the page kept its own dark tokens. Mermaid 12 draws
node and edge labels as real HTML inside <foreignObject>, so the blanket rule
caught them: near-black text on a near-black node fill. The dark-palette
flowchart printed as empty boxes.

Fix (owner ruling: "on dark theme, just print a white page"). Printing
renders the light half of the user's pair, one rule, no diagram-specific
fill hacks:

  • ThemeProvider forces preferredMode to light while printing. The class
    write happens inside the beforeprint handler, because a real print snapshot
    is taken before React would flush a state update; a matchMedia('print')
    change drives the same switch for headless emulation and preview, where the
    async Mermaid re-render also lands. Graphviz follows with no re-render at all
    (it paints var(--*) tokens). The stored preference is never written.
  • print.css exempts diagram content from its typography rules
    (:not([data-diagram-block] *)): a diagram colours itself and its own
    <style> has no !important to defend itself with.
  • usePrintMode shares the subscription, so .plannotator-print applies under
    print emulation too. fix(ui): load the diagram engine lazily, and keep the canvas controls out of the diagram; ui 0.41.1 #1562's data-print-hide on the zoom strip is untouched.

Proof (headless Chromium, emulateMedia({media:'print'}) + page.pdf(), on
a fixture with a flowchart, a sequence diagram and a dot fence, in
plannotator dark and one-light). Every diagram label is paired with the
shape actually behind it and scored for WCAG contrast:

build palette print theme labels below 4.5:1 min
before plannotator (dark) theme-plannotator 23 15 1.01
before one-light theme-one-light light 23 0 9.43
after plannotator (dark) theme-one-light light 23 0 9.43
after one-light theme-one-light light 23 0 9.43

Light-mode users see no change. PDFs and screenshots saved for both builds.

Tests. ThemeProvider.test.tsx gains a print describe: beforeprint flips
to the light half and afterprint restores; the light tokens are on <html>
before act flushes (the real Cmd+P ordering); the print media query drives it
too; a light-mode user sees nothing change. The shared matchMedia stub now
returns one query per media string.

3 — MEDIUM: hole in #1559's root-only exclusion

Finding. The origin-file exemption skipped a directory exclusion whenever
the origin path contained that segment anywhere, which lifted it tree-wide: a
request from src/main/java/com/example/vendor/app/Widget.java also returned
matches from the repo-ROOT vendor/.

Fix. Narrowed to the directory INSTANCE the origin lives in — a root-only
glob is only lifted when the origin's FIRST segment is that directory (a deep
same-named package was never pruned by it anyway), plus a pure post-filter
(isCodeNavPathAllowed) so an always-ignored name lifted for one
node_modules no longer returns matches from a different one.

Repro (live review server on the QA agent's three-file repo):

origin matches
src/main/java/com/example/vendor/app/Widget.java package file + neutral file, root vendor/ absent (was present)
src/other/Other.java package file + neutral file
vendor/RootVendorFile.java its own file + the rest — a genuine root-vendor file still finds its siblings

Tests. The #1558 test that encoded the bug is replaced by its inverse, plus
unit tests for the predicate and two real-ripgrep cases on the QA repro
(a neutral third file was added to the temp fixture). Module is vendored to Pi
by vendor.sh.

Verification

bun run typecheck; full bun test (4768 pass, 0 fail);
DOM_TESTS=1 bun test --isolate packages/ui/components packages/ui/hooks packages/editor
(1024 pass, 0 fail) plus the DOM allowlist test;
bun run --cwd packages/ui smoke:package; viewer rebuild + check:manifest
(repinned — the print and diagram changes moved both hashes); review + hook
builds. Every server run with PLANNOTATOR_BROWSER=/usr/bin/true and headless
Playwright.

…d diagramAnchor defensively

PATCH merged its body into the stored annotation verbatim in both runtimes,
so any local process could store a value POST refuses. `{"diagramAnchor":
null}` was answered 200 and the SSE broadcast then blanked the open page:
DiagramBlock read `.family` off it during render.

Two layers:

- `validateAnnotationPatch` in @plannotator/core/external-annotation (the
  module both handlers already import) allowlists and field-validates the
  patch with the same validators POST applies — diagramAnchor through
  parseDiagramAnchor, htmlAnchor / elementContext / the target arrays through
  their own parsers, the scalars by type and cap. Unknown keys are dropped,
  `id` and `source` stay immutable, `null` clears an optional field and is
  refused on an anchor or a structural one.
- The renderer no longer trusts the field: DiagramBlock and Viewer read it
  with `!= null` / `?.`, so no ingest — API, draft or share link — can make a
  render throw.
…package of the same name

The origin-file exemption (#1558) lifted a directory exclusion whenever the
ORIGIN path contained that segment anywhere, which lifted it tree-wide: a
request from `src/main/java/com/example/vendor/app/Widget.java` also returned
matches from the repo-root `vendor/` third-party tree it exists to exclude.

Narrowed to the directory INSTANCE the origin lives in:

- a root-only glob (`!/vendor`) is only lifted when the origin's FIRST segment
  is that directory — a deep same-named package was never pruned by it anyway;
- `isCodeNavPathAllowed` post-filters every result, so an always-ignored name
  lifted for an origin under one `node_modules` no longer returns matches from
  a different one.

A file that really lives under an excluded root still finds its own siblings.
… prints on white paper

The print stylesheet has always assumed white paper: it paints the ground
white and the text near-black. A dark-palette page kept its own tokens under
it, and #1560's diagram engine made that visible — Mermaid 12 draws node and
edge labels as real HTML inside `<foreignObject>`, so the blanket
`div, span, p { color: #1a1a1a !important }` repainted them near-black on a
near-black node fill and the flowchart printed as empty boxes.

- ThemeProvider renders the LIGHT half of the user's pair while printing. The
  class write happens inside the `beforeprint` handler, because a real print
  snapshot is taken before React would flush; the `print` media query drives
  the same switch for headless emulation and preview, where the async Mermaid
  re-render also lands. The stored preference is never touched, and a
  light-mode user sees no change.
- print.css exempts diagram content from the typography rules
  (`:not([data-diagram-block] *)`): a diagram colours itself, and its own
  `<style>` has no `!important` to defend itself with.
- usePrintMode shares the same subscription, so `.plannotator-print` is
  applied under print emulation too.
@backnotprop
backnotprop merged commit 865d679 into main Sep 18, 2026
28 checks passed
@backnotprop
backnotprop deleted the fix/qa-028-highs branch September 18, 2026 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant