Conversation
WalkthroughRemoves ModeSwitcher from the Sidebar and refactors ModeCarousel into a hook-driven, modular carousel with new subcomponents (ModeContent, ModeHeader, ModeNavigation, AnimatedBackground) and hooks (useModeDetection, useScrollProgress, useScrollSnap); introduces types and constants for modes, icons, and labels. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant ModeCarousel
participant ScrollContainer
participant Hooks as useScrollProgress / useModeDetection / useScrollSnap
participant ModeNavigation
participant AnimatedBackground
participant App as onModeSelect
User->>ScrollContainer: user scrolls (or clicks nav)
ScrollContainer->>Hooks: scroll event -> useScrollProgress updates progress
Hooks->>ModeCarousel: progress MotionValue update
ModeCarousel->>AnimatedBackground: pass progress
AnimatedBackground->>AnimatedBackground: animate indicator position
Note over Hooks,ModeCarousel: When scroll settles (debounced)
Hooks->>Hooks: compute nearestIndex (useModeDetection)
Hooks->>ModeCarousel: notify nearestIndex
ModeCarousel->>Hooks: useScrollSnap(scroll to nearestIndex)
ModeCarousel->>App: call onModeSelect(newMode)
ModeNavigation->>ModeCarousel: button click -> request mode change
ModeCarousel->>Hooks: set index -> useScrollSnap -> scroll container
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/ModeCarousel.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx
| <Button | ||
| key={mode} | ||
| variant="ghost" | ||
| size="sm" | ||
| onClick={() => onModeSelect(mode)} | ||
| className={`relative z-10 h-9 w-9 rounded-lg transition-colors duration-200 ${isActive | ||
| ? "text-neutral-100" | ||
| : "text-neutral-400 hover:text-neutral-300" | ||
| }`} | ||
| > | ||
| <Icon className="w-4 h-4" /> | ||
| </Button> |
There was a problem hiding this comment.
Add accessible names and state to nav buttons.
These icon-only buttons expose no discernible label or pressed state, so assistive tech users just hear “button” with no context and can’t tell which mode is active. Please add an accessible name and pressed state so screen-reader and keyboard users can operate the carousel reliably.
Apply this diff to fix the issue:
<Button
key={mode}
variant="ghost"
size="sm"
onClick={() => onModeSelect(mode)}
+ aria-label={modeLabels[mode]}
+ aria-pressed={isActive}
className={`relative z-10 h-9 w-9 rounded-lg transition-colors duration-200 ${isActive
? "text-neutral-100"
: "text-neutral-400 hover:text-neutral-300"
}`}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/ModeCarousel.tsx
around lines 274 to 285, the icon-only buttons lack accessible names and state;
add an accessible name and expose pressed state by setting an aria-label (e.g.,
`aria-label={mode}` or a more descriptive string like `Switch to ${mode} mode`)
and adding `aria-pressed={isActive}` so assistive tech announces which mode is
active; keep the existing onClick and visual styles, but ensure the active state
is reflected in both `aria-pressed` and the visual styling.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx (1)
30-43: Add accessible names and state to nav buttons.Icon-only buttons lack accessible labels and pressed state, preventing assistive technology users from understanding button purpose and active mode. The
modeLabelsconstant should be imported and used foraria-label, andaria-pressedshould reflect active state.Apply this diff to fix the issue:
+import { modeLabels } from "../../constants"; + // ... existing imports ... <Button key={mode} variant="ghost" size="sm" onClick={() => onModeSelect(mode)} + aria-label={modeLabels[mode]} + aria-pressed={isActive} className={`relative z-10 h-9 w-9 rounded-lg transition-colors duration-200 ${ isActive ? "text-neutral-100" : "text-neutral-400 hover:text-neutral-300" }`} >
🧹 Nitpick comments (3)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useScrollSnap.ts (1)
22-22: Consider documenting the 5px threshold.The 5-pixel tolerance prevents unnecessary scroll adjustments, but the specific value isn't explained. Adding a comment would help future maintainers understand why this threshold was chosen.
+ // Use 5px tolerance to avoid micro-adjustments and scroll loops if (Math.abs(scrollContainer.scrollLeft - targetScrollX) > 5) {apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/AnimatedBackground/AnimatedBackground.tsx (1)
12-15: Extract hardcoded dimensions to shared constants.The hardcoded
buttonWidth(36px) andgap(4px) values are duplicated from Tailwind classes and could drift if the styles change. Consider extracting these to theconstants.tsfile alongsidemodeIconsandmodeLabelsfor better maintainability.apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/ModeCarousel.tsx (1)
72-81: Consider extracting inline styles to a constant.The large inline style object could be extracted to a constant (e.g.,
CAROUSEL_SCROLL_STYLES) for better readability and potential reuse. However, this is a minor optimization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/ModeCarousel.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/AnimatedBackground/AnimatedBackground.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/AnimatedBackground/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeContent/ModeContent.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeContent/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeHeader/ModeHeader.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeHeader/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/constants.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useModeDetection.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useScrollProgress.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useScrollSnap.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeContent/index.ts
🧰 Additional context used
🧬 Code graph analysis (8)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/types.ts (1)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/index.ts (2)
SidebarMode(2-2)ModeCarouselProps(2-2)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx (3)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/types.ts (1)
SidebarMode(3-3)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/AnimatedBackground/AnimatedBackground.tsx (1)
AnimatedBackground(8-40)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/constants.ts (1)
modeIcons(4-7)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/constants.ts (1)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/types.ts (1)
SidebarMode(3-3)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeHeader/ModeHeader.tsx (2)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/types.ts (1)
SidebarMode(3-3)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/constants.ts (1)
modeLabels(9-12)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useModeDetection.ts (1)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/types.ts (1)
SidebarMode(3-3)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/AnimatedBackground/AnimatedBackground.tsx (1)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/AnimatedBackground/index.ts (1)
AnimatedBackground(1-1)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeContent/ModeContent.tsx (2)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/types.ts (1)
SidebarMode(3-3)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeHeader/ModeHeader.tsx (1)
ModeHeader(8-16)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/ModeCarousel.tsx (6)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useScrollProgress.ts (1)
useScrollProgress(10-58)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useScrollSnap.ts (1)
useScrollSnap(10-32)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useModeDetection.ts (1)
useModeDetection(12-70)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeHeader/ModeHeader.tsx (1)
ModeHeader(8-16)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeContent/ModeContent.tsx (1)
ModeContent(11-24)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx (1)
ModeNavigation(14-48)
🪛 GitHub Actions: CI
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/types.ts
[error] 1-1: TS2305: Module '"react"' has no exported member 'MotionValue'.
🔇 Additional comments (13)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeHeader/index.ts (1)
1-1: LGTM!Standard barrel export pattern for the ModeHeader component.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/index.ts (1)
1-1: LGTM!Standard barrel export pattern for the ModeNavigation component.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/AnimatedBackground/index.ts (1)
1-1: LGTM!Standard barrel export pattern for the AnimatedBackground component.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeHeader/ModeHeader.tsx (1)
1-16: LGTM!Clean, straightforward presentational component that renders the mode label with proper typing and styling.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/constants.ts (1)
1-12: LGTM!Well-typed constant mappings for mode icons and labels. The
Record<SidebarMode, typeof LayoutList>type ensures all modes have corresponding icons and maintains type safety across the lucide-react icon set.apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/index.ts (1)
1-2: LGTM!Clean barrel export structure that properly separates component and type exports.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/ModeCarousel.tsx (2)
31-52: LGTM! Clean hook composition.The hook-driven architecture cleanly separates concerns (progress tracking, snapping, mode detection) and makes the component logic easy to follow.
94-99: Remove unusedisActiveprop.The
isActiveprop is passed toModeContentbut the component definition (visible in the code snippets) doesn't accept or use this prop. This creates confusion and should be removed.Apply this diff:
<ModeContent mode={mode} - isActive={mode === currentMode} >Likely an incorrect or invalid review comment.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx (1)
26-26: Remove this review comment—the code is already type-safe.The TypeScript type
Record<SidebarMode, typeof LayoutList>enforces exhaustiveness at compile time. If a new mode is added toSidebarModewithout a corresponding entry inmodeIcons, TypeScript will fail to compile. The code at line 26 is runtime-safe by design and requires no changes.Likely an incorrect or invalid review comment.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useModeDetection.ts (1)
69-69: Callback is already stable; no issue exists.The
onModeSelectprop passed toModeCarouselissetCurrentModefromuseState, which is a stable reference across all re-renders. React's state setter functions maintain their identity and don't cause effect churn. The effect dependency array will not trigger unnecessary reattachments, so no performance concern exists.Likely an incorrect or invalid review comment.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useScrollProgress.ts (1)
53-55: The code is correct;onScrollProgressis already stable.The parent component (Sidebar.tsx) passes
setScrollProgress, which is a React state setter fromuseState(). React guarantees that state setters are stable references across renders and never change, so the effect dependency ononScrollProgressis appropriate and causes no performance issues.Likely an incorrect or invalid review comment.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeContent/ModeContent.tsx (2)
1-3: LGTM!The imports are correctly structured with proper type imports for better tree-shaking.
11-24: Clean component structure.The component implementation is well-structured with proper composition of ModeHeader and children. The use of inline styles for
scrollSnapAlignandscrollSnapStop(lines 16-17) is reasonable since Tailwind doesn't provide built-in utilities for these CSS properties.
| // Transform progress (0-1) to translateX position | ||
| // For 2 modes: 0 -> 0px, 1 -> 40px (buttonWidth + gap) |
There was a problem hiding this comment.
Clarify the progress input range in the comment.
The comment states "Transform progress (0-1)" but the actual input range is [0, modeCount - 1] (e.g., 0-1 for 2 modes, 0-2 for 3 modes), not a normalized 0-1 range.
Apply this diff:
- // Transform progress (0-1) to translateX position
- // For 2 modes: 0 -> 0px, 1 -> 40px (buttonWidth + gap)
+ // Transform progress (0 to modeCount-1) to translateX position
+ // For 2 modes: progress 0 -> 0px, progress 1 -> 40px (buttonWidth + gap)📝 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.
| // Transform progress (0-1) to translateX position | |
| // For 2 modes: 0 -> 0px, 1 -> 40px (buttonWidth + gap) | |
| // Transform progress (0 to modeCount-1) to translateX position | |
| // For 2 modes: progress 0 -> 0px, progress 1 -> 40px (buttonWidth + gap) |
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/AnimatedBackground/AnimatedBackground.tsx
around lines 17 to 18, the inline comment incorrectly says "Transform progress
(0-1)" even though progress actually ranges from 0 to modeCount - 1 (e.g., 0-1
for 2 modes, 0-2 for 3 modes); update the comment to clearly state the expected
input range is [0, modeCount - 1] and give the concrete mapping (e.g., 0 -> 0px,
modeCount - 1 -> (modeCount - 1) * (buttonWidth + gap)) so readers understand it
is not a normalized 0–1 value.
| interface ModeContentProps { | ||
| mode: SidebarMode; | ||
| isActive: boolean; | ||
| children: ReactNode; | ||
| } |
There was a problem hiding this comment.
Unused isActive prop in interface.
The isActive prop is declared in the interface but never destructured or used in the component implementation (line 11). This could indicate incomplete implementation or dead code.
Consider either:
- Removing the prop if it's not needed
- Using it for conditional styling or behavior (e.g., to apply active state classes)
If isActive should be used for conditional rendering or styling, I can help implement that logic. Would you like me to suggest an implementation?
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeContent/ModeContent.tsx
around lines 5 to 9, the interface declares an isActive prop that is never
consumed by the component; remove dead code by deleting isActive from
ModeContentProps (and any callers) OR use it by destructuring isActive in the
component signature and applying it to the rendered element for conditional
styling/behavior (e.g., add an "active" className, toggle styles, or set
aria-current/aria-pressed for accessibility) so the prop has an effect.
| @@ -0,0 +1,13 @@ | |||
| import type { MotionValue, ReactNode } from "react"; | |||
There was a problem hiding this comment.
Fix the incorrect import source for MotionValue.
The pipeline failure indicates that MotionValue is being imported from "react", but it doesn't exist there. MotionValue is a type from the framer-motion library.
Apply this diff to fix the import:
-import type { MotionValue, ReactNode } from "react";
+import type { ReactNode } from "react";
+import type { MotionValue } from "framer-motion";📝 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.
| import type { MotionValue, ReactNode } from "react"; | |
| import type { ReactNode } from "react"; | |
| import type { MotionValue } from "framer-motion"; |
🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: TS2305: Module '"react"' has no exported member 'MotionValue'.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/types.ts
around line 1, the type MotionValue is incorrectly imported from "react"; update
the import so MotionValue is imported from "framer-motion" and ReactNode remains
imported from "react" (i.e., replace the single import with proper imports:
MotionValue from "framer-motion" and ReactNode from "react").
1 participant
Summary by CodeRabbit
New Features
Style