Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR refactors MainScreen layout by introducing a TabView component that conditionally renders either a NewTabView for new tabs or the traditional Sidebar + CenterView. It adds tab tracking (isNew flag), sidebar mode switching (Tabs/Changes), and removes unused example stores and components. Changes
Sequence DiagramssequenceDiagram
actor User
participant MainScreen
participant TabView
participant Tab Store
participant Sidebar
participant ModeCarousel
User->>MainScreen: open app
MainScreen->>TabView: render
TabView->>Tab Store: read active tab
alt isNew flag set
TabView->>TabView: render NewTabView
User->>TabView: interact with new tab
TabView->>Tab Store: markTabAsUsed(id)
Tab Store->>Tab Store: set isNew: false
else existing tab
TabView->>TabView: render Sidebar + CenterView
Sidebar->>ModeCarousel: render with modes
User->>ModeCarousel: select mode (Tabs/Changes)
ModeCarousel->>Sidebar: onModeSelect callback
Sidebar->>Tab Store: setMode(mode)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
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: 3
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/TabView/NewTabView/NewTabView.tsx (1)
14-40: Consider using icon components instead of emoji for better cross-platform consistency.Emoji rendering can vary significantly across different operating systems and may not display consistently. For a desktop application, using an icon library (like lucide-react or similar) would provide more predictable visual presentation.
apps/desktop/src/renderer/stores/tabs.ts (1)
30-30: Consider explicitly settingisNew: falsefor the initial tab.The initial "Home" tab doesn't have the
isNewproperty set. Whileundefinedis treated as falsy in the current check (activeTab?.isNew), explicitly settingisNew: falsewould make the intent clearer and be more consistent with the tab state model.Apply this diff:
- tabs: [{ id: "tab-1", title: "Home" }], + tabs: [{ id: "tab-1", title: "Home", isNew: false }],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/renderer/screens/main/MainScreen.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/TabView/NewTabView/NewTabView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/NewTabView/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/index.tsx(1 hunks)apps/desktop/src/renderer/stores/tabs.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src/renderer/screens/main/components/TabView/NewTabView/NewTabView.tsx (1)
apps/desktop/src/renderer/screens/main/components/TabView/NewTabView/index.ts (1)
NewTabView(1-1)
apps/desktop/src/renderer/screens/main/components/TabView/index.tsx (4)
apps/desktop/src/renderer/stores/tabs.ts (1)
useTabsStore(27-82)apps/desktop/src/renderer/screens/main/components/TabView/NewTabView/NewTabView.tsx (1)
NewTabView(1-48)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/index.tsx (1)
Sidebar(4-65)apps/desktop/src/renderer/screens/main/components/TabView/CenterView/CenterView.tsx (1)
CenterView(7-26)
apps/desktop/src/renderer/screens/main/MainScreen.tsx (1)
apps/desktop/src/renderer/screens/main/components/TabView/index.tsx (1)
TabView(6-24)
🔇 Additional comments (2)
apps/desktop/src/renderer/screens/main/components/TabView/NewTabView/index.ts (1)
1-1: LGTM!Clean re-export following standard barrel export pattern.
apps/desktop/src/renderer/screens/main/MainScreen.tsx (1)
5-16: LGTM!Clean refactoring that properly delegates the layout responsibility to the new TabView component. The change maintains the existing structure while introducing the tab-based UI system.
| export function TabView() { | ||
| const { tabs, activeTabId } = useTabsStore(); | ||
| const activeTab = tabs.find((tab) => tab.id === activeTabId); | ||
|
|
||
| if (activeTab?.isNew) { | ||
| return ( | ||
| <div className="flex flex-1"> | ||
| <NewTabView /> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <div className="flex flex-1"> | ||
| <Sidebar /> | ||
| <CenterView /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Consider how tabs transition from "new" to "used" state.
The TabView correctly renders NewTabView for new tabs, but there's no mechanism to transition tabs out of the "new" state. The NewTabView buttons have no handlers, and markTabAsUsed isn't being called anywhere.
Users will be stuck in the NewTabView until they switch tabs or close the tab. Should clicking any of the NewTabView action buttons mark the tab as used?
Would you like me to propose an implementation that wires up the NewTabView buttons to call markTabAsUsed when clicked?
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/screens/main/components/TabView/index.tsx around
lines 6 to 24, TabView renders NewTabView for tabs with isNew but never
transitions them to "used"; wire the NewTabView action buttons to call
markTabAsUsed when clicked so the tab state updates. Pass a handler (or provide
the store method directly) into NewTabView that invokes
markTabAsUsed(activeTabId) after the user performs any action (or on first
meaningful interaction), ensure activeTabId is validated before calling, and
update any NewTabView props/handlers so clicks trigger the call and cause the UI
to re-render with the standard view.
| <button | ||
| type="button" | ||
| className="p-6 border border-border rounded-lg hover:bg-accent hover:border-accent-foreground transition-colors text-left" | ||
| > | ||
| <div className="text-2xl mb-2">📁</div> | ||
| <div className="font-medium text-foreground">Open Project</div> | ||
| <div className="text-sm text-muted-foreground"> | ||
| Browse your files | ||
| </div> | ||
| </button> | ||
| <button | ||
| type="button" | ||
| className="p-6 border border-border rounded-lg hover:bg-accent hover:border-accent-foreground transition-colors text-left" | ||
| > | ||
| <div className="text-2xl mb-2">⚡</div> | ||
| <div className="font-medium text-foreground">Quick Actions</div> | ||
| <div className="text-sm text-muted-foreground">Common tasks</div> | ||
| </button> | ||
| <button | ||
| type="button" | ||
| className="p-6 border border-border rounded-lg hover:bg-accent hover:border-accent-foreground transition-colors text-left" | ||
| > | ||
| <div className="text-2xl mb-2">🔍</div> | ||
| <div className="font-medium text-foreground">Search</div> | ||
| <div className="text-sm text-muted-foreground">Find anything</div> | ||
| </button> | ||
| <button | ||
| type="button" | ||
| className="p-6 border border-border rounded-lg hover:bg-accent hover:border-accent-foreground transition-colors text-left" | ||
| > | ||
| <div className="text-2xl mb-2">⚙️</div> | ||
| <div className="font-medium text-foreground">Settings</div> | ||
| <div className="text-sm text-muted-foreground">Configure app</div> | ||
| </button> |
There was a problem hiding this comment.
Action buttons lack functionality.
The four action buttons are currently non-functional placeholders with no onClick handlers. Users viewing the NewTabView won't be able to perform any actions or transition out of this state.
Would you like me to help implement handlers for these buttons, or is this intentionally left as a placeholder for future work?
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/TabView/NewTabView/NewTabView.tsx
around lines 10 to 43, the four action buttons are inert placeholders with no
onClick handlers; add onClick handlers that invoke appropriate actions: wire
each button to a prop callback (e.g., onOpenProject, onQuickActions, onSearch,
onSettings) or to local functions that dispatch navigation/actions, and update
the component signature to accept these callbacks if not present; ensure
handlers are passed down from the parent when used, and add basic accessibility
(aria-label) and preventDefault only if needed.
| markTabAsUsed: (id) => { | ||
| set((state) => ({ | ||
| tabs: state.tabs.map((tab) => | ||
| tab.id === id ? { ...tab, isNew: false } : tab, | ||
| ), | ||
| })); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
The markTabAsUsed method is defined but never invoked.
I don't see any calls to markTabAsUsed in the provided code. This means tabs will remain in the "new" state indefinitely with no way to transition to the regular view through user interaction. When and how should tabs be marked as used?
Run this script to check if markTabAsUsed is called elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Find all calls to markTabAsUsed method
# Search for markTabAsUsed invocations
rg -n 'markTabAsUsed\(' --type=ts --type=tsx -g '!**/node_modules/**'Length of output: 27
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Find all calls to markTabAsUsed method - corrected version
# Search for markTabAsUsed invocations in TypeScript files
rg -n 'markTabAsUsed' -g '!node_modules' | head -50
Length of output: 153
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Check how tabs store is imported and used
# Find imports of the tabs store
rg -n "from.*tabs" -g '!node_modules' | head -20
echo "---"
# Check for any property access patterns on tabs store (dynamic access)
rg -n "\.markTabAsUsed|markTabAsUsed\s*:" -g '!node_modules'
Length of output: 1275
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Check actual store method invocations in components
# Look at TabItem.tsx to see what it does with the store
cat apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/TabItem.tsx
echo "---"
# Search for invocations of store methods (pattern: storeName.method)
rg -n 'useTabsStore.*\(' -A 10 -g '!node_modules' | head -60
Length of output: 7615
---
🏁 Script executed:
```shell
#!/bin/bash
# Description: Check how new tabs are created and what isNew flag represents
# Find createNewTab function
rg -n 'createNewTab' -B 5 -A 10 -g '!node_modules'
Length of output: 2836
markTabAsUsed is never invoked and should be called to transition tabs from the "new" state.
Verification confirms markTabAsUsed is defined but unused throughout the codebase. New tabs are created with isNew: true and TabView checks this flag to render the new tab view, but no code path calls markTabAsUsed to transition tabs to the regular view. Either invoke markTabAsUsed when users interact with new tabs (e.g., when typing or clicking in the new tab content), or remove the unused method if this feature is not needed.
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs.ts around lines 72 to 78, the
markTabAsUsed method is defined but never invoked so tabs remain stuck with
isNew: true; either wire this method into the UI interaction points or remove
it. To fix, call the store action when a user interacts with a new tab (for
example in the TabView component add an onClick/onFocus or onInput handler that
checks if tab.isNew then calls markTabAsUsed(tab.id)), or if the new-tab UX is
not required, delete the unused markTabAsUsed method and remove the isNew flag
from new tab creation and TabView checks. Ensure the chosen approach updates the
tab state consistently and remove dead code paths if deleting.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
apps/desktop/src/renderer/stores/sidebar-state.ts (1)
21-21: Use enum member for consistency.The initialization uses a string literal
"tabs"instead of the enum memberSidebarMode.Tabs. While TypeScript accepts this, using the enum explicitly improves type safety and consistency with other usages (e.g., line 9 in Sidebar/index.tsx).Apply this diff:
- currentMode: "tabs", + currentMode: SidebarMode.Tabs,apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeContent/ModeContent.tsx (1)
5-11: Consider removing unusedisActiveprop.The
isActiveprop is defined inModeContentProps(line 7) but is not destructured or used in the component implementation (line 11). If this prop is planned for future use, consider adding a comment explaining its purpose; otherwise, remove it to keep the interface minimal.If not needed, apply this diff:
interface ModeContentProps { mode: SidebarMode; - isActive: boolean; children: ReactNode; } -export function ModeContent({ mode, children }: ModeContentProps) { +export function ModeContent({ mode, children }: ModeContentProps) {apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/constants.ts (1)
4-7: Type definition is overly restrictive.The type
Record<SidebarMode, typeof LayoutList>constrains all icon values to be exactly the type ofLayoutList. While this may work if all lucide-react icons share the same type, it's semantically incorrect and fragile. Consider using a more generic type.Apply this diff to use a more appropriate type:
-export const modeIcons: Record<SidebarMode, typeof LayoutList> = { +export const modeIcons: Record<SidebarMode, React.ComponentType<{ className?: string }>> = { tabs: LayoutList, changes: GitBranch, };Or, if lucide-react exports a common icon type:
+import type { LucideIcon } from "lucide-react"; + -export const modeIcons: Record<SidebarMode, typeof LayoutList> = { +export const modeIcons: Record<SidebarMode, LucideIcon> = { tabs: LayoutList, changes: GitBranch, };apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/index.tsx (1)
42-49: Use enum member instead of string literal.Line 43 uses the string literal
"changes"for comparison, which is inconsistent with the enum usage on line 9 (SidebarMode.Tabs,SidebarMode.Changes). Using the enum member improves type safety and consistency.Apply this diff:
- {(mode) => { - if (mode === "changes") { + {(mode) => { + if (mode === SidebarMode.Changes) { return ( <div className="flex-1 flex items-center justify-center text-sidebar-foreground/60 text-sm"> Changes view coming soon...apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx (1)
38-42: Consider simplifying the className logic for readability.The template literal spans multiple lines with nested conditionals. While functional, extracting the className logic could improve readability.
+ const buttonClassName = `relative z-10 h-8 w-8 rounded-sm flex items-center justify-center transition-colors duration-150 ${ + isActive + ? "text-sidebar-foreground" + : "text-sidebar-foreground/60 hover:text-sidebar-foreground/80" + }`; + return ( <button key={mode} type="button" onClick={() => onModeSelect(mode)} title={label} - className={`relative z-10 h-8 w-8 rounded-sm flex items-center justify-center transition-colors duration-150 ${ - isActive - ? "text-sidebar-foreground" - : "text-sidebar-foreground/60 hover:text-sidebar-foreground/80" - }`} + className={buttonClassName} >Alternatively, consider using a utility like
clsxorcnfor cleaner conditional class composition.apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/hooks/useScrollSnap.ts (1)
30-30: Unnecessary ref in dependency array.Including
isInitialMount(aReact.MutableRefObject) in the dependency array has no effect, since changes toref.currentdon't trigger re-renders. While harmless, it can be removed for clarity.- }, [currentIndex, scrollContainer, isInitialMount]); + }, [currentIndex, scrollContainer]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/desktop/src/renderer/components/ZustandExample.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/ModeCarousel.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/AnimatedBackground/AnimatedBackground.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/AnimatedBackground/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeContent/ModeContent.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeContent/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeHeader/ModeHeader.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeHeader/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeNavigation/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/constants.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/hooks/useModeDetection.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/hooks/useScrollProgress.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/hooks/useScrollSnap.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/types.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/index.tsx(1 hunks)apps/desktop/src/renderer/stores/example-store.ts(0 hunks)apps/desktop/src/renderer/stores/sidebar-state.ts(2 hunks)
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/stores/example-store.ts
- apps/desktop/src/renderer/components/ZustandExample.tsx
✅ Files skipped from review due to trivial changes (3)
- apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeContent/index.ts
- apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeHeader/index.ts
- apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/AnimatedBackground/index.ts
🧰 Additional context used
🧬 Code graph analysis (9)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeHeader/ModeHeader.tsx (2)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/types.ts (1)
SidebarMode(5-5)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/constants.ts (1)
modeLabels(9-12)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/constants.ts (2)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/index.ts (1)
SidebarMode(2-2)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/types.ts (1)
SidebarMode(5-5)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/hooks/useModeDetection.ts (2)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/index.ts (1)
SidebarMode(2-2)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/types.ts (1)
SidebarMode(5-5)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx (3)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/types.ts (1)
SidebarMode(5-5)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/AnimatedBackground/AnimatedBackground.tsx (1)
AnimatedBackground(8-39)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/constants.ts (2)
modeIcons(4-7)modeLabels(9-12)
apps/desktop/src/renderer/stores/sidebar-state.ts (2)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/index.ts (1)
SidebarMode(2-2)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/types.ts (1)
SidebarMode(5-5)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeContent/ModeContent.tsx (2)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/types.ts (1)
SidebarMode(5-5)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeHeader/ModeHeader.tsx (1)
ModeHeader(8-16)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/AnimatedBackground/AnimatedBackground.tsx (1)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/AnimatedBackground/index.ts (1)
AnimatedBackground(1-1)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/index.tsx (3)
apps/desktop/src/renderer/stores/sidebar-state.ts (1)
useSidebarStore(17-37)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/types.ts (1)
SidebarMode(5-5)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/ModeCarousel.tsx (1)
ModeCarousel(10-111)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/ModeCarousel.tsx (7)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/types.ts (1)
ModeCarouselProps(7-14)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/hooks/useScrollProgress.ts (1)
useScrollProgress(10-60)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/hooks/useScrollSnap.ts (1)
useScrollSnap(9-31)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/hooks/useModeDetection.ts (1)
useModeDetection(12-66)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeHeader/ModeHeader.tsx (1)
ModeHeader(8-16)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeContent/ModeContent.tsx (1)
ModeContent(11-24)apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx (1)
ModeNavigation(13-51)
🔇 Additional comments (10)
apps/desktop/src/renderer/stores/sidebar-state.ts (1)
4-7: LGTM!The SidebarMode enum is well-defined with clear string values.
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/types.ts (1)
1-14: LGTM!The type definitions are well-structured. Re-exporting
SidebarModeand definingModeCarouselPropscreates a clean public API for the ModeCarousel feature.apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeNavigation/index.ts (1)
1-1: LGTM!Standard barrel export pattern.
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/constants.ts (1)
9-12: LGTM!The
modeLabelsmapping is correctly typed and provides clear labels for each mode.apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/components/ModeHeader/ModeHeader.tsx (1)
1-16: LGTM!The ModeHeader component is simple, focused, and correctly implements the mode label rendering using the constants module.
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/index.ts (1)
1-2: LGTM!The barrel exports create a clean public API, properly separating component and type exports.
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/index.tsx (2)
12-25: LGTM!The animated sidebar with width transitions and pointer-events management is well-implemented. The motion.aside configuration correctly handles the collapsed state.
51-69: Placeholder navigation looks good for initial implementation.The static navigation items (Dashboard, Projects, Settings) appear to be placeholder content, which is appropriate for a UI infrastructure PR. The structure is clean and ready for future implementation.
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/hooks/useScrollProgress.ts (1)
10-60: LGTM! Well-implemented scroll progress tracking.The hook correctly:
- Uses
requestAnimationFramefor smooth, performance-friendly updates- Separates scroll tracking from progress exposure via two focused effects
- Handles cleanup properly (removes listeners and cancels pending RAF)
- Uses passive event listeners for better scrolling performance
The intentional omission of
currentIndexfrom the scroll effect dependencies (line 50) is correct—scroll position drives progress, whilecurrentIndexonly sets the initial value.apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/ModeCarousel.tsx (1)
10-111: LGTM! Well-architected carousel component.The component demonstrates solid patterns:
- Proper hook orchestration: Separates concerns across
useScrollProgress,useScrollSnap, anduseModeDetection- Callback ref pattern (lines 25-29): Correctly handles ref attachment for hook consumption
- Graceful degradation (lines 54-64): Simplifies to non-carousel layout for single mode
- Drag-aware interactions (lines 73, 80): Disables snap and pointer events during drag to prevent conflicts
- Correct width calculations (lines 85, 91): Properly sizes container and sections for horizontal scrolling
- Scroll configuration (lines 72-81): Appropriate native scroll behaviors including snap, smooth scrolling, and touch handling
| const buttonWidth = 32; | ||
| const gap = 4; | ||
| const totalButtonWidth = buttonWidth + gap; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract magic numbers to shared constants.
The hardcoded values 32 and 4 duplicate the Tailwind classes h-8 w-8 (32px) and gap-1 (4px) used in ModeNavigation. If the visual size changes, both locations must be updated, creating a maintenance risk.
Consider extracting these to a shared constants file:
// In constants.ts or a shared location
export const MODE_BUTTON_SIZE = 32; // h-8 w-8
export const MODE_BUTTON_GAP = 4; // gap-1Then import and use them here and in the Tailwind classes via arbitrary values:
- const buttonWidth = 32;
- const gap = 4;
+ const buttonWidth = MODE_BUTTON_SIZE;
+ const gap = MODE_BUTTON_GAP;| if (finalIndex >= 0 && finalIndex < modes.length && modes[finalIndex]) { | ||
| // Snap to the nearest mode | ||
| const targetScrollX = finalIndex * finalContainerWidth; | ||
| if (Math.abs(finalScrollLeft - targetScrollX) > 5) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract magic numbers to shared constants.
The 5px threshold (line 42) is duplicated in useScrollSnap.ts at line 21, and the 150ms debounce delay (line 54) is a magic number without context.
Extract these to constants for maintainability:
// In constants.ts or at the top of the file
const SNAP_THRESHOLD_PX = 5;
const SCROLL_SETTLE_DELAY_MS = 150;Then use them:
- if (Math.abs(finalScrollLeft - targetScrollX) > 5) {
+ if (Math.abs(finalScrollLeft - targetScrollX) > SNAP_THRESHOLD_PX) {
scrollContainer.scrollTo({
left: targetScrollX,
behavior: "smooth",
});
}- }, 150);
+ }, SCROLL_SETTLE_DELAY_MS);Also applies to: 54-54
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/hooks/useModeDetection.ts
around lines 42 and 54, the numeric literals `5` (px threshold) and `150` (ms
debounce) are magic numbers; extract them to named constants (e.g.,
SNAP_THRESHOLD_PX and SCROLL_SETTLE_DELAY_MS) either at the top of this file or
in a shared constants.ts and replace the inline literals with those constants,
and update any other file (like useScrollSnap.ts) to import and use the same
constants to avoid duplication.
| const targetScrollX = currentIndex * scrollContainer.offsetWidth; | ||
|
|
||
| // Only scroll if we're not already at the target position | ||
| if (Math.abs(scrollContainer.scrollLeft - targetScrollX) > 5) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use shared constant for snap threshold.
The 5px threshold is duplicated in useModeDetection.ts at line 42. Extract to a shared constant as noted in the useModeDetection.ts review.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/hooks/useScrollSnap.ts
around line 21, replace the hardcoded 5px snap threshold with the shared
constant used in useModeDetection.ts (line 42) by importing that constant from
the common/shared location; create the shared constant (e.g., SNAP_THRESHOLD_PX)
in the agreed utilities/constants module if it doesn't exist, update both
useScrollSnap.ts and useModeDetection.ts to import and use that constant, and
run a quick grep to ensure no other files use the literal 5 for the same purpose
before committing.
1 participant
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.