Skip to content

fix: high-confidence bugs from last-100-PR review - #1374

Merged
BigSimmo merged 29 commits into
mainfrom
cursor/recent-pr-bugfixes-f30d
Jul 29, 2026
Merged

fix: high-confidence bugs from last-100-PR review#1374
BigSimmo merged 29 commits into
mainfrom
cursor/recent-pr-bugfixes-f30d

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Summary

Review of the last 100 PRs found several high-confidence production bugs. This PR fixes the confirmed non-RAG P1/P2 defects, CI follow-ups, and review-thread repairs.

Fixed here

  1. P1 — Safety plan “Load example” looked print-ready (fix(safety-plan): draft-mark incomplete plans and require contact reach methods #1281): example mode stays active while any SEED row remains.
  2. P1 — Signed-image cache / painted URL survived logout and account switch (Harden clinical-notes trust gating and finish audit remediations #1305 surface): LRU cleared on identity change; mounted hook drops painted URL on auth-header change; cache cleared before setSession on user-id change so child effects cannot re-paint via requestAnimationFrame.
  3. P2 — Copy Diagnostics leaked clinical ?q= URLs (fix: Implement audit design fixes and fallback improvements #1351): redactLogValue consumes full non-whitespace URL tokens (including parentheses).
  4. P2 — Document search ilike fallback ignored status=indexed (Merge search performance and correctness fixes #1290).
  5. P2 — Document not-found claimed deletion / hardcoded blue link (fix: Implement audit design fixes and fallback improvements #1351).
  6. P2 — Focus token + internal <Link> wiring.
  7. CI — Production UI strict-mode on safety-plan export notice: locator scoped to [data-safety-plan-copy].
  8. Branch sync — merged origin/main (GitHub DIRTY was staleness).

RAG findings (deferred — needs approval)

Protected RAG/answer surfaces from #1289/#1292 were reviewed but not changed here.

Verification

  • Vitest: auth-signed-url-cache + privacy 22 passed (includes account-switch + parenthesis URL cases)
  • Playwright: safety-plan export notice scoped locator (prior head green on Production UI)
  • npm run verify:cheap on earlier CI-fix head; re-run left to hosted CI on this head
  • eslint on touched files clean
  • Provider checks not run

Risk and rollout

  • Risk: Medium for auth-bound signed-URL cache clearing and safety-plan example guard.
  • Rollback: revert this PR; no schema/migration changes.
  • Provider or production effects: None

Clinical Governance Preflight

  • Source-backed claims still require linked source verification before clinical use
  • No patient-identifiable document workflow was introduced or expanded without explicit governance approval
  • Supabase target remains Clinical KB Database (sjrfecxgysukkwxsowpy)
  • Service-role keys and private document access remain server-only
  • Demo/synthetic content remains clearly separated from real clinical sources
  • Source metadata, review status, and outdated/unknown-source behavior remain conservative
  • Deployment classification/TGA SaMD impact was checked when clinical decision-support behavior changed — no CDS behaviour change

RAG impact: no retrieval behaviour change — document-search status gate only rejects non-indexed docs to match existing search_document_chunks; no ranking/comparator/imputation edits.

Notes

  • Codex P1 threads (example guard, mounted sign-out paint, account-switch cache race): fixed and resolved.
  • CodeRabbit privacy URL + ui-smoke status scoping: fixed/confirmed and resolved.
Open in Web Open in Cursor 

Summary by CodeRabbit

  • New Features
    • Document search now returns an empty results set with a “document not indexed” strategy when the document isn’t indexed.
  • Bug Fixes
    • Signed private image/URL links are cleared promptly on auth identity changes, sign-out, and session expiry (including viewer behavior).
    • “Copy Diagnostics” and related diagnostics copying now redacts messages, URLs/paths, and browser details consistently.
    • Document viewer and safety-plan UI better handle example mode and prevent stale/private content across account changes.
  • Style
    • Updated not-found/unavailable messaging and applied themed accent styling; improved focus styling.
    • Search results now use in-app navigation links.
  • Documentation / Tests
    • Updated ledger and expanded privacy/diagnostics, signed-URL cache, viewer, safety-plan, and indexing edge-case tests.

… nav

Two P2 bugs found in the search/results-band bug hunt:

1. route-error-boundary.tsx: focus-visible outline used the undefined custom
   property --focus-ring (falling back to browser Highlight) instead of the
   app-wide --focus token (--clinical-accent). All other focus rings in the
   codebase already use --focus.

2. document-search-results.tsx: three raw <a> tags routed internal navigation
   (service record title, service 'Open' CTA, document title link) instead of
   Next.js <Link>. Adds the missing Link import and swaps all three. The
   document action buttons already used DocumentActionLink (<Link>-backed);
   only the inline title/CTA anchors were missed.
Private image signed URLs were retained in the module LRU after logout
or session expiry, so cached bearer URLs could still paint for ~10 minutes.
Clear the cache alongside other auth-bound client state.
- Keep safety-plan Load example non-shareable (fake crisis numbers)
- Redact clinical URLs/paths in Copy Diagnostics clipboard payloads
- Gate document search to indexed status before ilike fallback
- Soften document not-found copy and use theme accent tokens
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1dde2f94-f66d-4067-8633-5f9ffc513278

📥 Commits

Reviewing files that changed from the base of the PR and between 432f9fc and c14edb9.

📒 Files selected for processing (2)
  • src/components/DocumentViewer.tsx
  • tests/document-viewer-shell.dom.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/components/DocumentViewer.tsx

📝 Walkthrough

Walkthrough

The PR adds safety-plan example-mode safeguards, blocks search for non-indexed documents, redacts copied diagnostics, clears signed URL state across auth changes, updates internal navigation and unavailable-document states, and records review verification details.

Changes

Clinical safeguards and interface updates

Layer / File(s) Summary
Safety-plan example-mode gating
src/components/patient-safety-plan.tsx, tests/patient-safety-plan.dom.test.tsx, tests/ui-tools.spec.ts
Seeded example plans remain non-shareable until seeded content is removed, with updated warnings, labels, finalisation gating, and coverage.
Privacy redaction for diagnostics
src/lib/privacy.ts, src/app/global-error.tsx, src/components/route-error-boundary.tsx, tests/privacy.test.ts, tests/route-error-boundary-diagnostics.dom.test.tsx
Copied diagnostics redact sensitive messages, URLs, and user agents; URL precedence and clipboard output are tested.
Auth-bound signed URL invalidation
src/lib/supabase/client.tsx, src/components/clinical-dashboard/use-signed-image-url.ts, src/components/DocumentViewer.tsx, tests/auth-signed-url-cache.dom.test.tsx, tests/document-viewer-shell.dom.test.tsx
Signed URL caches and rendered URL state are cleared during sign-out, expiry, initialization, and identity changes while same-user token refreshes preserve the current URL.
Document search and navigation states
src/app/api/documents/[id]/search/route.ts, src/components/clinical-dashboard/document-search-results.tsx, src/app/(search-app)/*, tests/private-access-routes.test.ts, tests/ui-smoke.spec.ts
Non-indexed documents return empty results without chunk queries, internal links use Link, and unavailable-document copy and styling are updated.
Review ledger update
docs/branch-review-ledger.md
The ledger adds the 2026-07-29 findings, fixes, PR reference, and verification records.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Clinician
  participant PatientSafetyPlan
  participant Clipboard
  Clinician->>PatientSafetyPlan: Load example
  PatientSafetyPlan->>PatientSafetyPlan: Derive exampleActive
  PatientSafetyPlan->>PatientSafetyPlan: Disable finalisation
  Clinician->>PatientSafetyPlan: Copy plan
  PatientSafetyPlan->>Clipboard: Write example-only export
Loading

Possibly related PRs

Suggested reviewers: cursoragent

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the PR's main fixes from the last-100-PR review.
Description check ✅ Passed The description matches the template well with summary, verification, risk, governance, and notes sections filled in.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@supabase

supabase Bot commented Jul 29, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

Prefer https?:// → [url] so clinical ?q= query text cannot survive as a
residual path fragment, and harden the Copy Diagnostics privacy test.
Append-only review record for main@70e5101c scope last-100-prs-bug-review.
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

CI triage

CI failed on this PR. Automated classification of the 2 failed job(s):

  • Production UIneeds investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.
  • PR requiredneeds investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.

Compared with main CI run #6575 (success).

Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger.

@BigSimmo

Copy link
Copy Markdown
Owner Author

@cursoragent Inspect the PR and repository state, identify the root cause of every failing CI check, unresolved review comment, merge conflict, and related defect. Apply the smallest robust fix using existing project patterns and best practices, without unrelated changes. Run Bugbot and all relevant lint, type-check, test, build, and CI-equivalent commands, fix any issues found, then resolve only the review comments that are fully addressed. Summarise the root causes, changes made, validation results, and any remaining blockers.

@cursor

cursor Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Root causes

Issue Cause
Production UI (→ PR required) ui-tools safety-plan test used unscoped getByText(/Copying, printing…/), which strict-mode-failed with 2 matches
GitHub DIRTY / CONFLICTING Branch behind main; git merge-tree was clean (staleness, not a content conflict)
Codex P1 — example guard Any edit (including plan date only) cleared exampleActive while SEED fake numbers remained → Finalise could enable
Codex P1 — signed URL paint LRU cleared on logout, but mounted useSignedImageUrl kept the old url until refetch finished

No unresolved review threads left after fixes. No real merge conflicts.

Changes pushed

  1. Merged origin/main
  2. Scoped Playwright locator to [data-safety-plan-copy]
  3. Example mode now derived from remaining SEED row IDs (date-only edit stays blocked)
  4. Painted signed-image URL cleared on auth header change
  5. Codex threads replied + resolved

Validation

  • Vitest: patient-safety-plan + auth-signed-url 8/8; related suite 170/170
  • Playwright: safety-plan export test 1 passed
  • verify:cheap: 424 files / 4279 passed | 4 skipped
  • eslint on touched files: clean
  • PR now MERGEABLE; hosted CI re-running on 85b7e38b

Remaining blockers

Hosted Production UI / PR required must finish green on the new head — local reproducer passes; waiting on GitHub Actions.

Open in Web Open in Cursor 

@BigSimmo

Copy link
Copy Markdown
Owner Author

@codex review

@BigSimmo
BigSimmo marked this pull request as ready for review July 29, 2026 04:06
@BigSimmo

Copy link
Copy Markdown
Owner Author

@claude review PR and fix issues

@github-actions

Copy link
Copy Markdown
Contributor

Hi @BigSimmo — thanks for the @claude mention.

Open this in Claude Code on the web to continue here: https://claude.ai/code

Context: BigSimmo/Database · #1374

Automated backlink from the Claude backlink workflow. Claude Code can pick up the mention and push a scoped fix to the branch.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@BigSimmo

Copy link
Copy Markdown
Owner Author

@CodeRabbit review PR

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

@BigSimmo The PR description images show dark “Open in Web” and “Open in Cursor” buttons. I’ll review PR #1374 against its stated privacy, cache invalidation, safety-plan, and document-search objectives.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e131dd0be8

ℹ️ 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".

Comment thread src/components/patient-safety-plan.tsx Outdated
Comment thread src/lib/supabase/client.tsx

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
tests/patient-safety-plan.dom.test.tsx (1)

63-81: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Do not treat any single edit as replacing the example.

This test codifies an unsafe transition: changing only the date, adding a row, or replacing one contact clears exampleActive, while other seeded entries—including non-working numbers—remain. Since ready then becomes true for a complete plan, the watermark and finalisation block disappear despite the UI saying to replace every entry.

Keep the example guard active until all seeded/example rows are removed or replaced, and add a regression test that edits only the date and verifies the example warning and disabled finalise control remain.

🤖 Prompt for 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.

In `@tests/patient-safety-plan.dom.test.tsx` around lines 63 - 81, Keep the
PatientSafetyPlan example state active until every seeded/example entry has been
removed or replaced, rather than clearing it after a single edit such as a date
change, added row, or contact replacement. Update the example/ready guard logic
and extend the existing regression coverage with a date-only edit asserting the
example warning remains visible and “Finalise plan” stays disabled.
src/lib/supabase/client.tsx (1)

357-398: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Gate signed-URL cache writes on auth epoch. clearSignedUrlCache() removes the old entry, but useSignedImageUrl can still repopulate the module Map from an in-flight response after sign-out/expiry/account switch because the completion path only checks a local active flag. invalidateAuthRequests() only aborts registered requests, and this hook never registers its fetch. Check isAuthEpochCurrent(epoch) before setCachedSignedUrl(...) (or wire the fetch into the auth request lifecycle).

🤖 Prompt for 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.

In `@src/lib/supabase/client.tsx` around lines 357 - 398, The signed-image URL
fetch must not repopulate the cache after authentication changes. In
useSignedImageUrl, capture the auth epoch when starting the request and check
isAuthEpochCurrent(epoch) alongside the existing active guard before calling
setCachedSignedUrl; preserve the existing cleanup behavior for unmounted or
cancelled requests.
🧹 Nitpick comments (1)
tests/auth-signed-url-cache.dom.test.tsx (1)

90-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Coverage gap: the race this suite sidesteps, and the account-switch path, are both untested.

Test 1 deliberately disables the probe before signOut (lines 118-123) to avoid the in-flight-fetch race flagged in src/lib/supabase/client.tsx (signOut/markSessionExpired/fingerprint effect) — so the suite currently can't catch a regression where a pending signed-URL fetch re-seeds the cache after the clear. Also, neither test exercises the fingerprint-effect clear path for a direct account switch (SIGNED_IN fired for a different user id without an intervening SIGNED_OUT), which is one of the transitions the new code specifically targets ("Account switch / sign-in / sign-out" per the client.tsx comment).

Consider adding: (1) a case that keeps the probe enabled through signOut and asserts the cache doesn't get re-populated with the stale URL even if the fetch resolves late, and (2) a case that fires SIGNED_IN for a new user id directly and asserts clearSignedUrlCache runs via the fingerprint effect.

🤖 Prompt for 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.

In `@tests/auth-signed-url-cache.dom.test.tsx` around lines 90 - 159, Add coverage
in the auth signed-URL cache tests for both requested transitions: keep
SignedImageProbe enabled through signOut, control a pending fetch, resolve it
late, and assert the cleared cache is not re-seeded with the stale private URL;
also simulate a direct SIGNED_IN event for a different user without SIGNED_OUT
and assert the fingerprint-effect path clears the cache. Use the existing
AuthProvider, SignedImageProbe, cache helpers, and auth-event utilities rather
than changing production behavior.
🤖 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 `@src/components/patient-safety-plan.tsx`:
- Around line 479-485: Update addEntry so uid(key) is called before the
functional setEntries updater, storing the resulting entry for reuse inside the
updater. Keep the updater limited to deriving the next entries state without
mutating uidRef.current, while preserving the existing finalisation and
edit-marking behavior.

In `@src/lib/privacy.ts`:
- Around line 24-28: Update the URL replacement pattern in the privacy
sanitization chain to consume the complete non-whitespace URL token, including
parentheses, instead of stopping at ). Preserve the URL-before-path replacement
order, and add a regression test covering a URL such as
https://psychiatry.tools/dsm?q=clozapine%20(ANC)%20Jane to verify no suffix
remains unredacted.

In `@tests/ui-smoke.spec.ts`:
- Line 4152: Update the missing-document assertion in the relevant UI smoke test
to target the not-found/status panel locator instead of page.locator("body"),
while preserving the existing unavailable/private/missing/removed text pattern.

---

Outside diff comments:
In `@src/lib/supabase/client.tsx`:
- Around line 357-398: The signed-image URL fetch must not repopulate the cache
after authentication changes. In useSignedImageUrl, capture the auth epoch when
starting the request and check isAuthEpochCurrent(epoch) alongside the existing
active guard before calling setCachedSignedUrl; preserve the existing cleanup
behavior for unmounted or cancelled requests.

In `@tests/patient-safety-plan.dom.test.tsx`:
- Around line 63-81: Keep the PatientSafetyPlan example state active until every
seeded/example entry has been removed or replaced, rather than clearing it after
a single edit such as a date change, added row, or contact replacement. Update
the example/ready guard logic and extend the existing regression coverage with a
date-only edit asserting the example warning remains visible and “Finalise plan”
stays disabled.

---

Nitpick comments:
In `@tests/auth-signed-url-cache.dom.test.tsx`:
- Around line 90-159: Add coverage in the auth signed-URL cache tests for both
requested transitions: keep SignedImageProbe enabled through signOut, control a
pending fetch, resolve it late, and assert the cleared cache is not re-seeded
with the stale private URL; also simulate a direct SIGNED_IN event for a
different user without SIGNED_OUT and assert the fingerprint-effect path clears
the cache. Use the existing AuthProvider, SignedImageProbe, cache helpers, and
auth-event utilities rather than changing production behavior.
🪄 Autofix (Beta)

✅ Autofix completed


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 81384ec7-f412-41f9-b4db-01d4a951bece

📥 Commits

Reviewing files that changed from the base of the PR and between 70e5101 and e131dd0.

📒 Files selected for processing (16)
  • docs/branch-review-ledger.md
  • src/app/(search-app)/differentials/not-found.tsx
  • src/app/(search-app)/documents/[id]/not-found.tsx
  • src/app/api/documents/[id]/search/route.ts
  • src/app/global-error.tsx
  • src/components/clinical-dashboard/document-search-results.tsx
  • src/components/patient-safety-plan.tsx
  • src/components/route-error-boundary.tsx
  • src/lib/privacy.ts
  • src/lib/supabase/client.tsx
  • tests/auth-signed-url-cache.dom.test.tsx
  • tests/patient-safety-plan.dom.test.tsx
  • tests/privacy.test.ts
  • tests/private-access-routes.test.ts
  • tests/route-error-boundary-diagnostics.dom.test.tsx
  • tests/ui-smoke.spec.ts

Comment thread src/components/patient-safety-plan.tsx Outdated
Comment thread src/lib/privacy.ts
Comment thread tests/ui-smoke.spec.ts Outdated
- Keep safety-plan example mode while any SEED row remains (date-only edits
  no longer enable Finalise with fake crisis numbers)
- Clear mounted signed-image URL state on auth identity change
- Scope safety-plan Playwright export notice to [data-safety-plan-copy]
  to avoid Production UI strict-mode failures
@BigSimmo
BigSimmo enabled auto-merge (squash) July 29, 2026 04:21
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 3 file(s) based on 2 unresolved review comments.

Files modified:

  • src/lib/privacy.ts
  • tests/privacy.test.ts
  • tests/ui-smoke.spec.ts

Commit: d9cb3b6cf2bc0ecf7ced09614a1a65fc6df5332b

The changes have been pushed to the cursor/recent-pr-bugfixes-f30d branch.

Time taken: 9m 45s

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 90eebbcbf9

ℹ️ 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".

Comment thread src/components/clinical-dashboard/use-signed-image-url.ts Outdated
@BigSimmo

Copy link
Copy Markdown
Owner Author

@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: 90eebbcbf9

ℹ️ 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".

Fixed 3 file(s) based on 2 unresolved review comments.

Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
@BigSimmo

Copy link
Copy Markdown
Owner Author

@copilot review PR

auto-merge was automatically disabled July 29, 2026 12:40

Head branch was pushed to by a user without write access

Copilot AI commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

@copilot review PR

Reviewed current head 1d26698.

Blocking finding (P1): src/components/DocumentViewer.tsx still keeps mounted signedUrl/downloadSignedUrl state across auth-only identity changes, so a prior user’s private bearer URL can remain rendered until refetch/error settles. This matches the still-unresolved thread on src/lib/supabase/client.tsx and is not fixed by the current branch diff.

CI status: latest branch CI run 30452197361 is still in progress; get_job_logs reports "No failed jobs found" so far.

I did not apply code changes from this @copilot review-only request. Review record appended in 3d173834.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
tests/auth-signed-url-cache.dom.test.tsx (1)

159-236: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Missing coverage for TOKEN_REFRESHED with the same user.

Given the concern raised on use-signed-image-url.ts (render-time reset may key off token identity rather than user identity), consider adding a case where the same user's session is refreshed (new access token, same user id) and asserting the signed URL does not momentarily disappear, since the cache is not cleared on refresh.

🤖 Prompt for 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.

In `@tests/auth-signed-url-cache.dom.test.tsx` around lines 159 - 236, Add a test
alongside the existing sign-out and account-switch cases that emits a
TOKEN_REFRESHED event for the same user with a new access token but unchanged
user id. Assert the existing signed URL remains rendered throughout the refresh
and the cached entry remains intact, verifying no render-time reset occurs when
only token identity changes.
🤖 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 `@src/components/clinical-dashboard/use-signed-image-url.ts`:
- Around line 21-29: Key the reset logic in useSignedImageUrl to a stable
authenticated-user identity, such as session.user.id or the existing auth
fingerprint, instead of authorizationHeader. Preserve the existing setUrl(null)
and setFailed(false) behavior only when that identity changes, so token
refreshes for the same user do not clear the cached URL.

In `@src/lib/privacy.ts`:
- Line 26: Update the URL-matching replacement in redactLogValue so it stops at
serialized JSON/string delimiters instead of consuming subsequent fields, while
continuing to redact complete URLs and allow parentheses within them. Preserve
the existing [url] replacement behavior and handling for non-URL text.

---

Nitpick comments:
In `@tests/auth-signed-url-cache.dom.test.tsx`:
- Around line 159-236: Add a test alongside the existing sign-out and
account-switch cases that emits a TOKEN_REFRESHED event for the same user with a
new access token but unchanged user id. Assert the existing signed URL remains
rendered throughout the refresh and the cached entry remains intact, verifying
no render-time reset occurs when only token identity changes.
🪄 Autofix (Beta)

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: 7739d7ab-8f94-4d9d-8212-f9e839d90ab4

📥 Commits

Reviewing files that changed from the base of the PR and between e131dd0 and 1d26698.

📒 Files selected for processing (12)
  • docs/branch-review-ledger.md
  • src/components/clinical-dashboard/document-search-results.tsx
  • src/components/clinical-dashboard/use-signed-image-url.ts
  • src/components/patient-safety-plan.tsx
  • src/lib/privacy.ts
  • src/lib/supabase/client.tsx
  • tests/auth-signed-url-cache.dom.test.tsx
  • tests/patient-safety-plan.dom.test.tsx
  • tests/privacy.test.ts
  • tests/private-access-routes.test.ts
  • tests/ui-smoke.spec.ts
  • tests/ui-tools.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • tests/privacy.test.ts
  • tests/ui-smoke.spec.ts
  • tests/private-access-routes.test.ts
  • src/lib/supabase/client.tsx
  • src/components/clinical-dashboard/document-search-results.tsx

Comment thread src/components/clinical-dashboard/use-signed-image-url.ts Outdated
Comment thread src/lib/privacy.ts Outdated
claude added 3 commits July 29, 2026 13:42
Three review findings, two of them regressions this branch introduced.

P1 — mounted document-viewer URLs survived an identity change. Clearing the
module LRU does not reach DocumentViewer's own `signedUrl`/`downloadSignedUrl`
state, which was reset only on a *full* document reload. An auth-only
transition keeps the same load key, so after sign-out or an account switch the
previous identity's bearer link to a private PDF stayed rendered — and the
error UI kept offering it — until the URL expired on its own. The viewer now
drops both during render when the auth identity changes. Reproduced first: the
new test finds user A's anchor still present after the switch without this.

Redaction consumed neighbouring diagnostic fields. Non-string values are
JSON-stringified before redaction and compact JSON has no whitespace, so the
`\S+` URL pattern swallowed the closing quote and every field after it —
`{"url":"…","code":"23505"}` redacted to `{"url":"[url]`. The class now excludes
whitespace and quotes: parentheses are still consumed, so the
`?q=clozapine%20(ANC)%20Jane` leak stays fixed, but a serialized object stays
readable.

Signed-image reset keyed to the wrong value. `authorizationHeader` is a fresh
object on every access-token refresh, so a mounted private image blanked and
refetched roughly hourly for a user still entitled to see it. Keyed to the user
id instead, which changes in the same render on a real identity change and so
is no later than before — it just ignores refreshes. Asserting the settled DOM
could not catch this because the LRU repaints within a frame, so the test
inspects the render sequence.

Verification: verify:cheap exit 0 — Test Files 428 passed (428), Tests 4394
passed | 4 skipped (4398). Each new test was checked to fail against the
pre-fix code rather than pass vacuously.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c0a894374d

ℹ️ 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".

Comment thread src/components/DocumentViewer.tsx
claude added 4 commits July 29, 2026 14:46
Review asked for the preview to be reissued after an identity change so a
document the new identity can still read is not stranded. I implemented that
(bumping previewAttempt to force a full reload) and then removed it: the full
reload routes through openSourcePreview({ useCache: true }), which reads the
module signed-URL LRU, so it would repaint the PREVIOUS identity's URL anywhere
that cache had not already been cleared - the exact leak this reset closes.

Clearing without reissuing is the conservative failure: a blank preview that
recovers on reload, rather than the prior clinician's document. The comment now
states that so the next person does not re-add the bump. The strand is real and
worth fixing properly by reissuing with useCache: false; tracked as follow-up
rather than shipped unverified.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
The merge=union driver keeps both copies of rows present on this branch and on
main; the guard flags them as duplicate ref/HEAD/scope records (#88).

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a6c88d543e

ℹ️ 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".

Comment thread src/lib/privacy.ts Outdated
encodeURIComponent does not escape apostrophes, so a clinical query can carry
one verbatim. Excluding ' from the URL character class stopped the match there
and left the remainder in the clipboard:

  https://psychiatry.tools/dsm?q=patient's%20suicidal%20thoughts
    -> [url]'s%20suicidal%20thoughts

That is the exact leak this redaction exists to prevent, in the Copy Diagnostics
path this PR is meant to make safe. The class now excludes whitespace and the
double quote only - the double quote is what keeps a serialized JSON object
readable, and no test depended on stopping at an apostrophe. Over-consuming a
trailing ' in JS-style single-quoted log output redacts slightly more, never
less.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 432f9fcab3

ℹ️ 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".

Comment thread src/components/DocumentViewer.tsx

Copy link
Copy Markdown
Owner Author

Production UI red on 432f9fca — flake, re-running once

UI_RESULT: failure, so this is a real job failure and not the #095 cancellation false-red. Two tests:

- tests/ui-phone-scroll.spec.ts :: document detail header overlay and footer follow browser document scrolling together
- tests/ui-tools.spec.ts :: search route keeps the correct composer at mobile width on /differentials?q=acute+confusion&focus=1&run=1

Why this is not attributable to the change. The immediately preceding head a6c88d54 passed Production UI on this same branch (PR required: success, 15:54). The only delta between that head and this one is:

 src/lib/privacy.ts    | 18 ++++++++++++------
 tests/privacy.test.ts | 12 ++++++++++--

That commit changes one character class in redactLogValue and adds a test. redactLogValue is reachable only from the error-diagnostics copy paths (GlobalError, RouteErrorBoundary) — it cannot affect document-detail header/footer scroll behaviour or the differentials composer. No component, route, CSS or phone-chrome file is touched.

Corroboration that these are known flakes. Both belong to the family PR #1391 is currently open to fix — it identifies six hydration races in ui-smoke, ui-tools, ui-accessibility and ui-overlap, with the diagnosis that in this app visible never implies interactive: the server-rendered tree paints before React owns it, so a single un-retried action asserted once is a coin flip under load. The ui-tools composer assertion is exactly that shape. The ui-phone-scroll document-detail test also failed on PR #1377 earlier today and passed on its next run.

One re-run, then I stop. I have re-run the failed job. If the same tests fail again on an unchanged head I will not re-roll a third time — I will report it as a real blocker and leave it for a human, because a required check that gets re-rolled until it is green is exactly the habit #095 warns teaches people to discount red.


Generated by Claude Code

…itch

The render-phase auth-identity reset in DocumentViewer cleared only the two
signed source URLs. An auth-only transition leaves the document load key
unchanged, so `isFullDocumentReload` is false and the detail effect
deliberately keeps the current window mounted until the replacement request
settles. On a slow, offline, or denied request that window is the previous
identity's extracted private content — title, pages, images, table facts,
chunks, index health, the generated summary, and the in-document search
query and snippets — left readable to the new account for as long as its
request takes.

Clear all of it in the same render-phase reset, and show the loading state
while the refetch (triggered by the identity change through
`authorizationHeader`) is in flight.

The reset is guarded on the previous identity being non-null so the ordinary
`null -> A` first-mount transition does not discard the server-rendered
`initialDetail` on every page load. Sign-out (`A -> null`) and account switch
(`A -> B`) both still clear.

`initialDetail` also had to be invalidated explicitly: it is server-rendered
for whoever requested the page, and the detail effect's `useInitialResult`
branch replays it whenever the route still matches the initial one — so the
very effect re-run the identity change triggers would have re-applied user
A's SSR payload to user B and undone the clear. A latched
`initialDetailIdentityStale` flag forces that refetch to the network.

Test defers user B's detail response indefinitely and asserts A's title,
chunk text, and table/diagram content are gone at the moment the identity
changes rather than when B's reply lands. Verified red against the previous
implementation (`expected <h1 …(1)></h1> to be null`).
@BigSimmo
BigSimmo merged commit 3704007 into main Jul 29, 2026
28 checks passed
@BigSimmo
BigSimmo deleted the cursor/recent-pr-bugfixes-f30d branch July 29, 2026 17:21
BigSimmo pushed a commit that referenced this pull request Jul 29, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FvU8z73P6TXUXoYBqN5K1P
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.

4 participants