docs(document-viewer): add the Phase 3 execution handover - #1765
Conversation
… helper) Resolves the Codex P2 review finding on PR #1754: the authorizationHeader lowercase trap was recorded as issue #286 in the open items but not added to the recommended execution queue, so session-start and /issues summaries would not schedule it. Added as queue position 113 at A2 level, scoped to the next auth module touch.
Phases 0-2 of the viewer redesign merged as PR #1741; Phase 3 was never started, so the viewer still rasters one page at a time and every page flip on a long guideline is a cold render. This brief scopes Phase 3 to every capability except crop -> page overlay, which is excluded because `bbox` is absent from `DocumentDetailImage` and plumbing it crosses into `src/lib/**document**` -- a path that trips `clinicalRiskPatterns` and forces a governance preflight. Toolbar density is recorded as already shipped in Phase 2. It also corrects ledger #279, which claimed the viewer canvas cannot be gated in a browser and proposed bumping Playwright or pinning `pdfjs-dist` down. Measured: pinned `playwright@1.62.1` expects Chromium 151.0.7922.34, this container ships 141.0.7390.37, and CI runs HeadlessChrome/151.0.0.0. `pdfjs-dist@6.2.108` calls `Map.prototype.getOrInsertComputed`, which ships in 151 and not 141, so the failure is container-only and neither remedy is needed. All line references verified against main at 8db1e53. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe PR adds a Phase 3 document-viewer handover plan. It documents rendering, virtualization, keyboard controls, verification, governance, signed-URL constraints, review-ledger updates, and outstanding issues. ChangesDocument viewer Phase 3 handover
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:b8c94dff23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/branch-review-ledger.md`:
- Line 810: Update the verification statement in the ledger entry to say “all
ten applicable gates completed; executable-code checks skipped by the docs-only
risk router,” and append the specific skipped check names if they are available
from the PR objectives or surrounding record.
In `@docs/plans/document-viewer-phase3-handover.md`:
- Around line 143-150: The Phase 3 plan must make the signed-URL fixes mandatory
before Task 3: require the dedupe key to use the lowercase authorization header,
move cache writes outside the shared promise so superseded responses cannot
repopulate cleared entries, and add regression tests for both behaviors before
modifying use-signed-image-url.ts. Track the cache-write fix separately if issue
`#286` covers only header casing.
- Around line 7-11: Update the Phase 3 scope and Task 3 references to resolve
the governance mismatch: either defer signed-URL API batching, or explicitly
list src/app/api/images/signed-urls/route.ts as a scope exception and require
the governance preflight triggered by clinicalRiskPatterns. Apply the same
clarification to the corresponding sections noted in the review.
- Line 146: Update the inline code span in the dedupe-key example to remove the
trailing space, changing the referenced key from `endpoint ` to `endpoint` while
preserving the surrounding text.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6f0bad5-62da-4303-8bbf-82b191fa80e8
📒 Files selected for processing (3)
docs/branch-review-ledger.mddocs/outstanding-issues.mddocs/plans/document-viewer-phase3-handover.md
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Resolves the duplicate #286 that made the PR un-mergeable. Main's in-page-nav series claims #286-#288; this branch independently created its own #286 for the authorizationHeader casing trap. Kept both sides and renumbered the branch's row to #289, bumping issues:next-id to 290. The auto-merge silently dropped the branch's #286 detail row rather than conflicting on it -- only the recommended-queue hunk raised a marker. That row is restored as #289. This is the silent row-loss mode archive row #112 describes: the guard catches duplicate ids but not a vanished one. Also addresses review findings on the handover brief: - Require the Clinical Governance Preflight for Phase 3 regardless of what scripts/pr-policy.mjs classifies. AGENTS.md:257 keys the requirement to behaviour -- source rendering and document access -- and Phase 3 changes both. The classifier not matching src/components/** is the PR #1489 gap recorded at pr-policy.mjs:62, not an exemption. - Resolve the scope contradiction: the brief claimed everything stays in src/components/document-viewer/** while Task 3 authorised wiring src/app/api/images/signed-urls/route.ts, which is clinical-risk. That is now an explicit, discouraged scope exception. - Correct the signed-URL warning. Both identity bugs are already fixed on main (authorizationIdentity() helper, identity in the dedupe key, cache write outside the shared promise, tests/auth-signed-url-cache.dom.test.tsx). The brief now says so and requires that coverage stay green, rather than reading as an open disclosure with a "consider" remedy. - Drop a literal NUL byte that had landed in the prose, and remove the trailing-space code span (MD038). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo
…cope Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/branch-review-ledger.md`:
- Line 820: Update the line containing the `pr-handoff-stop` failure in the
ledger entry to replace the bare `#286` with a stable reference, such as the
issue title or both the historical failure identifier and current issue
identifier. Preserve the existing test-result context and distinguish it from
the in-page-navigation issue currently associated with `#286`.
- Line 828: Synchronize issue `#279` with its durable disposition: update the
corresponding entry through the /issues workflow to record the required CI
compatibility check or Playwright/pdfjs version change, or explicitly state in
the handover row that it is not authoritative for `#279`’s disposition. Use the
issue `#279` entry and the handover record as the target symbols.
In `@docs/outstanding-issues.md`:
- Around line 166-169: Regenerate the outstanding-issues queue from the complete
open-row set so it includes every open issue, including `#258`, `#260`–#262,
`#264`–#270, and `#271`–#285, rather than jumping from `#257` to `#286`. Update the
queue section while preserving its existing row format and completeness claim.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82f2941b-0ea6-4158-9bc8-e01516abec83
📒 Files selected for processing (3)
docs/branch-review-ledger.mddocs/outstanding-issues.mddocs/plans/document-viewer-phase3-handover.md
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The brief told the next session to close#279 with `ledger:append --supersede`. That is the wrong tool and the wrong file: #279 lives in docs/outstanding-issues.md, so it closes through `issues:done` / `issues:update`, while `ledger:append` writes the separate branch-review ledger and would have left the durable row untouched. Also updates #279 itself, which still carried the refuted remedy (bump the pinned Playwright build, or pin pdfjs-dist down). Neither is needed: pinned playwright@1.62.1 expects Chromium 151.0.7922.34, this container ships 141.0.7390.37, and CI runs HeadlessChrome/151.0.0.0. pdfjs-dist 6.2.108 calls Map.prototype.getOrInsertComputed, which ships in 151 and not 141, so the raster failure is container-only. Recording that on the row stops any session acting on the refuted fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo
The preamble claimed the queue is 'the ordered view of every open ledger row'. It is not: 116 queue rows against 143 open rows, because the queue deliberately omits deprioritised-but-open items. The claim made the queue look lossy when it was working as designed. Caught in PR #1765 review. My first rebuttal was wrong -- I cited the sentence about post-verification removals, which is about a different thing, and missed the completeness claim one line above it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo
Summary
docs/plans/document-viewer-phase3-handover.md— an execution brief for Phase 3 ofdocs/plans/document-viewer-redesign-plan.md. Phases 0–2 merged as PR feat(document-viewer): rework the viewer for phone and PWA reading #1741; Phase 3 was never started, so the viewer still rasters one page at a time and every page flip on a long guideline is a cold render.#279indocs/outstanding-issues.md, which carried a refuted remedy.docs/outstanding-issues.md, which falsely claimed to list every open row.docs/branch-review-ledger.md(append-only, with a superseding row).Scope is every Phase 3 capability except crop → page overlay. That exclusion is deliberate:
bboxis SELECTed atsrc/lib/document-detail.ts:441but absent fromDocumentDetailImageinsrc/lib/document-detail-contract.ts, so plumbing it is a wider contract change than this phase should carry.Correction to
#279#279claimed the viewer canvas cannot be gated in a browser and proposed either bumping the pinned Playwright build or pinningpdfjs-distdown. Both would have been unnecessary dependency surgery. Measured:/opt/pw-browsers/chromium-1194)playwright@1.62.1expects (browsers.jsonrev 1234)lighthouse-budget.json:27)pdfjs-dist@6.2.108callsMap.prototype.getOrInsertComputedpdf.mjs:2454, 6889, 6896getOrInsertComputedships in Chromium 151 and not 141, so the raster failure is container-only — CI's browser already runs pdf.js 6. Writing that gate is Task 0 of the brief, and#279now records the measurement with an explicit "do not bump Playwright, do not pinpdfjs-distdown".Verification
npm run verify:pr-localclassified this as recognised low-risk documentation scope and completed the ten applicable gates with none failed:The risk router skipped lint, typecheck, the full unit suite, RAG fixture validation, and build — no executable product code changed.
docs:check-linkspassed with 1688 repo path references resolving, which is what validates the ~40 file/line citations in the brief.check:outstanding-issuespassed: 287 rows, unique ids,next-id=290, and no ids deleted from base — the check that caught the silent row loss described below.git merge-tree --write-tree origin/main HEADexits 0.mainat50ef12eafter rebasing off a 64-commit-stale base; the viewer files were unchanged across that range.Risk and rollout
Documentation only — no product, test, config, or dependency changes, and nothing here reaches a runtime path. Rollout is the merge itself; rollback is
git revert. The brief instructs future work rather than performing any, so it carries no behavioural risk of its own.Clinical Governance Preflight: not applicable to this PR.
classifyPullRequestFilesreturnsclinicalRisk: falsefordocs/**, and the behavioural trigger inAGENTS.md:257does not fire either — no ingestion, answer generation, search/ranking, source rendering, document access, privacy, production config, or clinical output changes here. Note that the brief itself requires the preflight for the Phase 3 implementation work, which does change source rendering and document access.RAG impact: no retrieval behaviour change — no protected ranking surface is touched (
src/lib/rag/**, clinical-search, retrieval-selection, ranking-config, answer-ranking, the eval harness, the golden fixture, the retrieval RPCs).Notes
#286:main's in-page-nav series and this branch'sauthorizationHeaderrow independently claimed the same id. Kept both, renumbered the branch's to#289. The auto-merge silently dropped the branch's detail row rather than conflicting on it — only the queue hunk raised a marker. That row is restored; it is the failure mode archive row#112documents, where the guard catches duplicate ids but not a vanished one.Summary by CodeRabbit
#289, and corrected issue#279to clarify container-only PDF compatibility concerns and follow-up coverage.