Skip to content

fix(web): separate adjacent highlighted list items by 1px - #11378

Open
jaytel0 wants to merge 2 commits into
pingdotgg:mainfrom
jaytel0:fix/web-list-item-spacing
Open

jaytel0 wants to merge 2 commits into
pingdotgg:mainfrom
jaytel0:fix/web-list-item-spacing

Conversation

@jaytel0

@jaytel0 jaytel0 commented Sep 12, 2026 •

Copy link
Copy Markdown

What Changed

Add a 1px gap between adjacent menu and listbox options so a hovered or keyboard-highlighted row stays visually separate from the selected row. This covers menu actions, radio and checkbox items, submenu triggers, selects, comboboxes, and the command palette.

Virtualized branch and font pickers use a measured item separator. The font picker’s height calculation includes those separators.

Why

In the Reasoning menu, the selected row and the neighboring hovered row currently touch, making their backgrounds run together. The same issue occurs in the shared option components.

Sibling selectors add space only between neighboring options, preserving row heights, popup padding, labels, and section separators. Virtualized rows use the list’s separator API so scrolling accounts for the extra pixel. Existing sidebar and model-picker spacing is preserved.

UI Changes

Before After
Selected Low and hovered Medium before the fix The same state with a 1px gap
reasoning-hover.mp4

Validation

  • Web package typecheck passed; existing Effect suggestions remain.
  • Targeted lint and formatting passed. Lint reports seven warnings on unchanged code in the branch and font pickers.
  • Verified the Reasoning menu and branch picker in the isolated web app: exactly 1 CSS pixel between rows; 28px row heights retained.
  • Browser verification of the actual shared components covered mixed menu items, groups, section separators, submenus, select keyboard selection, command keyboard navigation, filtering and clearing, light/dark themes, and 1px spacing after scrolling a 100-row virtualized list. At a 390px viewport, menu rows retained their 32px height.
  • Web and Electron share these components. The native React Native UI is unaffected; no separate Electron or native-device run was performed.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Model: GPT-6 Astra. Harness: Codex desktop.

Summary by CodeRabbit

  • Style
    • Improved visual spacing between items in branch selection, font selection, autocomplete, menus, and dropdown lists.
    • Added consistent separators and small gaps between adjacent options for clearer list organization.
    • Updated selection popups to accommodate the added spacing without clipping content.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 12, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-12T05:56:10.997841Z 6085479 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions github-actions Bot added size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list. labels Sep 12, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 6085479

Macroscope's review found this PR approvable — This is a small, focused UI fix that adds 1px separation between adjacent highlighted menu and option rows. Its runtime impact is limited to existing popup spacing and virtualized list dimensions, with no changes to data, workflows, defaults, or security-sensitive behavior.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 12, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 6e6eedfc-746f-4289-b376-3a73cbd9d38b

📥 Commits

Reviewing files that changed from the base of the PR and between 6085479 and df1b205.

📒 Files selected for processing (6)
  • apps/web/src/components/BranchToolbarBranchSelector.tsx
  • apps/web/src/components/settings/FontFamilyPicker.tsx
  • apps/web/src/components/ui/autocomplete.tsx
  • apps/web/src/components/ui/combobox.tsx
  • apps/web/src/components/ui/menu.tsx
  • apps/web/src/components/ui/select.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/web/src/components/ui/select.tsx

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The change adds 1px spacing between adjacent options and menu items. Virtualized branch and font lists render shared separators. The font picker adjusts its popup height calculation for those separators.

Changes

UI spacing updates

Layer / File(s) Summary
Shared option and menu spacing
apps/web/src/components/ui/{autocomplete,combobox,menu,select}.tsx
Option and menu item components now apply 1px spacing between adjacent items. ComboboxItemSeparator renders an aria-hidden 1px spacer and is exported.
Virtualized list separator integration
apps/web/src/components/BranchToolbarBranchSelector.tsx, apps/web/src/components/settings/FontFamilyPicker.tsx
Branch and font lists use ComboboxItemSeparator between rows. The font picker height calculation includes separator spacing.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: maria-rcks, juliusmarminge

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 6 files. 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 clearly and concisely describes the primary change: adding a 1px separation between adjacent highlighted list items.
Description check ✅ Passed The description is complete and focused. It explains what changed and why, documents the UI impact with before-and-after images and a video, and includes validation details with all checklist items co…
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

@jaytel0

jaytel0 commented Sep 12, 2026

Copy link
Copy Markdown
Author

Reviewed the docstring-coverage warning. The new ComboboxItemSeparator already documents why the virtualized list owns its spacing. The other touched functions keep their existing behavior and only gain spacing styles or a separator prop. I’m leaving their documentation unchanged to keep this cosmetic fix focused, consistent with the repository’s guidance against documenting obvious UI behavior. CodeRabbit’s review reports no actionable findings and its status check passed.

@jaytel0

jaytel0 commented Sep 16, 2026

Copy link
Copy Markdown
Author

@maria-rcks — you merged the related popup-trigger and topbar-seam fixes (#10468, #10914), so I thought this small UI fix might be in your area. Could you take a look and merge if it looks good once CI passes?

This adds a 1px gap between adjacent highlighted menu/option rows, including the virtualized branch and font pickers. Before/after images and a hover video are in the description; targeted local checks and browser verification passed. Bot reviews have no unresolved actionable findings. CI is still awaiting maintainer authorization. Thanks!

@jaytel0

jaytel0 commented Sep 19, 2026 •

Copy link
Copy Markdown
Author

Resolved the conflict with upstream cleanup (#9917) in df1b205. The removed, unused combobox helpers stay removed; the 1px separator used by the virtualized branch and font pickers is retained. The diff against current main remains six files, 18 additions and one deletion.

Validation after refreshing dependencies: web typecheck passed; the resolved combobox file passed targeted formatting and lint; the PR diff passed whitespace checks. Browser verification of the current shared combobox and LegendList confirmed 28px rows with exact 1px gaps, both at the start and after scrolling to options 94–100. Earlier full UI evidence remains in the description.

CodeRabbit completed review of df1b205 with no actionable findings. CI still requires maintainer authorization; other expected bot reviews on the new head remain outstanding.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant