Fix scoped-label pagination and stabilize reconciliation preflight test#1191
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthrough
ChangesSearch-scope label pagination
Reconciliation preflight testing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/reconciliation-preflight.test.ts (1)
105-114: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd CLI-level coverage for the preflight command.
The
reconciliation-preflighttest file has no subprocess/smoke coverage for the executable; the existing line only coverscollectReconciliationState()plusJSON.stringify, so argument handling, stdout vs stderr, and exit-code regressions can still pass unnoticed. Keep a smallnode scripts/reconciliation-preflight.mjs [--json]subprocess test alongside the in-process test unless equivalent coverage already exists elsewhere.🤖 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/reconciliation-preflight.test.ts` around lines 105 - 114, Add a focused subprocess smoke test alongside the existing collectReconciliationState test in reconciliation-preflight.test.ts, invoking node scripts/reconciliation-preflight.mjs with and without --json as appropriate. Assert argument handling, valid stdout JSON versus stderr behavior, and successful exit status, while preserving the existing in-process coverage.
🤖 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 `@tests/search-scope.test.ts`:
- Around line 25-35: Update the mock query builder in the test to record calls
to order(), specifically the document_labels ordering request, and assert that
order("id", { ascending: true }) is made. Apply the same verification to the
additional affected range, while preserving the existing pagination assertions.
---
Nitpick comments:
In `@tests/reconciliation-preflight.test.ts`:
- Around line 105-114: Add a focused subprocess smoke test alongside the
existing collectReconciliationState test in reconciliation-preflight.test.ts,
invoking node scripts/reconciliation-preflight.mjs with and without --json as
appropriate. Assert argument handling, valid stdout JSON versus stderr behavior,
and successful exit status, while preserving the existing in-process coverage.
🪄 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 Plus
Run ID: bf759243-2fbc-44e8-bf53-0da4eb35b7cc
📒 Files selected for processing (3)
src/lib/search-scope.tstests/reconciliation-preflight.test.tstests/search-scope.test.ts
CodeRabbit: mock order() was a no-op, so removing .order would still pass. Record and assert ascending id orders for document_labels pages.
* docs(issues): close stale #66/#67 after main landings #66 is on origin/main via PR #1174; #67 was fixed in PR #1191 by running the preflight JSON contract in-process. Also drop already- resolved #30/#75 from the recommended queue and renumber. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs: pin branch-review ledger SHA for #66/#67 closeout Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Summary
fields=index, differentials abort/debounce, universal documents typeahead soft-timeout (750ms), shared(search-app)shell to avoid composer remount, and Answer rate-limit in-memory fallback outside production./services→/dsm) syncssearchModeduring render (no stale-mode paint) even when the query string is unchanged; extracted ClinicalDashboard lazy imports to stay under the maintainability budget.RAG impact: no retrieval behaviour change — typeahead documents domain timeout and shell URL sync only; ranking formulas and full
/api/searchretrieval path unchanged.Verification
npm run verify:pr-local— focused Vitest on touched sources (362) plus api-rate-limit / search-shell / universal / route / site-map suites green;docs:check-indexOKverify:uinot required for this pass; mode-home smoke vianpm run ensurereturned HTTP 200 for/,/services,/dsm,/documents/search,/therapy-compass,/?mode=prescribing, and/api/answer/streamreturned 200 after the rate-limit fallback fixeval:retrieval:latency/ soak / live OpenAI canary — approval-gated provider work; not needed for timeout-only typeahead changeRisk and rollout
GlobalSearchShelllayouts and prior timeout/fallback behaviourClinical Governance Preflight
[REDACTED]([REDACTED])Notes
fields=indexremains for identity-only consumers (cross-mode links).Summary by CodeRabbit