feat: improve document viewer section navigation - #102
Merged
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:a879c65258
ℹ️ 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".
| </div> | ||
| <details className={cn("group lg:hidden", panel)}> | ||
| <details id="source-text-mobile" className={cn("group scroll-mt-24 lg:hidden", panel)}> |
There was a problem hiding this comment.
When viewed on mobile, the new Text nav item points at this closed <details> element. Fragment navigation scrolls to the disclosure but does not set open, so the indexed text/search panel remains hidden; the smoke assertion only sees the summary label. Users jumping back from the PDF still need an extra, non-obvious tap to reveal the source text, so please open the disclosure for #source-text-mobile or target an always-open element.
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo pushed a commit
that referenced
this pull request
Jul 29, 2026
…iew batch Eight review findings (1 Codex P2, 7 CodeRabbit), each verified against the code before acting. MOST CONSEQUENTIAL — the L2-3 "recall is byte-identical" claim was wrong, and it was load-bearing. fetchDocumentTitleAliasRows (rag-candidate-sources.ts:482) applies .limit(12) with no ORDER BY, so which twelve documents return is plan-dependent; a new index can change the title-alias set feeding candidate assembly. "No query text changes" is true, but recall does not follow from it. That claim was the argument for keeping L2-3 out of canary territory, so the gating is revised: the documents-list and (status,id) uses stay ordering-safe, the RAG-path index is canary-gated unless the unordered .limit(12) is made deterministic first — the cheaper fix, since an unordered LIMIT is latent nondeterminism regardless of this work. Operator SQL alone never reaches staging, DR, or local replay: migrations/ is the source of truth and schema.sql only a mirror, so hand-run statements hit the live database and nothing else, and a required_indexes registration would fail on every replayed environment. Authoring the migration is now a required part of #102, following the 20260717170000 idempotent pattern. This PR still ships no migration (the #1312 objection), but the runbook no longer implies the operator sequence is sufficient. Test guard hardened: the ordering case anchored on a fixed 5 ms sleep, which can expire before the handler reaches the limiter. It now waits for consumeSubjectApiRateLimit to be entered, then asserts scope is untouched — the same guarantee without the timing fragility. Ledger: the 78e2beb record still described the reverted scope-vs-ratelimit overlap. The ledger is append-only, so this appends a superseding record via ledger:append --supersede rather than editing the row, per the contract. Status wording: #102 is "runbook prepared", not done, while the operator steps are pending; #105 separates shipped implementation from pending browser verification; #98/#99/#102/#103/#105 restored to the execution queue with their remaining actions. Fixed the MD038 malformed RAG-impact code span. Verification: verify:cheap exit 0 (427 files, 4386 passed / 4 skipped); check:branch-review-ledger pass (1232 records, no duplicates). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
BigSimmo pushed a commit
that referenced
this pull request
Jul 29, 2026
Resolved docs/outstanding-issues.md by keeping this branch's updated #98, #102, #104 and #105 entries (the ones this PR rewrites) and adding main's new #108 and #109 rows; the rest of the conflict was prettier re-padding column widths after a summary-cell length change. next-id stays at 110. Removed four exact-duplicate branch-review-ledger rows produced by the union merge driver keeping both copies of rows present on this branch and on main - the duplication risk tracked as #88.
15 tasks
BigSimmo pushed a commit
that referenced
this pull request
Jul 30, 2026
Codex review on this PR caught that docs/outstanding-issues.md #102 still called documents_status_id_idx ordering-safe after this PR's own runbook correction reclassified it as canary-gated (its rag-candidate-sources.ts consumer shares the RAG-hazard mechanism with the trigram index; only the search-scope.ts ordered consumer stays safe). Mirrors the runbook's exact corrected wording so an operator following the ledger entry alone doesn't retain an unguarded index.
BigSimmo added a commit
that referenced
this pull request
Jul 30, 2026
…ottom refusal guard non-vacuous (#1423) * docs(operator): correct the mislabelled ordering-safe documents index My error, live in a merged runbook. The 2026-07-29 correction identified the unordered LIMIT 12 in rag-candidate-sources.ts:482 as the RAG hazard, then attributed it only to the trigram index and listed documents_status_id_idx as ordering-safe. Re-reading the statement it describes: query = query.or(filters).eq("status", "indexed").limit(12); One statement carries both the status equality and the unordered LIMIT 12, and (status, id) is exactly the index serving that equality. So the mechanism the doc already documents applies to it verbatim: a new plan can return a different twelve title-alias rows into candidate assembly. It is canary-gated. The generalisation that produced the error is worth naming, because it is the trap: (status, id) has two consumers. search-scope.ts:271-277 pages with an explicit .order("id") and is genuinely safe; rag-candidate-sources.ts:482 is unordered and is not. Safety of one consumer is not transitive to the other. Also marks the SQL block itself, which is what an operator actually copies. The prose gated the indexes while the block still listed all three together with no distinction, so a copy-paste run would apply the canary-gated ones regardless. Gates: prettier clean; docs:check-links 1381 references resolve; docs:check-scripts 395 npm-run references resolve. No code or SQL applied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF * test(chrome): make the calculator near-bottom refusal non-vacuous Follow-up to the #1396 review. The overlay branch of this test was moved until it passed, and nothing checked that its scroll position still exercised a refusal, so it could have stopped guarding anything silently. Derives both target offsets from computeScrollHideUpdate's own clauses instead of chosen numbers, because the two motions refuse for different reasons: in-flow runwayAfterCollapse <= revealIntentDistance + collapseRunwaySlack reserve-only offset > postCollapseMaxOffset + bottomClampTolerance Adds non-vacuity assertions ahead of the behavioural check. "Chrome stayed visible" only evidences a refusal if the offset sat inside the refusal band and the other clauses were satisfied. The load-bearing one is postCollapseMaxOffset >= topRevealOffset + hideIntentDistance: without it, a short post-collapse range refuses the hide on its own and the near-bottom clause is never reached, so the test passes while proving nothing. Renames the test. "96px of near-bottom runway" named collapseRunwaySlack, which exists only on the in-flow branch; reserve-only has no slack term. One test covers both owners, so the title now names the behaviour. Nothing outside the definition referenced the old name, so no flake-ledger identity is broken. No production hide policy changed — only what the test asserts about it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF * docs(issues): correct #102's stale ordering-safe claim for (status,id) Codex review on this PR caught that docs/outstanding-issues.md #102 still called documents_status_id_idx ordering-safe after this PR's own runbook correction reclassified it as canary-gated (its rag-candidate-sources.ts consumer shares the RAG-hazard mechanism with the trigram index; only the search-scope.ts ordered consumer stays safe). Mirrors the runbook's exact corrected wording so an operator following the ledger entry alone doesn't retain an unguarded index. * docs: record PR 1423 review --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
BigSimmo pushed a commit
that referenced
this pull request
Jul 30, 2026
Addresses the Codex P2 on this PR and fixes my own mistake in the first attempt.
The correction itself is unchanged in substance: `documents_status_id_idx` is
canary-gated, not ordering-safe, because `rag-candidate-sources.ts:482` pairs
`.eq("status","indexed")` with an unordered `.limit(12)` and `(status, id)`
serves that equality. The reasoning stays in the runbook, which is the
authority; the ledger row now just points at it.
What changed is the shape. My first version added prose to #102's Detail cell,
which is the widest cell in that column — so Prettier repadded every row and the
diff became 63 insertions / 63 deletions. Row #133 records that this exact
whole-table behaviour has already caused mechanical conflicts that silently
stopped all CI, and it is very likely how the original correction was lost
between #1423 and main in the first place.
Three attempts established the rule empirically, since my first two hypotheses
were both wrong: growing the cell reflows (68 lines), *shrinking* it also
reflows (68 lines, because the column width tracks this cell), and trailing
whitespace padding does not help because Prettier trims it. The replacement is
therefore exactly the same length as the text it replaces — 97 characters — and
the diff is now 1 insertion / 1 deletion with Prettier reporting the file
unchanged.
Verified on an untouched row: before this, #100's Detail cell lost exactly one
padding space (3533 -> 3532 chars), which is what proved the column width, not
my cell's content, was driving the churn.
Gates: outstanding-issues guard 137 rows / unique ids; branch-review-ledger 117
live + 1206 archived, no duplicates; prettier clean. No SQL applied.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiFBigSimmo added a commit
that referenced
this pull request
Jul 30, 2026
…#1440) * docs(issues): re-apply #102's canary-gated correction lost to a merge Codex raised this as a P2 on PR #1423: with the runbook corrected, ledger #102 still told an operator the `(status,id)` use was ordering-safe, so the two operator entry points contradicted each other. A reply on that thread claimed the fix landed in 2bdc245. It did not reach `main`. Verified by content rather than by SHA, which matters here: #1423 was squash-merged, so `git merge-base --is-ancestor` returns NO for every original commit and proves nothing either way. Reading `docs/outstanding-issues.md` at `origin/main` (0310c6a) shows the retracted sentence still present, while the runbook correction and the test change from the same PR both landed. Most likely cause, and it is a documented hazard: #1428 ("archive five completed ledger rows") touched this same file on `main`, and #112 records that `docs/outstanding-issues.md` conflicts present as ordinary content conflicts a hurried resolution can settle by dropping one side. Union merge has since been added to this file, but it would not have saved this case — both sides edited the same sentence. The row now mirrors the runbook's classification and names the runbook as the authority if the two ever diverge again, so a future reader has a tie-breaker rather than two equal-looking claims. Gates: verify:cheap exit 0, Test Files 434 passed (434), Tests 4563 passed | 4 skipped. Outstanding-issues guard, branch-review-ledger guard, docs:check-links, docs:check-scripts, prettier all pass. No SQL applied; no index created. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF * docs(issues): correct #102 within the existing table width Addresses the Codex P2 on this PR and fixes my own mistake in the first attempt. The correction itself is unchanged in substance: `documents_status_id_idx` is canary-gated, not ordering-safe, because `rag-candidate-sources.ts:482` pairs `.eq("status","indexed")` with an unordered `.limit(12)` and `(status, id)` serves that equality. The reasoning stays in the runbook, which is the authority; the ledger row now just points at it. What changed is the shape. My first version added prose to #102's Detail cell, which is the widest cell in that column — so Prettier repadded every row and the diff became 63 insertions / 63 deletions. Row #133 records that this exact whole-table behaviour has already caused mechanical conflicts that silently stopped all CI, and it is very likely how the original correction was lost between #1423 and main in the first place. Three attempts established the rule empirically, since my first two hypotheses were both wrong: growing the cell reflows (68 lines), *shrinking* it also reflows (68 lines, because the column width tracks this cell), and trailing whitespace padding does not help because Prettier trims it. The replacement is therefore exactly the same length as the text it replaces — 97 characters — and the diff is now 1 insertion / 1 deletion with Prettier reporting the file unchanged. Verified on an untouched row: before this, #100's Detail cell lost exactly one padding space (3533 -> 3532 chars), which is what proved the column width, not my cell's content, was driving the churn. Gates: outstanding-issues guard 137 rows / unique ids; branch-review-ledger 117 live + 1206 archived, no duplicates; prettier clean. No SQL applied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF * docs: record issue correction review --------- Co-authored-by: Claude <noreply@anthropic.com>
BigSimmo added a commit
that referenced
this pull request
Aug 18, 2026
…guard migrations (#316) (#2151) * feat(db): Phase 4 — restore 20 missing indexes, drop 2 orphans, land guards (#316) Owner-authorised off-peak production window against Clinical KB Database (sjrfecxgysukkwxsowpy). D4 auto-deploy is OFF, so nothing reached production on merge; every hosted change was made by an explicit named step. Rebuilt all 20 missing_live indexes with CREATE INDEX CONCURRENTLY using canonical definitions cross-read against their defining migrations — Batch A 14/14, Batch B 6/6, every one indisvalid AND indisready with normalised pg_get_indexdef matching canonical. Zero invalid builds, zero retries, zero skips, zero lock waits. No transactional build was attempted; #102's bare-column indexes were held out. Dropped both unexpected_live indexes concurrently rather than codifying them: the repo chain already commands both drops and each is a strict leading-column subset of a present canonical index. Live now reports 210 public indexes against the manifest's 210 repo-defined, zero invalid anywhere. Codified in four migrations, pushed to production by real supabase db push (never migration repair --status applied; all four history rows carry executed statements). 20260819100200 discharges the plan 4.4 debt by guarding the two trigram indexes restored 2026-08-14 that 20260804110240 never checked. 20260819100300 extends search_schema_health() required_indexes 22 -> 30, adopting all 8 Phase 6.3 monitor-candidates; production reports ok true, missing []. Staging brought to full parity in the same task: the trigram index was rebuilt into the canonical coalesce(content,'') form and the drift comparison is now green with zero unexpected drift, down from 19. Live-drift run 32171070287: unexpected drift 37 -> 16, missing_live 20 -> 0, unexpected_live 2 -> 0. Two escalations recorded rather than absorbed: PITR is not enabled on production (only daily physical backups), and the migration_history block did not drop because no guard here validates any object those 15 versions create — an empty intersection, so no allowlist entry was earned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ledger): record the Phase 4 index-restoration review (#316) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(db): mark Phase 4 complete on the coordination board and queue the PITR gap (#316) Board: Phase 4 row moved to COMPLETE 2026-08-19 for production and staging, with the measured outcome (20 rebuilt, 2 orphans dropped, live-drift 37 -> 16, staging drift 19 -> 0) and both escalations recorded rather than summarised away. D4 is recorded as decided OFF and confirmed empirically in that window, so the explicit-window model the plan assumes is live again and every future phase needs its own approved window and its own push. Adds a next-dispatch pointer: the remaining live-drift findings are exactly the fifteen migration_history rows (#Q5JHBJ), and Phase 4 earned no allowlist entry for any of them - measured, not skipped, since the intersection between the objects those versions create and the twenty-two Phase 4 guards validate is empty. Queues a P2 for the owner: point-in-time recovery is off on the live project, so the plan's standing 'restore point before any mutating phase' rule cannot be met. Phase 4 was safe regardless because every statement was index-only with an exact one-statement inverse, but that reasoning does not generalise to any future window that mutates data. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(db): make the migration chain produce the canonical content trigram index (#316) The 20260819100200 trigram guard failed the Supabase Preview check on PR #2151, and it was correct to. A preview branch builds from the migration chain alone, and the chain permanently produces the wrong definition of document_chunks_content_trgm_idx. Root cause: 20260606000000 creates it first as lower(coalesce(section_heading,'') || ' ' || content); 20260622000000 and 20260705180000 both carry the canonical coalesce(content,'') form but use CREATE INDEX IF NOT EXISTS, so they are no-ops, and no migration anywhere drops the index. schema.sql, the drift manifest and production hold the canonical form; every environment built from migrations alone holds the 2026-06-06 one. That is not cosmetic - the older expression is NULL for any row with NULL content, so those chunks are absent from the trigram index entirely. Forensics 3.3(d) had already seen this and scoped it as staging-only, repairing it by hand there. It was never staging-only: it is db reset, DR replay, CI migration replay, and the preview branch. The guard is what converted a silent environment-specific divergence into a reproducible failure. The repair is conditional and never runs a write-blocking build on a populated hosted database: no-op when already canonical, drop-and-recreate when the form is wrong and the table is empty, and a loud raise when the form is wrong and the table has rows. Ordered at 100150 so a fresh replay is canonical before 100200 validates it. Verified by replaying the entire chain into the same scratch Postgres image the manifest generator uses: without the fix it fails exactly as CI did, with it 204/204 apply and the index is canonical. The no-op path was proven on production itself - the index OID is unchanged at 1491258 across the push, with search_schema_health() still ok. Staging took the same migration by the Phase 2 method and its drift comparison stays green. schema.sql and drift-manifest.json are unchanged: the mirror was already right, and it was the chain that disagreed with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(issues): resolve the two-pending-mutations collision on #316 check-docs-links failed CI because the inbox refused to plan its batch: two update mutations were pending on #316 at once - 7a93a5a2 from the 2026-08-18 coordination session, which arrived via a main merge, and b048dbdd from this Phase 4 session. That is the exact collision the playbook warns about, and it blocks every ledger read until someone makes an explicit cancellation decision. Both are cancelled and replaced by one merged update, so neither session's result is lost. 7a93a5a2 was also partly stale: it recorded D4 as an open owner decision and listed Phase 4 as remaining, and both are now settled. Its still-current facts - the RPC-divergence track closing on 2026-08-18 and the Supabase auto-deploy finding - are carried forward verbatim, alongside the Phase 4 outcome, the chain-defect fix, and the two owner escalations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
7 tasks
BigSimmo added a commit
that referenced
this pull request
Aug 27, 2026
Merge origin/main (scripts-index count conflict). Restore conditional provider-retention wording on /privacy, sync AGENTS.md branching limit to 1, reclassify #102 as an update request, and restore truncated inbox outcomes.
BigSimmo pushed a commit
that referenced
this pull request
Aug 27, 2026
…napshot - AGENTS.md / docs/operator-supabase-branching-cap.md: the automatic-branching limit reduction (3 -> 1) has no available independent evidence it was actually applied in the Supabase dashboard. The only check we can run (zero active preview branches) is equally consistent with the limit still being 3, so AGENTS.md keeps documenting 3 as the authoritative, verifiable state, and the operator doc is reframed as a recommended action pending an operator-supplied durable record (screenshot or exported settings value). - data/outstanding-issues-snapshot.json: regenerate after the #102 inbox record changed from done to update, which changed the ledger's pending count (22 -> 31). `node scripts/check-outstanding-issues-snapshot.mjs` now reports in step. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0196uN8MrcdNMC3siuR94THX
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
npm run typechecknpm run test:e2e:chromium -- tests/ui-smoke.spec.ts -g "document viewer puts pinned evidence before the PDF preview on mobile"npx prettier --check src/components/DocumentViewer.tsx tests/ui-smoke.spec.tsnpm run verify:cheap- runtime, lint, typecheck, and 583 Vitest tests passednpm run verify:ui- 35 Chromium Playwright tests passednpm run verify:releasebefore release or handoff confidence claimsnpm run format:check- not run for this branch; repo currently has unrelated pre-existing formatting backlog outside these touched filesClinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
pdfjs-distupdate so any later PDF-rendering regressions are attributable to that dependency change rather than viewer navigation polish.