Skip to content

Ship soft Category filter (replace phone select blue highlight) - #1606

Closed
BigSimmo wants to merge 19 commits into
mainfrom
cursor/category-filter-mockups-8e31
Closed

Ship soft Category filter (replace phone select blue highlight)#1606
BigSimmo wants to merge 19 commits into
mainfrom
cursor/category-filter-mockups-8e31

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Summary

  • Ships Direction A as the shared phone MobileResultFilterControl (soft value button + portaled menu) across Tools and the other results-band surfaces that previously used a native <select>.
  • Hardens the control against review findings: viewport clamp/flip without overflowing the top, close when the phone trigger hides or leaves the viewport, skip disabled options for keyboard/typeahead focus, soft-disable placeholders with aria-disabled so taps are ignored instead of blur-dismissing the menu, ignore Ctrl/Cmd/Alt chords and unmatched letters in typeahead, preventScroll on option focus, Safari mousedown guards on options/menu/open trigger, resting-value accent, unmatched-value label, unmatched-value typeahead starts at option 0, idempotent onChange, accessible name containing visible text, dismissable-layer memoization, text-xs value typography, min-w-[7rem] floor on the compressible phone filter rail slot, skip unchanged menuBox writes on scroll, restore trigger focus on toggle-close and Tab from a portaled option.
  • Updates category-filter mockups so RESULT_COUNTS / ToolList / selected-state headers stay consistent, and README matches the shipped Direction A state.

Test plan

  • npx vitest run tests/search-results-header-band.dom.test.tsx (36 passed)
  • npm run format
  • CI pr-required on the pushed head
  • Spot-check phone filters on Tools / Specifiers / Formulation / Services (Chromium); physical iPhone Safari still recommended for the Safari focus quirk

Clinical Governance Preflight

Not clinical-risk scope: UI filter chrome / mockups only. No ingestion, answer generation, retrieval/ranking, source rendering, document access, privacy, or production env changes.

RAG impact: no retrieval behaviour change — phone result-filter chrome and design-scratch mockups only.

Open in Web Open in Cursor 

Summary by CodeRabbit

  • New Features

    • Added a category-filter directions prototype with multiple interactive design options and comparison examples.
    • Introduced redesigned mobile filter menus with clearer active states and improved accessibility.
  • Bug Fixes

    • Improved keyboard navigation, focus restoration, dismissal behavior, and positioning for mobile filter menus.
    • Enhanced filter rendering in constrained spaces and responsive layouts.
  • Documentation

    • Added prototype guidance and included the new route in the sitemap.
  • Tests

    • Updated automated coverage for menu interactions, accessibility, selection behavior, and responsive layouts.

Explore five replacements for the Tools Category native-select blue
highlight: soft value button, chip rail, custom listbox, segments+More,
and quiet accent rail — under /mockups/category-filter-directions.
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 53 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 574e54f1-ba60-406f-b53f-835a1c319498

📥 Commits

Reviewing files that changed from the base of the PR and between af4adbc and bccd3ba.

📒 Files selected for processing (7)
  • docs/branch-review-ledger.md
  • src/components/category-filter-directions-mockups.tsx
  • src/components/clinical-dashboard/search-results-header-band.tsx
  • src/components/services/services-navigator-page.tsx
  • tests/search-results-header-band.dom.test.tsx
  • tests/ui-smoke.spec.ts
  • tests/ui-tools.spec.ts
📝 Walkthrough

Walkthrough

Added a category-filter design-study route with five mobile directions. Replaced mobile native selects with accessible portaled menus and updated related DOM, accessibility, interaction, and layout tests.

Changes

Category filter flow

Layer / File(s) Summary
Category-filter mockup and route
src/app/mockups/category-filter-directions/page.tsx, src/components/category-filter-directions-mockups.tsx, src/app/mockups/mockups-layout-client.tsx, docs/site-map.md, mockups/README.md
Added the category-filter directions mockup, route metadata, sitemap entry, documentation, and mockup-specific layout exclusions.
Portaled mobile filter menu
src/components/clinical-dashboard/search-results-header-band.tsx, src/components/services/services-navigator-page.tsx
Replaced the native mobile filter with a portaled menu. Added keyboard navigation, focus restoration, dismissal behavior, positioning updates, disabled options, radio-menu semantics, and the restingValue integration.
Filter interaction and layout coverage
tests/search-results-header-band.dom.test.tsx, tests/ui-accessibility.spec.ts, tests/ui-formulation.spec.ts, tests/ui-smoke.spec.ts, tests/ui-specifiers.spec.ts, tests/ui-tools.spec.ts, tests/ui-stress.spec.ts
Updated tests for custom menu interaction, accessible names, data-value assertions, resting values, and revised mobile inset measurements.

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

Sequence Diagram(s)

sequenceDiagram
  participant SearchResultsHeaderBand
  participant FilterTrigger
  participant PortaledMenu
  participant BrowserFocus
  SearchResultsHeaderBand->>FilterTrigger: Render selected filter value
  FilterTrigger->>PortaledMenu: Open menu
  PortaledMenu->>BrowserFocus: Focus active option
  BrowserFocus->>PortaledMenu: Navigate with keyboard
  PortaledMenu->>SearchResultsHeaderBand: Select menuitemradio option
  SearchResultsHeaderBand->>BrowserFocus: Restore trigger focus
Loading

Possibly related PRs

Suggested labels: codex

Suggested reviewers: claude

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% 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
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.
Title check ✅ Passed The title clearly and concisely describes the main change: replacing the phone Category select highlight with a soft filter control.
Description check ✅ Passed The description includes the summary, targeted verification, test results, UI scope, and clinical governance impact; full UI verification is noted as pending.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

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

cursoragent and others added 2 commits August 4, 2026 10:39
Render option panels in-flow (not clipped), and seed resting vs open
frames for each direction so the study is reviewable without clicking.
@BigSimmo

BigSimmo commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

@cursoragent Unblock the current open PR. Confirm the PR number and GitHub head first from context. If more than one open PR could apply, stop and say which one you would use and why.

Fetch and start from the remote tip that matches that GitHub head. If the named branch ref is missing or stale, use the PR head ref. Preserve unrelated local WIP; do not discard dirty work; do not treat a local-ahead commit as the reviewed tip. Do not merge the PR, force-push, rebase, or arm auto-merge. No provider-backed gates without approval. If you cannot push or resolve threads, diagnose and comment only; if inline replies fail, resolve when possible and put blocker dispositions in the summary comment. If auto-merge is already armed, push only for a real blocker and avoid cancelling in-flight required CI unless the push clears that blocker.

If the PR is already merged or closed: confirm outcome, post one summary, and stop.

Primary job: clear mergeability and required-CI blockers on this tip. Do a full product review only if needed to explain or fix a blocker. Stay light otherwise—no broad verification suites or Playwright unless needed to prove one concrete unblock fix.

Snapshot the GitHub head: tip, behind/ahead, mergeable state, merge-tree versus origin/main (real conflict versus behind-but-clean), required checks on that tip including Production UI when selected, and unresolved threads that block merge or required CI. Missing checks while dirty are not green. Tip moved → re-snapshot.

Ledger-lookup against that GitHub head under an unblock/fix scope for this PR. If already handled at this head for unblock work, merge-tree clean, required checks green, and no blocking threads: summarize, comment, stop.

Unblock once: real conflict → merge origin/main with the smallest correct resolution (prefer main’s shared queues; keep this PR’s notes); behind-but-clean → one sync, late if required CI is in flight; clean → leave. skip-branch-sync and similar labels do not block a manual sync needed to clear a real blocker. No sync thrash. After any sync or push, re-snapshot tip, merge-tree, and required checks before declaring done. Dedupe the ledger if a merge touched it.

Fix only what blocks required checks or mergeability on this tip. Ignore advisory red unless it reveals a real required failure. Prefer the smallest safe fix; if a bot/autofix commit caused the break, prefer revert. Prove with the smallest targeted check only. Format and commit before push.

Push only blocker-fix commits. Append ledger for the final GitHub head under the unblock scope. Never push a ledger-only tip.

One PR comment: tip, sync/merge-tree, what blocked and what you fixed or dispositioned, required CI status, residual risks; merge left to me.

Stop when merge-tree is clean on the current GitHub tip, no blocking threads remain for merge/required CI, and required checks are green or clearly in progress.

@BigSimmo
BigSimmo marked this pull request as ready for review August 4, 2026 17:02
@cursor

cursor Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

❌ Unable to add follow-up after multiple attempts. The agent may be experiencing issues.

Please try again later or Open in Web to view the status.

Open in Web Open in Cursor 

Learn more about Cursor Agents

@BigSimmo
BigSimmo marked this pull request as draft August 4, 2026 17:03

@devin-ai-integration devin-ai-integration 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.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

Ship Direction A as MobileResultFilterControl: custom button + portaled
menu with accent wash for open/filtered state, keyboard parity with DSM,
and no native-select blue highlight. Update unit and Playwright callers.
@cursor cursor Bot changed the title Category filter mockups: replace phone select blue highlight Ship soft Category filter (replace phone select blue highlight) Aug 4, 2026
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

CI triage

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

  • Static PR checksneeds 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 #8077 (cancelled).

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

@BigSimmo

BigSimmo commented Aug 5, 2026

Copy link
Copy Markdown
Owner Author

@cursoragent Work the current open PR end-to-end. Confirm the PR number and GitHub head first from context. If more than one open PR could apply, stop and say which one you would use and why.

Fetch and start from the remote tip that matches that GitHub head. If the named branch ref is missing or stale, use the PR head ref. Preserve unrelated local WIP, including any local-only ledger commits; do not discard dirty work, and do not treat a local-ahead commit as the reviewed tip. Do not merge the PR, force-push, rebase, or arm auto-merge unless I explicitly ask. No provider-backed gates without separate approval. If you cannot push or resolve threads, diagnose and comment only; if inline replies fail, resolve when possible and put dispositions in the summary comment. If auto-merge is already armed, push only for a real blocker, and avoid pushes that would cancel in-flight required CI unless the push itself clears that blocker.

If the PR is already merged or closed: confirm the head and merge commit, note required-CI outcome, post one summary, and stop.

Goal: deep review plus Bugbot, fix actionable issues with the smallest correct changes, clear merge / required-CI / thread blockers, run strong local offline verification, push fixes, append the review ledger, and post one PR summary. Prefer thoroughness over speed. Regenerate large assets only when a fix requires it; then run the asset check and keep compatibility aliases byte-identical where the repo uses them.

Snapshot the GitHub head SHA: tip, base, behind/ahead, mergeable state, merge-tree versus origin/main (real conflict versus behind-but-clean), required checks on that tip including Production UI when selected, advisory separately, unresolved actionable threads. Missing checks while dirty are not green. If the tip moves mid-work, re-snapshot and continue from the new head.

Ledger-lookup against that GitHub head under the heavy review-and-fix scope for this PR. Already reviewed at this head with clean merge-tree, green required checks, and no new actionable threads → summarize, comment, stop unless I asked for a fresh superseding pass. Follow the repo review protocol.

Unblock once: real conflict → merge origin/main (prefer main’s shared queues; keep this PR’s notes); behind-but-clean → one sync, late if required CI is in flight; clean → leave. Labels like skip-branch-sync do not block a manual sync needed to clear a real blocker. No sync thrash. After any sync or push, re-snapshot tip, merge-tree, and required checks before declaring done. Dedupe the ledger if a merge touched it.

Review high-confidence delta risks only. Separate PR-introduced defects from pre-existing re-emitted debt. For generated assets, review contracts, aliases, cache, and manifests—not every generated line. If protected RAG or ranking surfaces are touched, say so before editing; fix PR-body policy text only when wrong or missing. Ignore bot noise. No nit spam or broad rewrites. Escalate verification by touched risk: clinical / RAG / privacy / migrations / auth → domain check plus production-readiness when warranted; UI / phone-chrome / routing / styling → ensure plus phone-chrome or UI gates when warranted; generated assets or docs inventory/links → asset or docs checks when warranted.

Fix P0/P1 always; clear scoped P2 when locally provable; else disposition and resolve. Required-check failures on this tip only; ignore advisory. Prefer reverting a bad autofix commit. After fixes: format and commit before push; smallest targeted proof; repo cheap gate; repo PR-local gate. No release, lighthouse, live eval, or live provider gates without approval.

Push only this PR’s fix commits. Append ledger for the final GitHub head under the heavy scope. Never push a ledger-only tip—include ledger in a real fix push, or leave a no-change ledger append local and unpushed.

One PR comment: tip, sync/merge-tree, fixed versus dispositioned, threads, required CI, decisive local gate lines, residual risks; merge left to me. Inline only for remaining human-needed P0/P1.

Stop when merge-tree is clean on the current GitHub tip, actionable threads are clear, required checks are green or clearly in progress, heavy gates for scope passed, summary posted, merge left to me.

REMEMBER TO GROUND YOUR RESPONSES AGAINST THE CURRENT REPO AND ENSURE THAT THE EDITS DO NOT CAUSE REGRESSION AND ADDITIONALLY, the edits make sense and take the entire repo and branch into account.

@cursor

cursor Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

❌ Unable to add follow-up after multiple attempts. The agent may be experiencing issues.

Please try again later or Open in Web to view the status.

Open in Web Open in Cursor 

Learn more about Cursor Agents

@BigSimmo
BigSimmo marked this pull request as ready for review August 5, 2026 09:39
chatgpt-codex-connector[bot]

This comment was marked as resolved.

@BigSimmo

BigSimmo commented Aug 5, 2026

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. The only repository destination is BigSimmo/Database, and the only branch destination is the pull request head branch cursor/category-filter-mockups-8e31 at starting commit 79e615d; never publish fixes to a detached or synthetic work branch and never create a stacked pull request. Use the authenticated GitHub connector to commit each approved fix to BigSimmo/Database:cursor/category-filter-mockups-8e31, then verify that the pull request head contains the published commit before reporting success. 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. For a fixed thread, reply with as the first line and as the second line. For a no-code disposition, use followed by . These result markers authorize the workflow to close that exact thread only after it verifies a fixed commit is the pull request head; a local-only commit is not a fix. If publication or verification fails, do not use either result marker, do not claim success, and leave the thread open with the blocker. If human input or new authorization is required, do the same. 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. Delightful!

Reviewed commit: 79e615dc3a

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

devin-ai-integration[bot]

This comment was marked as resolved.

Drop the setState-in-effect reset the menu-box layout effect did on close, and match the sort group's 50px phone height so the utility rail stays on one line. The medication edge-inset assertion now measures the trigger's content inset, which is what the old inner select reported.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
devin-ai-integration[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

Clamp/flip the portaled menu in-viewport, close it when the phone trigger
hides, skip disabled options for keyboard/typeahead focus, and align a11y
plus mockup selected-state chrome with the shipped soft filter control.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
coderabbitai[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

cursoragent and others added 2 commits August 5, 2026 11:58
Clear menu/focus state outside effects, and drive typeahead timing from
keyboard event.timeStamp so eslint react-hooks rules stay green in CI.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Bail out of MobileResultFilterControl first-letter typeahead when
Ctrl/Cmd/Alt is held so browser shortcuts are not preventDefault'd,
and clear the typeahead debounce timer on unmount.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Prevent mousedown focus steal on the open trigger so blur-driven close
does not race the click and reopen the menu on iPhone/Safari.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

cursoragent and others added 4 commits August 5, 2026 12:31
After the Safari mousedown guard, toggle-close unmounted the focused
option onto document.body; put focus back on the trigger so Tab continues
from the filter control.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Restore trigger focus when the open phone filter is toggled closed so Tab
does not restart at document.body, start unmatched-value typeahead at the
first option, keep the menu open while dragging its scrollbar, skip
unchanged menuBox writes on scroll, and return focus on Tab from a
portaled option.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Keeps trigger-focus restore from the remote tip plus typeahead unmatched
start, Tab return-focus, scrollbar dismiss guard, and menuBox scroll churn
skip from the local pass.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Soft-disable menu placeholders with aria-disabled so Chrome taps no longer
blur-dismiss the open phone filter, floor the compressible rail slot at
7rem so values stay readable on 320px, and only preventDefault typeahead
keys that actually match an option.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
@BigSimmo

BigSimmo commented Aug 5, 2026

Copy link
Copy Markdown
Owner Author

Review-finding closeout (head bf54a707)

Additional gaps from the latest analysis pass are fixed on this tip:

  1. Disabled option tap dismisses menu — placeholders are soft-disabled (aria-disabled, no native disabled) so mousedown preventDefault still runs; tap is ignored and the menu stays open.
  2. Compressible rail slot — phone filter wrapper is min-w-[7rem] (still shrinkable) so the value cannot collapse to an unreadable sliver on 320px rails.
  3. Unmatched typeahead letters swallowedhandleTypeaheadKey returns false on -1, so only real matches call preventDefault.

Earlier findings on this PR remain addressed (text-xs value, off-screen close, scroll churn skip, Tab/toggle focus restore, mockup counts/headers/aria-controls, Safari guards, etc.). Info/audit notes (accessible-name ripple, restingValue audit, Playwright suite validity, OS picker omission, physical iPhone Safari spot-check) are intentional / verification notes rather than code defects.

DOM proof: npx vitest run tests/search-results-header-band.dom.test.tsx36 passed.

Prefer this branch's portal MobileResultFilterControl over main's
native select (iOS anti-zoom) so the blue-highlight fix survives.
@BigSimmo
BigSimmo marked this pull request as draft August 5, 2026 13:24
@BigSimmo
BigSimmo marked this pull request as ready for review August 5, 2026 15:20
Comment on lines +668 to +675
for (let step = 0; step < total; step += 1) {
// Same-letter repeats advance to the next match (native <select> behaviour).
const index = (start + (buffer.length === 1 ? step + 1 : step)) % total;
const option = options[index];
if (!option || option.disabled) continue;
if (option.label.toLocaleLowerCase().startsWith(buffer)) return index;
}
return -1;

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.

Bug: In matchTypeaheadOptionIndex, a multi-character typeahead search from an unmatched value will never check the last option in the list due to a modulo arithmetic error.
Severity: MEDIUM

Suggested Fix

Adjust the index calculation to correctly handle the negative start value. A potential fix is to ensure the base for the modulo operation is always non-negative, for example by calculating the index as (start + step + total) % total when the buffer length is greater than one.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/components/clinical-dashboard/search-results-header-band.tsx#L668-L675

Potential issue: The `matchTypeaheadOptionIndex` function has a logic error when
handling multi-character typeahead searches that start from an unmatched value
(`fromIndex` is -1). The index calculation `(-1 + step) % total` for the first loop
iteration (`step = 0`) results in `-1` because of how JavaScript's modulo operator
handles negative numbers. While a guard prevents a crash from accessing `options[-1]`,
this effectively wastes one iteration of the loop. As a result, the loop finishes
without ever evaluating the last item in the `options` array, making it impossible for a
user to select it under these conditions.

Did we get this right? 👍 / 👎 to inform future reviews.

@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: bccd3babb0

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

| 2026-08-04 | claude/top-search-design-mockups-w53znc | b432448e4893a42d07558aff0dc04be797971231 | PR #1611 — results-band shelf Clear filter-only, memo deps, restored tests | Fixed two Qodo findings from merged #1555; mutation-tested guard added | tsc 0; eslint 0; vitest 4 files/59 tests; verify:pr-local blocked by lock parity (node 24.13 vs jsdom@30) |
| 2026-08-04 | claude/search-bar-decisions-doc | a7dea7f777255ade72878820a636413aaf9588af | search-bar handoff doc replacement + review fixes | Docs-only review fixes: mode/shelf accounting, Sort consumers, #230/#170 precision; removed unquoted-output claim from prior row | prettier --check . ; check:outstanding-issues ; docs:check-links ; docs:check-index |
| 2026-08-04 | claude/search-bar-decisions-doc | 3b4cd6e6bf1f36fb8aff098ce7d333641e0859d3 | search-bar handoff doc replacement + review fixes | Fixed CodeRabbit/Codex findings; Bugbot hosted stuck queued, local Bugbot-equivalent confirmed two P2 doc errors and rejected sheets-are-target finding. verify:pr-local PASS (docs scope). Decisive: prettier All matched files use Prettier code style!; outstanding-issues 228 rows next-id=231; docs link check passed: 1615; docs/codebase-index coverage OK | verify:pr-local (docs); prettier --check; check:outstanding-issues; docs:check-links; docs:check-index; check:branch-review-ledger |
| 2026-08-04 | codex/fix-mode-switching-and-loading-issues | 3e3b224a2ec13928d1e28173b1fc4c75d202d7d2 | PR #1607 unblock/fix | clean — behind 0, merge-tree clean, 0 unresolved threads, required CI in progress (no code fix) | merge-tree clean; behind_by 0; Unit/Build/Static/ProdUI in progress; no failing required |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve ledger rows in place

This added row is the same codex/fix-mode-switching-and-loading-issues record that the diff removes from its previous position below, so the commit is rewriting/moving existing ledger history instead of only appending the two new cursor/category-filter-mockups-8e31 records. For ledger updates, keep existing rows byte-for-byte in place and append new records only; otherwise future syncs/reviews lose the append-only audit trail this file relies on.

AGENTS.md reference: AGENTS.md:L145-L145

Useful? React with 👍 / 👎.

BigSimmo commented Aug 5, 2026

Copy link
Copy Markdown
Owner Author

Closing as superseded by #1615 ("Results bar: one line, one filter, nothing unreachable"), which merged to main as 62ac0e0 and rewrote the same surface this PR builds on. Recording the salvage path so none of the work is lost.

Why superseded rather than fixable-forward. Verified with git merge-tree --write-tree origin/main <tip> rather than trusting GitHub's DIRTY label, per the repo's anti-churn rule — these are real content conflicts, not staleness:

CONFLICT (content): Merge conflict in src/components/clinical-dashboard/search-results-header-band.tsx
CONFLICT (content): Merge conflict in tests/search-results-header-band.dom.test.tsx

Both conflicting files are exactly the ones #1615 rewrote. The 545-line search-results-header-band.tsx diff and its 321-line test diff were written against the pre-#1615 shape of that component, so resolving them means re-deriving the control against the new one-line results bar — a redo, not a merge.

Static PR checks was also red on head bccd3ba, independently of the conflict:

- production code consumes the decoration-only --text-soft compatibility alias:
  search-results-header-band.tsx:504, :531, :1051, :1109, :1124
- legacyShadowAliases increased from 231 to 232
- textSoftConsumers increased from 0 to 5

Worth carrying into the redo deliberately: --text-soft is a deprecated alias of --decoration-soft, pinned below 4.5:1 and banned on text nodes (docs/design-system/GATES.md §1). Those five call sites need --text-muted or another sanctioned text role — closing this PR moots the gate failure but does not fix the underlying contrast issue, so it must not be reintroduced.

What is worth salvaging into the redo — all of it readable on branch cursor/category-filter-mockups-8e31, which is kept, not deleted:

  • src/components/category-filter-directions-mockups.tsx (980 lines) and its route src/app/mockups/category-filter-directions/page.tsx — the Direction A–D study. Design-scratch, 404 in production, and not in conflict with Results bar: one line, one filter, nothing unreachable #1615; this is the cheapest piece to lift across intact.
  • The MobileResultFilterControl hardening list in the PR description above is the real value and should be treated as the acceptance criteria for the redo, not rewritten from scratch: viewport clamp/flip without top overflow, close when the phone trigger hides or leaves the viewport, skip disabled options for keyboard/typeahead focus, aria-disabled soft-disabled placeholders, ignore Ctrl/Cmd/Alt chords and unmatched letters, preventScroll on option focus, Safari mousedown guards, resting-value accent, unmatched-value label and typeahead start, idempotent onChange, accessible name containing visible text, dismissable-layer memoization, min-w-[7rem] floor on the compressible rail slot, skip unchanged menuBox writes on scroll, restore trigger focus on toggle-close and on Tab from a portaled option.
  • 24 review threads, all resolved, are preserved in this PR's history — that is where the reasoning behind several of those guards lives.

Tracking: PR #1620 already carries a ledger row for this — #247 P3 task, "Redo the phone Category filter's soft menu on top of PR #1615 (salvaged from closed PR #1606)". That row anticipated this closure and is now accurate. Note that #1620 and #1624 currently both allocate ledger IDs #246/#247/#248 to different items, so whichever lands second needs renumbering before #247 is stable.


Generated by Claude Code

@BigSimmo BigSimmo closed this Aug 5, 2026

@devin-ai-integration devin-ai-integration 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.

Devin Review found 2 new potential issues.

View 1 additional finding in Devin Review.

Open in Devin Review

below `sm`, so weight and colour are the only hierarchy left against the
18px query heading. */}
<select
<div ref={rootRef} onBlur={handleBlur} className={cn("relative min-w-0", className)}>

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.

🟡 Category filter on the Tools launcher shrinks to a small box instead of filling its row on phones

The filter control's outer wrapper lost its full-width sizing (cn("relative min-w-0", className) at src/components/clinical-dashboard/search-results-header-band.tsx:1068) when it stopped being a full-width label, so on the Tools launcher the phone Category filter now sizes to its text instead of spanning the row.
Impact: The Category filter on the Tools page looks noticeably narrower than before on phones, and long category names have less room.

Flex parent no longer stretches the control now that the root is an auto-width block

The previous implementation rendered <label className="relative inline-flex min-h-tap w-full min-w-0 …">; w-full resolved against the flex container, so the control filled the row. The new root is <div className={cn("relative min-w-0", className)}> with no width utility. Inside SearchResultsHeaderBand the slot (src/components/clinical-dashboard/search-results-header-band.tsx:462-472) and the grid wrappers used by specifiers/formulation are block/grid parents, so the div still stretches there. But src/components/applications-launcher-page.tsx:851 renders FilterTabs inside <div className="flex items-center gap-3">; the control (src/components/applications-launcher-page.tsx:413-421, className="sm:hidden") is now a flex item with width: auto, so it collapses to its content width. The inner w-full button resolves against that already-shrunk box.

Suggested change
<div ref={rootRef} onBlur={handleBlur} className={cn("relative min-w-0", className)}>
<div ref={rootRef} onBlur={handleBlur} className={cn("relative w-full min-w-0", className)}>
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

const character = key.toLocaleLowerCase();
if (character.length !== 1 || /\s/.test(character)) return null;
if (state.timer !== null) window.clearTimeout(state.timer);
state.buffer = now - state.lastAt < 500 ? `${state.buffer}${character}` : character;

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.

🟡 Typing the same letter twice quickly in the filter menu jumps nowhere instead of moving to the next match

Repeated identical letters are appended into one search string (state.buffer = … at src/components/clinical-dashboard/search-results-header-band.tsx:654) rather than being treated as a request for the next match, so pressing a letter twice within half a second finds nothing and focus does not move.
Impact: Keyboard users cycling between two options that start with the same letter get no response unless they pause between presses.

Buffer accumulation defeats the documented same-letter cycling branch

matchTypeaheadOptionIndex keeps a 500 ms buffer. Typing a then a again inside that window produces buffer === "aa", so the cycling branch at src/components/clinical-dashboard/search-results-header-band.tsx:670 (buffer.length === 1 ? step + 1 : step) is not taken and the prefix test option.label.toLocaleLowerCase().startsWith("aa") matches nothing, returning -1. handleTypeaheadKey then leaves focus where it is. The comment on line 669 states the intent ("Same-letter repeats advance to the next match (native <select> behaviour)"), which only holds when the repeat lands after the debounce expires. Native <select> and the ARIA typeahead convention cycle when every buffered character is identical.

Prompt for agents
In matchTypeaheadOptionIndex (src/components/clinical-dashboard/search-results-header-band.tsx), the typeahead buffer accumulates every character typed within the 500 ms window, including repeats of the same character. Native <select> and the ARIA typeahead convention treat a run of identical characters as "advance to the next option starting with that character" rather than as a literal multi-character prefix. Today typing the same letter twice quickly produces a buffer like "aa", which matches no label and leaves focus unmoved; the cycling branch only runs when the buffer is exactly one character (i.e. after the debounce expired). Consider detecting the all-same-character case (either before appending, or when computing the search prefix) and collapsing it to the single character while using the advance-by-one step, so rapid repeats cycle through matches.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

cursor Bot pushed a commit that referenced this pull request Aug 5, 2026
* fix(gates): catch lint and type errors before push, not in CI

Two open PRs burned full CI cycles this week on defects a single local
command would have caught: #1606 on a react-hooks/set-state-in-effect lint
error, #1618 on a TS2339 for `mode.devOnly` (a union member that lacks the
property, where app-modes.ts already exports the correct `"devOnly" in mode`
guard). Neither lint nor typecheck was in the pre-push path.

Typecheck could not simply be added, because it was already unusable
(outstanding-issues #210). tsconfig.json's `include` carries
`.next/types/**/*.ts` and `.next/dev/types/**/*.ts` — gitignored build
artifacts — so deleting a page leaves the stale generated validator importing
a removed module. Reproduced rather than inferred: a planted
`.next/dev/types/validator.ts` referencing a removed mockup page yields
`error TS2307: Cannot find module .../mockups/deleted-mockup-route/page.js`,
base config exit 2, source-only config exit 0. Full source typecheck is clean
(71s cold, 8.8s warm). Red locally and green in CI is how the gate got
abandoned, which is how the real type error then reached CI.

- tsconfig.typecheck.json + `typecheck:source`: identical compiler options,
  minus the `.next` globs, with a separate tsbuildinfo so the two incremental
  caches cannot invalidate each other. Route-signature validation is not lost;
  `next build` still covers it in CI.
- guard-push.mjs gains a fourth guard running eslint over the pushed files and
  this typecheck. Verified to reproduce both defects above with CI-identical
  messages. Scoped to the lint roots and to pushes that touch TS, skips loudly
  when node_modules is absent rather than pushing people to
  GUARD_PUSH_DISABLE=1, and overridable with SKIP_STATIC_GUARD=1.

Also corrects a doc claim that made #1580 surprising: "mockups are exempt"
was being read as blanket. Mockups are exempt from the wiring and reachability
gates and nothing else — they are still typechecked, and their client chunks
still count toward check:bundle-budget, which totals every built chunk rather
than the initial production bundle. That the budget's scope contradicts
ledger #13's "not an initial production bundle" position is a real unmade
decision, now recorded as #237 rather than papered over.

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

* docs(issues): capture the phone Category soft-menu fix salvaged from PR #1606

#1606 is closed, but it carried the one fix nothing else in the queue provides:
MobileResultFilterControl's native <select> paints a harsh system-blue highlight
on phones, and #1615 keeps that native select (its change is the iOS 16px
anti-zoom rule). So the fix does not survive #1615 landing.

Records it as #238 with the two defects the redo must not repeat: the unresolved
keyboard trap on disabled options, and the set-state-in-effect lint error that
PR #1620's new pre-push guard would now catch.

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

* issues: capture #239 stale Cloud acceptance pin on PR #1617, #240 remote-container browser gate drift

* Tighten guard coordinator test

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>

* fix(gates): shared source-typecheck lease and safer static pre-push

Treat typecheck:source:internal as a shared read-only coordinator lease with
a distinct per-worktree buildinfo file, drop the pinned in-repo cache path,
and harden staticGuard: acquire a short exclusive lease (fail-open when busy),
use a private eslint cache, escalate lint on eslint policy changes, fail closed
when the push tip is not HEAD, cover eslint-rules, and add Vitest coverage.
Align hook/docs wording with the fourth guard and point CLAUDE.md at #252.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>

* fix(gates): keep tsBuildInfoFile when run-heavy has no npm_execpath

Pre-push invokes run-heavy via plain node, so the npm_execpath spawn path
was skipped and the fallback dropped effectiveForwarded — undoing the
per-worktree buildinfo injection. Also warn when staticGuard passes on a
dirty working tree.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>

* docs: refresh scripts-index for lint:changed:internal

Keep docs:check-inventory green after adding the pre-push eslint wrapper
script to package.json.

Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>

* fix(gates): address Devin findings on static pre-push guard

- Treat "Database focused-test capacity is full" as coordinator busy so
  shared typecheck slot exhaustion fails open instead of faking a type error.
- Skip source typecheck when every changed .ts path is excluded by
  tsconfig.typecheck.json (edge functions, archive, scratch, worktrees).
- Restore check-github-shell-access.mjs (and its Role notes) in the scripts index.

* chore(ledger): record PR #1620 babysit

* fix(gates): emit structured heavy-run admission-busy signal

Prefer exit 75 + DATABASE_HEAVY_RUN_ADMISSION_BUSY over prose matching so
tsc/eslint output that quotes busy strings cannot false-pass the static guard.

* fix(gates): tip-check only when static work runs; isolate typecheck cache

Addresses follow-up Devin on PR #1620:
- Reorder staticGuard so tip-vs-HEAD fails closed only when lint/typecheck
  will actually read the working tree; ignore tag refs in the tip check.
- Pin a distinct tsBuildInfoFile on tsconfig.typecheck.json so direct tsc
  does not collide with the base config cache (run-heavy still overrides).

* fix(gates): keep lint failures when typecheck admission is busy

Addresses Devin on PR #1620 — a prior eslint failure must still block the
push if the follow-up source typecheck cannot get a coordinator slot.

---------

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 Aug 7, 2026
…d-end reachability

The roving-tabIndex implementation landed in 17d7ba6 with no test coverage, so
the contract it added could regress silently. Adds DOM coverage that fails
without it: one tab stop per group, Arrow/Home/End moving focus and selection,
wrap staying inside a dimension, the checked placeholder as the tab stop, and
the fallback when `value` matches no option. Verified by mutation — removing the
roving tabIndex and the key handler fails exactly these three. Adds the same
assertions as Chromium coverage in ui-accessibility, because jsdom cannot vouch
for focus behaviour under the sheet's real focus trap.

Dead ends now sit on the arrow path but are never selected by it. Previously
they were excluded from both the arrow cycle and the tab order, so a
keyboard-only reader could not reach the option or hear its "Not selectable from
here" note — while the comment claimed they stayed Tab-reachable, which
`tabIndex={-1}` denies. Moving focus without committing selection is the ARIA
guidance for a disabled radio and is the only arrangement that keeps the single
tab stop the role requires; giving dead ends `tabIndex={0}` would restore
reachability by adding a tab stop per option to a control whose point is having
one. No call site produces a dead end today, so this path is defensive and is
asserted rather than assumed.

Also corrects two records against the code: `#253` claimed the sheet inherits
native radio semantics, when it is a bespoke roving handler over buttons, and
its execution-queue row still asked for a `#1606` hand-merge that the deletion
of MobileResultFilterControl made impossible. Disambiguates the six-versus-seven
count — six band modes shipped a select, plus the tools launcher, which carries
no band.

Removes `activeResultFilterCount`, which no caller ever used.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013mcqh46SDdiR5KxScihkFQ
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.

2 participants