feat(ui): give SegmentedControl an option hint slot - #1848
Conversation
Prerequisite for the filter contract rollout (docs/filter-contract.md, PR #1847). The four one-of-N mode rails that are about to converge onto this primitive — differentials, medication, applications, specifiers — all carry a per-option count, and the primitive had nowhere to put one. Baking it into `label` would fold the number into the truncating span and lose the tabular alignment. Landing it separately because it depends on none of the contract work, and it makes each adoption a pure call-site change. Two things worth knowing: - The hint needs an explicit aria-label. Adjacent label and hint spans concatenate to "All62" in the computed accessible name, and a text node separator does not survive — the name computation normalises inter-element whitespace away. The button is therefore named "All (62)", the shape the differentials rail already used. Both behaviours are pinned by tests, including that a hintless option's name does not drift. - No .design-sync regeneration is needed, contrary to expectation: dtsPropsFor.SegmentedControl references SegmentedControlOption<string> by name rather than expanding it, so adding a field leaves the pinned props string unchanged. Verified — all 56 design-sync tests pass. Note for the adoption PRs: ResultFilterSheet renders option.hint the same adjacent-span way, so its options announce "Crisis12" today. Same fix, tracked with the rollout rather than smuggled in here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:39 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 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds optional hints to ChangesSegmentedControl hint support
Next.js agent guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant SegmentedControl
participant RadioInput
participant HintSpan
SegmentedControl->>RadioInput: set accessible name to label (hint)
SegmentedControl->>HintSpan: render hint with reserved numeric column
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
Regenerated by the production build run for this PR's bundle-budget check. Written by next dev via node_modules/next/dist/server/lib/generate-agent-files.js; left uncommitted it re-appears on every build and leaves a dirty tree. Its own commit rather than folded into the SegmentedControl change, so it can be dropped independently. Note this block is not yet on main — PR #1846 also carries it, so whichever lands first makes the other a no-op. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:d5ff316024
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #9872 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
…ft alias CI Static PR checks failure: --text-soft is a decoration-only compatibility alias that production must not consume, and check:design-system-contract ratchets its consumer count at zero (textSoftConsumers increased from 0 to 1). --text-muted is what ResultFilterSheet already uses for option.hint, so this also makes the desktop rail and the phone sheet render the same count the same way, which was the point of adding the slot. Same root cause as the radius fix on #1847: lifted from the mockup study, where the token gates do not apply. Also merges origin/main to clear the 26-commit staleness that failed PR mergeability. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/ui-v2-components.dom.test.tsx (1)
396-410: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that hintless options render no hint span.
Line 409 verifies only the radio accessible name. A regression could still render an empty
.tabular-numsspan and pass this test while adding width or spacing. Add an assertion for the absence of the hint span.Proposed test assertion
- expect(screen.getByRole("radio", { name: "All" })).toBeInTheDocument();+ const radio = screen.getByRole("radio", { name: "All" });+ expect(radio).toBeInTheDocument();+ expect(radio.querySelector(".tabular-nums")).not.toBeInTheDocument();🤖 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/ui-v2-components.dom.test.tsx` around lines 396 - 410, The hintless option test should also verify that no hint span is rendered. In the test “leaves an option without a hint unchanged,” assert that the hint-specific `.tabular-nums` element is absent while preserving the existing accessible-name assertion.
🤖 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.
Nitpick comments:
In `@tests/ui-v2-components.dom.test.tsx`:
- Around line 396-410: The hintless option test should also verify that no hint
span is rendered. In the test “leaves an option without a hint unchanged,”
assert that the hint-specific `.tabular-nums` element is absent while preserving
the existing accessible-name assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f069826d-c1a3-4350-afb3-1efb4d7f8bdd
📒 Files selected for processing (4)
AGENTS.mddocs/branch-review-ledger.mdsrc/components/ui/segmented-control.tsxtests/ui-v2-components.dom.test.tsx
Both heads fixed the same CI failure — production must not consume the decoration-only text alias — and theirs is the better fix, so this resolves entirely in its favour and drops mine. Theirs removes the hint's own colour so it inherits the button's opaque foreground, which sidesteps the alias without introducing a second token that would have to stay legible against both the checked and unchecked backgrounds. It also reserves a three-digit column with min-w-6 and text-right: tabular figures keep digit widths equal but do not stop the span growing at 9 -> 10, so without it a live count still moves the segment bounds. That was a real gap in my version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
…ntrol-count' into HEAD
…into claude/segmented-control-count
Summary
Prerequisite for the filter contract rollout (
docs/filter-contract.md, PR #1847). The four one-of-N mode rails about to converge onto this primitive — differentials, medication, applications, specifiers — all carry a per-option count, andSegmentedControlOptionhad nowhere to put one. Baking it intolabelwould fold the number into the truncating span and lose the tabular alignment.Landing it separately because it depends on none of the contract work, and it turns each mode adoption into a pure call-site change.
hint?: stringtoSegmentedControlOption, rendered as anums tabular-numssibling span so the rail does not reflow as a live count crosses a digit boundary.ResultFilterOption.hint, so a mode can build one option array and hand it to both the desktop rail and the phone sheet — which is the duplication the rollout is removing (differentials currently declares its three counts twice, once ascountand once ashint).Two findings worth recording
The hint needs an explicit
aria-label. Adjacent label and hint spans concatenate to "All62" in the computed accessible name, and a text-node separator does not survive — the name computation normalises inter-element whitespace away. I tried the whitespace fix first and measured that it did not work. The button is now named"All (62)", which is the shape the differentials rail already used before this. Both behaviours are pinned, including that a hintless option's name does not drift, since every existing call site passes no hint.No
.design-syncregeneration is needed — contrary to what I expected going in.dtsPropsFor.SegmentedControlreferencesSegmentedControlOption<string>by name rather than expanding it, so adding a field leaves the pinned props string unchanged. Verified rather than assumed: all 56 tests intests/design-sync-visual-exports.test.tspass untouched.Carried forward, not smuggled in
ResultFilterSheetrendersoption.hintthe same adjacent-span way, so its options announce "Crisis12" today. Same defect, same fix — tracked with the rollout rather than folded into an unrelated primitive PR.Verification
npm run verify:pr-local—lint,typecheckandtestgreen.buildfailed once on a corrupt generated.next/dev/types/validator.ts; that was the dev server writing into.nextwhile it was being deleted, not this diff. Re-run with the server stopped and a clean.next:tests/ui-v2-components.dom.test.tsx— 80 passed, including two new tests: the hint renders and reaches the accessible name as"All (62)", and a hintless option's name is unchanged.tests/design-sync-visual-exports.test.ts— 56 passed, no config regeneration.npm run check:bundle-budgetafterrm -rf .next && npm run build— production 1302.5 KiB and mockups 286.5 KiB, both within tolerance.No UI verification: no call site passes
hintyet, so there is no rendered change.git diff --stat origin/mainis the primitive, its tests, and thenext devAGENTS.md block. Browser proof belongs on the first adoption PR.Risk and rollout
hintand renders identically. Thearia-labelis only set when a hint is present, so no existing accessible name changes.Next in the rollout, once #1847 lands: one PR adopting
kind: "lens"across differentials, medication, applications and specifiers, replacing each mode'saria-presseddesktop rail with this control so the two breakpoints stop disagreeing about whether the dimension is one-of-N.Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation