fix: resolve quality and testing review findings#465
Merged
Conversation
…integration # Conflicts: # docs/branch-review-ledger.md
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds runtime validation for streamed answers and extracted documents, updates document mutation and ingestion logic, changes universal-search and tool mockup behavior, and expands route, client, unit, and browser test coverage. ChangesAnswer Search Flow
Document Processing and Mutation
Tools Mockup Filtering
Runtime Tests and Records
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant UniversalSearch
participant ClinicalDashboard
participant AnswerStream
User->>UniversalSearch: Select “Ask this question”
UniversalSearch->>ClinicalDashboard: Cross into Answer mode with query
ClinicalDashboard->>AnswerStream: POST /api/answer/stream
AnswerStream-->>ClinicalDashboard: SSE final event
ClinicalDashboard-->>User: Render validated answer
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ClinicalDashboard.tsx (1)
401-431: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancel the stream reader on every exit path.
readAnswerStreamonly cancels the reader on the success path. Theerrorand invalid-finalthrow paths unwind without cleanup, so retries can leave the response body/reader open until GC and keep the upstream stream running. Wrap the loop intry/finallyand cancel there.🤖 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/components/ClinicalDashboard.tsx` around lines 401 - 431, Update readAnswerStream to guarantee reader cleanup on all exit paths: wrap the read/parsing loop and final-payload handling in try/finally, and call reader.cancel().catch(() => undefined) from the finally block. Remove or avoid relying on the success-only cancellation so errors and invalid final payloads also terminate the stream.
🤖 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/tools-page-mockups/tools-page-mockup-page.tsx`:
- Around line 1089-1129: Thread the preview-selection wiring into the
filtered-results branch of SplitPaneMockup: pass selectedToolId and onSelectTool
to each filtered tool tile, or reuse the selection-aware component used by
WideAllToolsSection, instead of rendering plain ToolCard links. Ensure clicking
a filtered result updates the phone preview while preserving navigation
behavior.
---
Outside diff comments:
In `@src/components/ClinicalDashboard.tsx`:
- Around line 401-431: Update readAnswerStream to guarantee reader cleanup on
all exit paths: wrap the read/parsing loop and final-payload handling in
try/finally, and call reader.cancel().catch(() => undefined) from the finally
block. Remove or avoid relying on the success-only cancellation so errors and
invalid final payloads also terminate the stream.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b9643228-6935-4e14-807c-1231b0d915db
📒 Files selected for processing (18)
docs/branch-review-ledger.mdplaywright.config.tssrc/app/api/documents/[id]/table-facts/route.tssrc/app/api/documents/bulk/route.tssrc/app/api/ingestion/quality/route.tssrc/components/ClinicalDashboard.tsxsrc/components/clinical-dashboard/search-utils.tssrc/components/clinical-dashboard/universal-search-command-surface.tsxsrc/components/tools-page-mockups/tools-page-mockup-page.tsxsrc/lib/extractors/document.tstests/auth-callback-route.test.tstests/clinical-dashboard-search-utils.test.tstests/document-mutation-routes.test.tstests/extracted-document-validation.test.tstests/supabase-server.test.tstests/ui-smoke.spec.tstests/ui-universal-search.spec.tsvitest.config.mts
Comment on lines
+1089
to
+1129
| {filter.isFiltering ? ( | ||
| <> | ||
| <div className="flex items-center justify-between gap-3"> | ||
| <div className="flex items-center gap-2"> | ||
| <LayoutDashboard className="h-4 w-4 text-[color:var(--clinical-accent)]" aria-hidden="true" /> | ||
| <h2 className="text-base font-extrabold text-[color:var(--text-heading)]">Results</h2> | ||
| </div> | ||
| <p | ||
| aria-live="polite" | ||
| className="nums inline-flex min-h-8 items-center rounded-md border border-[color:var(--border)] bg-[color:var(--surface-subtle)] px-2.5 text-xs font-bold text-[color:var(--text-muted)]" | ||
| > | ||
| Showing {filter.filtered.length} of {tools.length} | ||
| </p> | ||
| </div> | ||
| {filter.filtered.length > 0 ? ( | ||
| <div className="grid gap-3 sm:grid-cols-2"> | ||
| {filter.filtered.map((tool) => ( | ||
| <ToolCard key={tool.id} tool={tool} suggested={tool.id === suggestedId} /> | ||
| ))} | ||
| </div> | ||
| ) : ( | ||
| <AllToolsEmptyState query={filter.query} onClear={filter.reset} /> | ||
| )} | ||
| </> | ||
| ) : ( | ||
| <> | ||
| <div className="flex items-center gap-2"> | ||
| <LayoutDashboard className="h-4 w-4 text-[color:var(--clinical-accent)]" aria-hidden="true" /> | ||
| <h2 className="text-base font-extrabold text-[color:var(--text-heading)]">Launcher overview</h2> | ||
| </div> | ||
| <p className="max-w-2xl text-sm font-medium leading-5 text-[color:var(--text-muted)]"> | ||
| Filters sit beside the overview, while the full-width All tools view below carries the main browsing | ||
| weight. | ||
| </p> | ||
| <div className="grid gap-3 sm:grid-cols-2"> | ||
| {["clinical-kb-search", suggestedId, "medication-prescribing", "favourites"].map((id) => ( | ||
| <ToolCard key={id} tool={toolById(id)} suggested={id === suggestedId} /> | ||
| ))} | ||
| </div> | ||
| </> | ||
| )} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "function ToolCard" -A 40 src/components/tools-page-mockups/tools-page-mockup-page.tsx
rg -n "function WideToolTile" -A 40 src/components/tools-page-mockups/tools-page-mockup-page.tsxRepository: BigSimmo/Database
Length of output: 3918
🏁 Script executed:
#!/bin/bash
sed -n '980,1185p' src/components/tools-page-mockups/tools-page-mockup-page.tsxRepository: BigSimmo/Database
Length of output: 8878
🏁 Script executed:
#!/bin/bash
sed -n '1030,1165p' src/components/tools-page-mockups/tools-page-mockup-page.tsxRepository: BigSimmo/Database
Length of output: 6495
🏁 Script executed:
#!/bin/bash
rg -n "selectedToolId|selectedTool|setSelectedToolId|onSelectTool|WideAllToolsSection|ToolCard|SplitPaneMockup" src/components/tools-page-mockups/tools-page-mockup-page.tsxRepository: BigSimmo/Database
Length of output: 2539
Filtering in SplitPaneMockup drops the preview-selection wiring The filtered-results branch renders plain ToolCard links, while WideAllToolsSection is the only list here that accepts selectedToolId/onSelectTool. In search mode, clicking a result won’t update the phone preview, so the “click any tool to preview it” interaction is unavailable until filtering is cleared. Consider threading the selection props through the filtered grid or reusing the selection-aware tile component.
🤖 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/components/tools-page-mockups/tools-page-mockup-page.tsx` around lines
1089 - 1129, Thread the preview-selection wiring into the filtered-results
branch of SplitPaneMockup: pass selectedToolId and onSelectTool to each filtered
tool tile, or reuse the selection-aware component used by WideAllToolsSection,
instead of rendering plain ToolCard links. Ensure clicking a filtered result
updates the phone preview while preserving navigation behavior.
This was referenced Jul 13, 2026
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 free
to 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
Why
The open Codex quality, testing, TypeScript, and coverage tasks identified 17 confirmed P2/P3 issues. This branch replays their reviewed fixes onto current
mainand removes the duplicate test-only branch from the merge path.Verification
Current integration branch:
git diff --check— passedOriginal reviewed branch evidence:
Not run
Browser checks were not rerun after replay because local API/browser interaction remains confirmation-gated. The integration worktree also lacks the
@next/bundle-analyzerpackage added tomainafter its original install, so hosted CI's clean install is the authoritative current typecheck/build.Risk and governance
Touches API mutation behavior, clinical answer payload validation, extractor parsing, and frontend navigation. No database schema, provider credentials, production data, or live services are changed. Failures remain additive/fail-closed and are covered by focused tests.