feat(tabs): scroll tab strip with the mouse wheel#1136
Conversation
📝 WalkthroughWalkthroughA persisted Tab Bar Wheel Scroll
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR adds mouse-wheel scrolling for overflowing tab strips. The main changes are:
Confidence Score: 4/5The custom-port default mismatch should be fixed before merging.
src/renderer/stores/settingsStore.ts, src/renderer/components/TabBar/TabBar.tsx, src/renderer/components/Settings/tabs/DisplayTab.tsx Important Files Changed
Reviews (1): Last reviewed commit: "feat(tabs): scroll tab strip with the mo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| persistentWebLink: false, | ||
| webInterfaceUseCustomPort: false, | ||
| webInterfaceCustomPort: 8080, | ||
| webInterfaceCustomPort: 9080, |
There was a problem hiding this comment.
Changing only the renderer default to 9080 leaves the main settings defaults, shared metadata, and web-server fallback at 8080. On a fresh install or reset with no persisted webInterfaceCustomPort, the UI can show 9080 while the server still starts on 8080, so users who enable the custom port without retyping it get a broken URL.
| const delta = Math.abs(e.deltaX) > Math.abs(e.deltaY) ? e.deltaX : e.deltaY; | ||
| if (delta === 0) return; | ||
| el.scrollLeft += delta; | ||
| e.preventDefault(); |
There was a problem hiding this comment.
When the tab strip overflows and is already at the left or right scroll limit, el.scrollLeft += delta is clamped and produces no movement, but this line still consumes the wheel event. A vertical wheel over the tab bar can then stop the surrounding content from scrolling even though the tab strip could not pan further.
| </div> | ||
|
|
||
| {/* Scroll Tab Strip With Mouse Wheel */} | ||
| <div | ||
| className="flex items-center justify-between pt-3 border-t" | ||
| style={{ borderColor: theme.colors.border }} | ||
| > | ||
| <div> | ||
| <p className="text-sm" style={{ color: theme.colors.textMain }}> | ||
| Scroll tabs with the mouse wheel | ||
| </p> | ||
| <p className="text-xs opacity-50 mt-0.5"> | ||
| When the tab strip overflows, hover over it and scroll the mouse wheel to pan the | ||
| tabs left and right. Disable to ignore the wheel over the tab bar. | ||
| </p> | ||
| </div> | ||
| <button | ||
| onClick={() => setTabBarWheelScroll(!tabBarWheelScroll)} | ||
| className="relative w-10 h-5 rounded-full transition-colors flex-shrink-0 outline-none" | ||
| tabIndex={0} | ||
| style={{ | ||
| backgroundColor: tabBarWheelScroll ? theme.colors.accent : theme.colors.bgActivity, | ||
| }} | ||
| role="switch" | ||
| aria-checked={tabBarWheelScroll} | ||
| aria-label="Scroll tabs with the mouse wheel" | ||
| > | ||
| <span | ||
| className={`absolute left-0 top-0.5 w-4 h-4 rounded-full bg-white transition-transform ${ | ||
| tabBarWheelScroll ? 'translate-x-5' : 'translate-x-0.5' | ||
| }`} |
There was a problem hiding this comment.
This adds a user-facing setting without its own data-setting-id or matching search/metadata entry. Users searching Settings for wheel, scroll, or tab will not be taken to this toggle, and metadata-driven settings surfaces will not know the new setting exists.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/components/TabBar/TabBar.tsx (1)
155-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
useEventListener()for the wheel binding.This manual
addEventListener/removeEventListenerpair diverges from the renderer standard. Please wire this throughuseEventListener()and keep the{ passive: false }option there instead of managing the native listener insideuseEffect. As per coding guidelines,src/renderer/**/*.{ts,tsx}should “UseuseEventListener()fromsrc/renderer/hooks/utils/useEventListener.tsfor event listener management. Do not pair rawaddEventListener/removeEventListenerinside useEffect.”🤖 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/renderer/components/TabBar/TabBar.tsx` around lines 155 - 173, The wheel handler in TabBar should be managed through the renderer’s shared event-listener hook instead of a manual native subscription. Update the TabBar component to use `useEventListener()` from `src/renderer/hooks/utils/useEventListener.ts` for the tab strip wheel binding, preserving the current `tabBarWheelScroll` gating, `handleWheel` logic, and `{ passive: false }` option there rather than inside `useEffect`. This should remove the direct `addEventListener`/`removeEventListener` pairing from `TabBar.tsx` while keeping the same scroll behavior.Source: Coding guidelines
🤖 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 `@src/renderer/components/TabBar/TabBar.tsx`:
- Around line 155-173: The wheel handler in TabBar should be managed through the
renderer’s shared event-listener hook instead of a manual native subscription.
Update the TabBar component to use `useEventListener()` from
`src/renderer/hooks/utils/useEventListener.ts` for the tab strip wheel binding,
preserving the current `tabBarWheelScroll` gating, `handleWheel` logic, and `{
passive: false }` option there rather than inside `useEffect`. This should
remove the direct `addEventListener`/`removeEventListener` pairing from
`TabBar.tsx` while keeping the same scroll behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dde75335-8d0a-4df8-b0b7-b2466aafd0ce
📒 Files selected for processing (4)
src/renderer/components/Settings/tabs/DisplayTab.tsxsrc/renderer/components/TabBar/TabBar.tsxsrc/renderer/hooks/settings/useSettings.tssrc/renderer/stores/settingsStore.ts
When the tab strip overflows, hovering over it and using the mouse
wheel now pans the tabs horizontally (VS Code style). A plain vertical
wheel (deltaY) is translated into horizontal scroll; trackpads and
Shift+wheel emit deltaX, so the handler follows whichever axis the
device reports.
The listener is attached natively with { passive: false } because
React's synthetic onWheel runs passively and cannot call
preventDefault, which is needed to stop an outer container from also
scrolling.
Gated behind a new "Scroll tabs with the mouse wheel" toggle in
Settings > Display (tabBarWheelScroll, default on).
peterjmorgan
force-pushed
the
feat/tab-bar-wheel-scroll
branch
from
d61dd74 to
5f237ff
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d61dd74b79
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| persistentWebLink: false, | ||
| webInterfaceUseCustomPort: false, | ||
| webInterfaceCustomPort: 8080, | ||
| webInterfaceCustomPort: 9080, |
There was a problem hiding this comment.
This unrelated default change makes the renderer start from 9080 while src/main/stores/defaults.ts, src/shared/settingsMetadata.ts, the web-server fallback, the Live overlay placeholder, and existing tests still define 8080. If the renderer default is consumed before or without successful settings hydration, the custom-port UI can show or persist a conflicting default that is unrelated to tab scrolling.
Useful? React with 👍 / 👎.
| showFilePreviewsInUnreadFilter: false, | ||
| useCmd0AsLastTab: true, | ||
| showBrowserTabDomain: true, | ||
| tabBarWheelScroll: true, |
There was a problem hiding this comment.
When this setting is managed outside the Display tab, it is not discoverable or resettable because the new key is added only to the renderer store and UI. The root AGENTS.md says new settings must also be registered in src/shared/settingsMetadata.ts, src/main/stores/defaults.ts, and src/renderer/components/Settings/searchableSettings.ts; the CLI list/reset/get paths rely on SETTINGS_METADATA, and Settings search relies on searchableSettings, so tabBarWheelScroll is omitted from CLI listings, cannot be reset by maestro-cli settings reset tabBarWheelScroll, and searches like mouse wheel do not find the toggle.
Useful? React with 👍 / 👎.
| // VS Code convention; trackpads and Shift+wheel emit deltaX, so we follow | ||
| // whichever axis the device reports. Attached as a native, non-passive | ||
| // listener because React's synthetic onWheel runs passively and cannot call | ||
| // preventDefault — without it the wheel would also scroll an outer container. |
There was a problem hiding this comment.
The root AGENTS.md writing style forbids em dash and en dash characters anywhere, including code comments. This newly added comment contains U+2014, so the patch violates the repo's mandatory style even though the code still runs.
Useful? React with 👍 / 👎.
| // preventDefault — without it the wheel would also scroll an outer container. | ||
| useEffect(() => { | ||
| const el = tabBarRef.current; | ||
| if (!el || !tabBarWheelScroll) return; |
There was a problem hiding this comment.
With tabBarWheelScroll off, this effect returns without installing any wheel listener, but the tab bar remains an overflow-x-auto element. Trackpads and Shift+wheel produce horizontal deltaX, so the browser's native horizontal scrolling still moves the tab strip even though the Display toggle says disabling it ignores the wheel over the tab bar; users disabling the feature because wheel gestures interfere still see those inputs move tabs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/renderer/stores/settingsStore.ts`:
- Around line 2537-2539: The hydration logic for tabBarWheelScroll in
settingsStore should validate the persisted value’s type before applying it,
since allSettings is a Record<string, unknown> and checking only for undefined
allows non-boolean values like strings to pass through. Update the patch
assignment in the settings hydration path to accept only actual booleans (or
explicitly coerce/ignore invalid values) so useSettings() reloads cannot
preserve a truthy non-boolean and TabBar.tsx reads the setting correctly.
🪄 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
Run ID: fc286072-931f-4b95-9cd8-51fc84a6279d
📒 Files selected for processing (4)
src/renderer/components/Settings/tabs/DisplayTab.tsxsrc/renderer/components/TabBar/TabBar.tsxsrc/renderer/hooks/settings/useSettings.tssrc/renderer/stores/settingsStore.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/hooks/settings/useSettings.ts
- src/renderer/components/Settings/tabs/DisplayTab.tsx
- src/renderer/components/TabBar/TabBar.tsx
| if (allSettings['tabBarWheelScroll'] !== undefined) | ||
| patch.tabBarWheelScroll = allSettings['tabBarWheelScroll'] as boolean; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the persisted type before hydrating tabBarWheelScroll.
Line 2537 reads from Record<string, unknown> but only guards against undefined, so a non-boolean persisted value survives unchanged. Because useSettings() reloads on external settings changes and TabBar.tsx treats this field by truthiness, values like 'false' will still keep wheel scrolling enabled.
Suggested fix
- if (allSettings['tabBarWheelScroll'] !== undefined)
- patch.tabBarWheelScroll = allSettings['tabBarWheelScroll'] as boolean;
+ if (typeof allSettings['tabBarWheelScroll'] === 'boolean')
+ patch.tabBarWheelScroll = allSettings['tabBarWheelScroll'];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (allSettings['tabBarWheelScroll'] !== undefined) | |
| patch.tabBarWheelScroll = allSettings['tabBarWheelScroll'] as boolean; | |
| if (typeof allSettings['tabBarWheelScroll'] === 'boolean') | |
| patch.tabBarWheelScroll = allSettings['tabBarWheelScroll']; |
🤖 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/renderer/stores/settingsStore.ts` around lines 2537 - 2539, The hydration
logic for tabBarWheelScroll in settingsStore should validate the persisted
value’s type before applying it, since allSettings is a Record<string, unknown>
and checking only for undefined allows non-boolean values like strings to pass
through. Update the patch assignment in the settings hydration path to accept
only actual booleans (or explicitly coerce/ignore invalid values) so
useSettings() reloads cannot preserve a truthy non-boolean and TabBar.tsx reads
the setting correctly.
1 participant
What
When the tab strip overflows, hovering over it and using the mouse wheel now pans the tabs horizontally (VS Code style). A plain vertical wheel (
deltaY) is translated into horizontal scroll; trackpads and Shift+wheel emitdeltaX, so the handler follows whichever axis the device reports.Gated behind a new Scroll tabs with the mouse wheel toggle in Settings -> Display (
tabBarWheelScroll, default on).Why
With a normal mouse (no horizontal wheel), there was no quick way to reach off-screen tabs without dragging or using keyboard shortcuts. This matches the tab-strip behavior users expect from VS Code and modern browsers.
How
TabBar.tsx: reads the new setting and attaches a native, non-passivewheellistener to the existing scroll container. It only acts when the strip actually overflows (scrollWidth > clientWidth), adjustsscrollLeft, and callspreventDefault()so the wheel doesn't also scroll an outer container.{ passive: false }because React's syntheticonWheelruns passively and cannot callpreventDefault().settingsStore.ts: newtabBarWheelScrollboolean (defaulttrue) + setter + load-from-disk patch, mirroring the existingshowBrowserTabDomaintab setting.useSettings.ts: added field + setter toUseSettingsReturn(the hook spreads...store).DisplayTab.tsx: new toggle in the tab-display section.Testing
TabBar.test.tsx+DisplayTab.test.tsx: 272 passed.Summary by CodeRabbit