Skip to content

Code review · P3 G4, the Paper shell in web/

review-code on autopilot/P3-G4, base 330ae6b, 2026-10-30. The step is P2 §10.5 G4: the Paper shell of ui-spec §7 rewritten as Spectio code, after the G1 boards, with the rules of inbox #89. It adds:

  • web/src/shell/: AppShell.tsx, TopBar.tsx, Sidebar.tsx, TabBar.tsx, theme.ts and shell.css, which replace Task 19's thin shell in routes.tsx and most of app.css;
  • web/src/shell/shell.test.tsx: the four tests P2 names, and one test per review fix;
  • static.py: Referrer-Policy and Cross-Origin-Opener-Policy on every screen answer (SEC-P3T19-03, from Task 19's review), with a test in test_web.py;
  • the app-launch shot in light and dark, and a new app-launch-phone pair (tools/media/live.js, shots.py);
  • inbox #89.

The reviewers were code-reviewer and security-reviewer, independent and in parallel. Format: README.md. The rules are owned by the ui-spec §7 note of 2026-10-30 (the shell) and dev-env §15.3 req. 17 (the headers), both changed by the docs commit.

Round Head code-reviewer security-reviewer
1 b19961a (the step and its shots) changes (1 major, 4 minors) approve (1 minor)
2 4e0d286 (a malformed percent sequence keeps the shell) approve (4 minors) approve (2 minors)
3 7c5737c (the phone pill stays neutral while connecting) approve (1 minor) approve (no finding)
simplify 7c5737c code-simplifier: no change

The reports were relayed by the coordinator in summary. Finding IDs restart each round: CR-P3G4-03 (r1) and CR-P3G4-02 (r2) are the same defect, and the test-output noise is CR-P3G4-02 (r1), CR-P3G4-03 (r2) and CR-P3G4-03 (r3). This record gives each its round.

Test-first. From the worker's report:

  • CR-P3G4-01 (r1): a malformed percent sequence keeps the shell (CR-P3G4-01) renders /runs/%E0 and checks that the last crumb is %E0 with aria-current="page".
  • CR-P3G4-02 (r2): the phone node pill stays neutral while connecting (CR-P3G4-02) sets the width to 390 px, stubs a fetch that never resolves, and checks that the pill named "Connecting" has no data-down. It failed before the fix with data-down="true".

Gate at 7c5737c, from the worktree, re-run by doc-writer:

uv run --locked pytest -q             # 504 passed, 15 deselected
uv run --locked ruff check .          # All checks passed!
uv run --locked ruff format --check . # 80 files already formatted
uv run --locked mypy packages         # Success: no issues found in 79 source files
uv run --locked lint-imports          # Contracts: 4 kept, 0 broken.
uv lock --check                       # Resolved 55 packages
cd web && npx vitest run              # 5 files, 24 tests passed
cd web && npx tsc --noEmit            # no output, exit 0
cd web && npm run build               # dist/index.html and two hashed assets

The web tests are not part of the uv gate. CI runs them in its own job, web.

CODE  step=P3-G4  review=approve b/M/m=0/1/9  security=approve b/M/m=0/0/3  rounds=3  simplify=nochange  tests=pass

The counts add up the three rounds per reviewer, repeats included.

Findings

Id Severity Finding Status
CR-P3G4-01 (r1) major A malformed percent sequence in the URL (/runs/%E0, /runs/%) made the root shell throw a URIError, so the whole app went blank. TanStack's decodePath (router-core utils.js:177-190) leaves invalid sequences undecoded, then crumbsFor called decodeURIComponent on them. TopBar renders inside the root route, so the default error screen replaced the shell and the reader had no navigation left. fixed in 4e0d286: label() falls back to the raw segment; the decode stays for the %25 and %5C the router keeps encoded
SEC-P3G4-01 (r1) minor The same double decode, seen as a link attack: a crafted link such as /runs/%ZZ blanks the page (client-side denial of service). The raw segment is also the current crumb, so a link can put text of the sender's choice into the chrome. No data is exposed, and React escapes the text. the crash is fixed in 4e0d286; the spoofing is SEC-P3G4-02 (r2), open
CR-P3G4-03 (r1), CR-P3G4-02 (r2) minor On the phone, the node pill showed the failed (red) colour while GET /version was still loading, because data-down was keyed on !server. The sidebar keys it on !!down, the fetch error (Sidebar.tsx:72), so the two indicators disagreed on every page load. fixed in 7c5737c: TopBar.tsx:119 keys data-down on !!down, as the sidebar does
CR-P3G4-02 (r1), CR-P3G4-03 (r2), CR-P3G4-03 (r3) minor Test output noise. The shell tests render the real router, whose scroll restoration calls window.scrollTo, which jsdom does not implement. npm --prefix web test prints "Not implemented: Window's scrollTo() method" once per navigation: 6 lines in r1, 7 in r2, 8 in r3 (the new test adds one). The tests at the base never rendered the router. open: follow-up FU-P3-23, with P1 Task 20's tests
CR-P3G4-04 (r1) minor Requirement (5) of the step says the top bar shows the server's node, as Task 19's did. On desktop and tablet it is in the sidebar foot only, as on LiteTopbar and LiteSidebar (G1). On the default tablet rail the foot text is .sp-sr, so a sighted reader at 768–1199 px cannot see the node without unfolding. Inbox #89 did not record this. recorded by the docs commit: inbox #89 rule 7 and the ui-spec §7 note. Whether the rail shows the node is left to Thomas with #89
CR-P3G4-05 (r1) minor The phone node pill cuts a long node name with an ellipsis. The full name is only in aria-label, so a sighted reader cannot see it. Same concern as UX-G3a-06; it matches the MLiteTopbar board. open: with UX-G3a-06, P1 Task 20 (ui-spec §4.11: a tap on a truncated title opens a details sheet)
CR-P3G4-04 (r2) minor On the phone, server reachability is no longer a live region. Task 19's top bar used role="status"; the phone pill is role="img", so a change from "Connecting" to online or "not reached" is not announced. The desktop sidebar foot still uses role="status". open: follow-up FU-P3-23
CR-P3G4-05 (r2), SEC-P3G4-01 (r2) minor The app-launch shots show "LOCAL · server on vm · your account root" in the sidebar foot, because the capture server ran as root. The docs therefore show Spectio running as root, unlike the boards (amartin), and they name the capture host's account. The reviewer reads dev-env §15.3 as ruling out a root server; this page found no line in §15.3 that says so [to be validated]. The hygiene check misses it because tools/media/fixtures.yaml forbids only /root\b. open: media-capture retakes app-launch and app-launch-phone as a non-root demo account, and the check forbids the account root; follow-up FU-P3-23
SEC-P3G4-02 (r2) minor The breadcrumb shows any decoded URL segment as the aria-current page label, with no length limit and no format check. A link like /runs/Server%20compromised%2C%20run%20%E2%80%A6 puts attacker-chosen text into the shell's chrome. React escapes it, so there is no XSS; it is content spoofing. Task 19's Run ${id} title already did this; G4 adds a second, more visible place. open: P1 Task 20, which shows the run's name from GET /runs/{id} in place of the raw segment

Fixes by round

Commit Findings
4e0d286 fix(web): a malformed percent sequence no longer blanks the shell CR-P3G4-01 (r1), the crash of SEC-P3G4-01 (r1)
7c5737c fix(web): the phone node pill stays neutral while connecting CR-P3G4-03 (r1) = CR-P3G4-02 (r2)
docs commit of this step CR-P3G4-04 (r1), recorded in inbox #89 rule 7 and the ui-spec §7 note

code-simplifier made no change. It left these alone, with reasons:

  • shell.css keeps .sp-page-title, .sp-page-sub and .sp-table [data-wide], which no component uses yet. They are for Task 20 (inbox #89 rule 6, UX-G3a-05 and UX-G3a-06).
  • theme.ts defines ok-rail, accent, accent-subtle and ink-0, which nothing reads yet. They copy design/tokens/spectio-tokens.css.
  • AppShell.tsx:30-31: the ?? false after useMediaQuery is needed, because Mantine 7.17.8 types the result as boolean | undefined.
  • Sidebar.tsx and TabBar.tsx loop over the same links with different classes, icon sizes and labels; a shared helper would add a concept.
  • The desktop search button's aria-label keeps "⌘K" out of its name.

For the Progress page

The G4 section of P3-M4 is written: P3-M4 · G4, with its findings and open questions.