Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds Zustand stores (sidebar, tabs, example), animated Framer Motion Sidebar and TopBar tab UI (drag/reorder), a new Separator component in the UI package, replaces lucide-react icons with react-icons, removes the local Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as TopBar/Sidebar Components
participant Store as Zustand Stores
participant DND as react-dnd
U->>UI: Click toggle / drag tab / click add/close
UI->>Store: call action (toggleSidebar / addTab / removeTab / reorderTabs / setActiveTab)
Store-->>UI: updated state (isSidebarOpen / tabs list / activeTabId)
UI->>DND: drag start / hover events
DND->>UI: hover -> request reorder
UI->>Store: reorderTabs(...)
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 ignored due to path filters (1)
📒 Files selected for processing (10)
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
🧹 Nitpick comments (2)
apps/desktop/src/renderer/stores/README.md (1)
1-191: Good, comprehensive Zustand docs; consider typingsetin slice examplesThis README does a nice job documenting the intended Zustand patterns (selectors, devtools, persist, slices, and the division of concerns vs tRPC). It should be very helpful onboarding-wise.
One optional refinement: in the “Slice Pattern for Large Stores” section, the
createUserSlice/createSettingsSlicehelpers accept an untypedsetparameter. For a “TypeScript first” codebase, you might consider givingsetan explicit type (e.g., based on your app store state) so examples don’t rely on implicitany.Otherwise this looks solid.
apps/desktop/src/renderer/stores/example-store.ts (1)
1-54: Store shape and actions look consistent; a couple of optional type/devtools refinementsThe example store is well-structured:
ExampleStatecleanly separates data (count,text) from actions.initialStateis reused for both initial setup andreset, keeping defaults in one place.- Increment/decrement use the functional
setform, which is safe for concurrent updates.- Lightweight selectors
useCount/useTextare a good pattern for consumers.Two optional improvements you could consider:
- Type
initialStateexplicitly – e.g., asPick<ExampleState, "count" | "text">– to catch drift if more data fields are added later.- Devtools in production – if you care about minimizing prod overhead, you might conditionally enable
devtoolsonly in development via your existing env flags.Neither is blocking; the current implementation is sound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
apps/desktop/CLAUDE.md(1 hunks)apps/desktop/package.json(1 hunks)apps/desktop/src/renderer/components/ZustandExample.tsx(1 hunks)apps/desktop/src/renderer/stores/README.md(1 hunks)apps/desktop/src/renderer/stores/example-store.ts(1 hunks)apps/desktop/src/renderer/stores/index.ts(1 hunks)apps/desktop/src/shared/ipc-channels/index.ts(1 hunks)apps/desktop/src/shared/ipc-channels/ui.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/desktop/src/renderer/components/ZustandExample.tsx (1)
apps/desktop/src/renderer/stores/example-store.ts (3)
useCount(53-53)useText(54-54)useExampleStore(30-50)
🪛 GitHub Actions: CI
apps/desktop/package.json
[warning] Typecheck step encountered multiple TypeScript errors across the monorepo. Original log shows numerous TS errors in old-desktop build.
🪛 LanguageTool
apps/desktop/CLAUDE.md
[grammar] ~2-~2: Ensure spelling is correct
Context: ...ation details For Electron interprocess communnication, ALWAYS use trpc as defined in `src/lib...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~3-~3: Ensure spelling is correct
Context: ...as defined in src/lib/trpc Please use alias as defined in tsconfig.json when poss...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (5)
apps/desktop/package.json (1)
64-65: Zustand dependency addition looks good; ensure lockfile/typechecks are updatedThe new
zustanddependency fits the added store/component usage and JSON stays valid. Please just make sure the lockfile is updated andapps/desktopstill passes thetypecheckscript after this addition.apps/desktop/src/shared/ipc-channels/ui.ts (1)
5-5: Import reorder is a no-op and safeReordering the
{ MosaicNode, Tab }type import doesn’t change behavior or typings; this is safe.apps/desktop/src/shared/ipc-channels/index.ts (1)
16-32: IPC channels composition remains consistentAdding/reordering
WorktreeChannelsin the imports andIpcChannelsextension list keeps the aggregate interface consistent and type-safe; no behavioral change here.apps/desktop/src/renderer/stores/index.ts (1)
1-7: Barrel export is straightforward and matches documented import patternCentralizing store exports via
export * from "./example-store";aligns with the docs’import { ... } from "renderer/stores";examples and keeps imports tidy.apps/desktop/src/renderer/components/ZustandExample.tsx (1)
1-68: Example component cleanly demonstrates the intended Zustand patternsThe component nicely showcases:
- Selector hooks (
useCount,useText) for state to minimize re-renders.- Direct store selection for actions (
increment,decrement,setText,reset).- Simple, readable wiring between UI controls and store actions.
This is a good, self-contained demo component; no changes needed from my side.
| # Implementation details | ||
| For Electron interprocess communnication, ALWAYS use trpc as defined in `src/lib/trpc` | ||
| Please use alias as defined in `tsconfig.json` when possible | ||
| Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary. | ||
|
|
||
| Please follow the clean code rules: | ||
|
|
||
|
|
||
| # Code quality |
There was a problem hiding this comment.
Tighten wording and fix minor spelling in this section
The new guideline about preferring Zustand and minimizing effects is clear and matches the new store patterns. A couple of small polish tweaks you might consider in the same block:
- Line 2:
communnication→communication - Line 3:
alias→aliases(to match plural usage)
Content-wise this section looks good.
🧰 Tools
🪛 LanguageTool
[grammar] ~2-~2: Ensure spelling is correct
Context: ...ation details For Electron interprocess communnication, ALWAYS use trpc as defined in `src/lib...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~3-~3: Ensure spelling is correct
Context: ...as defined in src/lib/trpc Please use alias as defined in tsconfig.json when poss...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In apps/desktop/CLAUDE.md around lines 1 to 8, tighten wording and fix typos:
change "communnication" to "communication" on line 2 and change "alias" to
plural "aliases" on line 3, and while here, slightly rephrase the third sentence
for clarity (e.g., "Prefer Zustand for state management when appropriate; avoid
useEffect unless absolutely necessary.") so the guidance reads cleanly and
consistently.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/TopBar/TopBar.tsx (1)
2-2: Tabs integration into TopBar looks consistent; consider future overflow handlingThe new
no-dragmiddle section with<Tabs />is wired correctly and respects the drag/no-drag separation for the title bar. One thing to watch for later: withflex-1 overflow-hidden, a large number of tabs will simply be clipped rather than scrollable; if you expect many tabs, adding horizontal scrolling (e.g.overflow-x-auto) or a more explicit overflow strategy would improve usability.Also applies to: 18-20
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/package.json(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/TopBar.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/components/Tabs/Tabs.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/components/Tabs/index.ts(1 hunks)apps/desktop/src/renderer/stores/index.ts(1 hunks)apps/desktop/src/renderer/stores/tabs.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/stores/index.ts
- apps/desktop/package.json
🧰 Additional context used
🧬 Code graph analysis (2)
apps/desktop/src/renderer/screens/main/components/TopBar/components/Tabs/Tabs.tsx (2)
apps/desktop/src/renderer/stores/tabs.ts (1)
useTabsStore(24-71)apps/desktop/src/renderer/screens/main/components/TopBar/components/Tabs/index.ts (1)
Tabs(1-1)
apps/desktop/src/renderer/screens/main/components/TopBar/TopBar.tsx (1)
apps/desktop/src/renderer/screens/main/components/TopBar/components/Tabs/Tabs.tsx (1)
Tabs(70-93)
🔇 Additional comments (1)
apps/desktop/src/renderer/screens/main/components/TopBar/components/Tabs/index.ts (1)
1-1: Barrel export for Tabs is clean and appropriateSimple re-export keeps the Tabs public API tidy and matches how TopBar consumes it. No issues here.
| import { X } from "lucide-react"; | ||
| import { useDrag, useDrop } from "react-dnd"; | ||
| import { useTabsStore } from "renderer/stores/tabs"; | ||
|
|
||
| const TAB_TYPE = "TAB"; | ||
|
|
||
| interface TabItemProps { | ||
| id: string; | ||
| title: string; | ||
| isActive: boolean; | ||
| index: number; | ||
| } | ||
|
|
||
| function TabItem({ id, title, isActive, index }: TabItemProps) { | ||
| const { setActiveTab, removeTab, reorderTabs } = useTabsStore(); | ||
|
|
||
| const [{ isDragging }, drag] = useDrag({ | ||
| type: TAB_TYPE, | ||
| item: { id, index }, | ||
| collect: (monitor) => ({ | ||
| isDragging: monitor.isDragging(), | ||
| }), | ||
| }); | ||
|
|
||
| const [, drop] = useDrop({ | ||
| accept: TAB_TYPE, | ||
| hover: (item: { id: string; index: number }) => { | ||
| if (item.index !== index) { | ||
| reorderTabs(item.index, index); | ||
| item.index = index; | ||
| } | ||
| }, | ||
| }); | ||
|
|
||
| return ( | ||
| <button | ||
| type="button" | ||
| ref={(node) => { | ||
| drag(drop(node)); | ||
| }} | ||
| onClick={() => setActiveTab(id)} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| setActiveTab(id); | ||
| } | ||
| }} | ||
| tabIndex={0} | ||
| className={` | ||
| group relative flex items-center gap-2 px-4 h-full min-w-[120px] max-w-[240px] cursor-pointer | ||
| ${isActive ? "bg-background" : "bg-muted/50 hover:bg-muted"} | ||
| ${isDragging ? "opacity-50" : "opacity-100"} | ||
| border-r border-border | ||
| `} | ||
| > | ||
| <span className="flex-1 truncate text-sm text-foreground">{title}</span> | ||
| <button | ||
| type="button" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| removeTab(id); | ||
| }} | ||
| className="opacity-0 group-hover:opacity-100 hover:bg-muted-foreground/20 rounded p-0.5 transition-opacity" | ||
| > | ||
| <X className="w-3.5 h-3.5" /> | ||
| </button> | ||
| </button> | ||
| ); | ||
| } | ||
|
|
||
| export function Tabs() { | ||
| const { tabs, activeTabId, addTab } = useTabsStore(); | ||
|
|
||
| return ( | ||
| <div className="flex items-center h-full"> | ||
| {tabs.map((tab, index) => ( | ||
| <TabItem | ||
| key={tab.id} | ||
| id={tab.id} | ||
| title={tab.title} | ||
| isActive={tab.id === activeTabId} | ||
| index={index} | ||
| /> | ||
| ))} | ||
| <button | ||
| type="button" | ||
| onClick={addTab} | ||
| className="px-3 h-full hover:bg-muted transition-colors text-muted-foreground hover:text-foreground" | ||
| > | ||
| <span className="text-lg">+</span> | ||
| </button> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix nested <button> structure and add basic tab ARIA roles
TabItem currently renders a <button> that contains another <button> for the close icon. Nested buttons are invalid HTML and can cause unpredictable focus/keyboard behavior and accessibility issues.
You can keep the UX the same by making the outer element a focusable div with tab semantics, and leaving the inner close button as-is. While you’re here, adding minimal ARIA roles will improve accessibility for the tab strip.
Suggested update:
function TabItem({ id, title, isActive, index }: TabItemProps) {
const { setActiveTab, removeTab, reorderTabs } = useTabsStore();
@@
- return (
- <button
- type="button"
- ref={(node) => {
- drag(drop(node));
- }}
- onClick={() => setActiveTab(id)}
- onKeyDown={(e) => {
- if (e.key === "Enter" || e.key === " ") {
- setActiveTab(id);
- }
- }}
- tabIndex={0}
- className={`
+ return (
+ <div
+ ref={(node) => {
+ drag(drop(node));
+ }}
+ role="tab"
+ aria-selected={isActive}
+ onClick={() => setActiveTab(id)}
+ onKeyDown={(e) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ setActiveTab(id);
+ }
+ }}
+ tabIndex={0}
+ className={`
group relative flex items-center gap-2 px-4 h-full min-w-[120px] max-w-[240px] cursor-pointer
${isActive ? "bg-background" : "bg-muted/50 hover:bg-muted"}
${isDragging ? "opacity-50" : "opacity-100"}
border-r border-border
`}
>
<span className="flex-1 truncate text-sm text-foreground">{title}</span>
<button
type="button"
onClick={(e) => {
e.stopPropagation();
removeTab(id);
}}
className="opacity-0 group-hover:opacity-100 hover:bg-muted-foreground/20 rounded p-0.5 transition-opacity"
>
<X className="w-3.5 h-3.5" />
</button>
- </button>
+ </div>
);
}
@@
export function Tabs() {
const { tabs, activeTabId, addTab } = useTabsStore();
return (
- <div className="flex items-center h-full">
+ <div className="flex items-center h-full" role="tablist">
{tabs.map((tab, index) => (
<TabItem
key={tab.id}This removes the invalid nesting, keeps drag-and-drop / click behavior intact, and gives assistive tech a clearer model of the tab strip. You can extend this later with aria-controls/tab panels as needed.
📝 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 { X } from "lucide-react"; | |
| import { useDrag, useDrop } from "react-dnd"; | |
| import { useTabsStore } from "renderer/stores/tabs"; | |
| const TAB_TYPE = "TAB"; | |
| interface TabItemProps { | |
| id: string; | |
| title: string; | |
| isActive: boolean; | |
| index: number; | |
| } | |
| function TabItem({ id, title, isActive, index }: TabItemProps) { | |
| const { setActiveTab, removeTab, reorderTabs } = useTabsStore(); | |
| const [{ isDragging }, drag] = useDrag({ | |
| type: TAB_TYPE, | |
| item: { id, index }, | |
| collect: (monitor) => ({ | |
| isDragging: monitor.isDragging(), | |
| }), | |
| }); | |
| const [, drop] = useDrop({ | |
| accept: TAB_TYPE, | |
| hover: (item: { id: string; index: number }) => { | |
| if (item.index !== index) { | |
| reorderTabs(item.index, index); | |
| item.index = index; | |
| } | |
| }, | |
| }); | |
| return ( | |
| <button | |
| type="button" | |
| ref={(node) => { | |
| drag(drop(node)); | |
| }} | |
| onClick={() => setActiveTab(id)} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| setActiveTab(id); | |
| } | |
| }} | |
| tabIndex={0} | |
| className={` | |
| group relative flex items-center gap-2 px-4 h-full min-w-[120px] max-w-[240px] cursor-pointer | |
| ${isActive ? "bg-background" : "bg-muted/50 hover:bg-muted"} | |
| ${isDragging ? "opacity-50" : "opacity-100"} | |
| border-r border-border | |
| `} | |
| > | |
| <span className="flex-1 truncate text-sm text-foreground">{title}</span> | |
| <button | |
| type="button" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| removeTab(id); | |
| }} | |
| className="opacity-0 group-hover:opacity-100 hover:bg-muted-foreground/20 rounded p-0.5 transition-opacity" | |
| > | |
| <X className="w-3.5 h-3.5" /> | |
| </button> | |
| </button> | |
| ); | |
| } | |
| export function Tabs() { | |
| const { tabs, activeTabId, addTab } = useTabsStore(); | |
| return ( | |
| <div className="flex items-center h-full"> | |
| {tabs.map((tab, index) => ( | |
| <TabItem | |
| key={tab.id} | |
| id={tab.id} | |
| title={tab.title} | |
| isActive={tab.id === activeTabId} | |
| index={index} | |
| /> | |
| ))} | |
| <button | |
| type="button" | |
| onClick={addTab} | |
| className="px-3 h-full hover:bg-muted transition-colors text-muted-foreground hover:text-foreground" | |
| > | |
| <span className="text-lg">+</span> | |
| </button> | |
| </div> | |
| ); | |
| } | |
| import { X } from "lucide-react"; | |
| import { useDrag, useDrop } from "react-dnd"; | |
| import { useTabsStore } from "renderer/stores/tabs"; | |
| const TAB_TYPE = "TAB"; | |
| interface TabItemProps { | |
| id: string; | |
| title: string; | |
| isActive: boolean; | |
| index: number; | |
| } | |
| function TabItem({ id, title, isActive, index }: TabItemProps) { | |
| const { setActiveTab, removeTab, reorderTabs } = useTabsStore(); | |
| const [{ isDragging }, drag] = useDrag({ | |
| type: TAB_TYPE, | |
| item: { id, index }, | |
| collect: (monitor) => ({ | |
| isDragging: monitor.isDragging(), | |
| }), | |
| }); | |
| const [, drop] = useDrop({ | |
| accept: TAB_TYPE, | |
| hover: (item: { id: string; index: number }) => { | |
| if (item.index !== index) { | |
| reorderTabs(item.index, index); | |
| item.index = index; | |
| } | |
| }, | |
| }); | |
| return ( | |
| <div | |
| ref={(node) => { | |
| drag(drop(node)); | |
| }} | |
| role="tab" | |
| aria-selected={isActive} | |
| onClick={() => setActiveTab(id)} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| setActiveTab(id); | |
| } | |
| }} | |
| tabIndex={0} | |
| className={` | |
| group relative flex items-center gap-2 px-4 h-full min-w-[120px] max-w-[240px] cursor-pointer | |
| ${isActive ? "bg-background" : "bg-muted/50 hover:bg-muted"} | |
| ${isDragging ? "opacity-50" : "opacity-100"} | |
| border-r border-border | |
| `} | |
| > | |
| <span className="flex-1 truncate text-sm text-foreground">{title}</span> | |
| <button | |
| type="button" | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| removeTab(id); | |
| }} | |
| className="opacity-0 group-hover:opacity-100 hover:bg-muted-foreground/20 rounded p-0.5 transition-opacity" | |
| > | |
| <X className="w-3.5 h-3.5" /> | |
| </button> | |
| </div> | |
| ); | |
| } | |
| export function Tabs() { | |
| const { tabs, activeTabId, addTab } = useTabsStore(); | |
| return ( | |
| <div className="flex items-center h-full" role="tablist"> | |
| {tabs.map((tab, index) => ( | |
| <TabItem | |
| key={tab.id} | |
| id={tab.id} | |
| title={tab.title} | |
| isActive={tab.id === activeTabId} | |
| index={index} | |
| /> | |
| ))} | |
| <button | |
| type="button" | |
| onClick={addTab} | |
| className="px-3 h-full hover:bg-muted transition-colors text-muted-foreground hover:text-foreground" | |
| > | |
| <span className="text-lg">+</span> | |
| </button> | |
| </div> | |
| ); | |
| } |
| import { create } from "zustand"; | ||
| import { devtools } from "zustand/middleware"; | ||
|
|
||
| export interface Tab { | ||
| id: string; | ||
| title: string; | ||
| } | ||
|
|
||
| interface TabsState { | ||
| tabs: Tab[]; | ||
| activeTabId: string | null; | ||
|
|
||
| addTab: () => void; | ||
| removeTab: (id: string) => void; | ||
| setActiveTab: (id: string) => void; | ||
| reorderTabs: (startIndex: number, endIndex: number) => void; | ||
| } | ||
|
|
||
| const createNewTab = (): Tab => ({ | ||
| id: `tab-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`, | ||
| title: "New Tab", | ||
| }); | ||
|
|
||
| export const useTabsStore = create<TabsState>()( | ||
| devtools( | ||
| (set) => ({ | ||
| tabs: [{ id: "tab-1", title: "Home" }], | ||
| activeTabId: "tab-1", | ||
|
|
||
| addTab: () => { | ||
| const newTab = createNewTab(); | ||
| set((state) => ({ | ||
| tabs: [...state.tabs, newTab], | ||
| activeTabId: newTab.id, | ||
| })); | ||
| }, | ||
|
|
||
| removeTab: (id) => { | ||
| set((state) => { | ||
| const tabs = state.tabs.filter((tab) => tab.id !== id); | ||
| if (tabs.length === 0) { | ||
| const newTab = createNewTab(); | ||
| return { tabs: [newTab], activeTabId: newTab.id }; | ||
| } | ||
|
|
||
| if (id === state.activeTabId) { | ||
| const closedIndex = state.tabs.findIndex((tab) => tab.id === id); | ||
| const nextTab = tabs[closedIndex] || tabs[closedIndex - 1]; | ||
| return { tabs, activeTabId: nextTab.id }; | ||
| } | ||
|
|
||
| return { tabs }; | ||
| }); | ||
| }, | ||
|
|
||
| setActiveTab: (id) => { | ||
| set({ activeTabId: id }); | ||
| }, | ||
|
|
||
| reorderTabs: (startIndex, endIndex) => { | ||
| set((state) => { | ||
| const tabs = [...state.tabs]; | ||
| const [removed] = tabs.splice(startIndex, 1); | ||
| tabs.splice(endIndex, 0, removed); | ||
| return { tabs }; | ||
| }); | ||
| }, | ||
| }), | ||
| { name: "TabsStore" }, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Guard removeTab against non-existent ids; activeTabId nullability is unused
The core store logic looks solid, but there are a couple of edge cases worth tightening:
- Potential runtime error in
removeTabwhen id is not found
If removeTab is ever called with an id that isn’t present in state.tabs but happens to equal activeTabId, findIndex returns -1, and nextTab becomes undefined, leading to nextTab.id throwing.
A small guard makes this safer without changing behavior for valid ids:
- removeTab: (id) => {
- set((state) => {
- const tabs = state.tabs.filter((tab) => tab.id !== id);
+ removeTab: (id) => {
+ set((state) => {
+ const tabs = state.tabs.filter((tab) => tab.id !== id);
if (tabs.length === 0) {
const newTab = createNewTab();
return { tabs: [newTab], activeTabId: newTab.id };
}
- if (id === state.activeTabId) {
- const closedIndex = state.tabs.findIndex((tab) => tab.id === id);
+ const closedIndex = state.tabs.findIndex((tab) => tab.id === id);
+
+ if (id === state.activeTabId && closedIndex !== -1) {
const nextTab = tabs[closedIndex] || tabs[closedIndex - 1];
return { tabs, activeTabId: nextTab.id };
}
return { tabs };
});
},activeTabIdtype could be simplified
Given that you always ensure at least one tab exists and always set activeTabId to a valid tab id (never null), you could simplify the state by making activeTabId just string instead of string | null, or explicitly handle the null case if you plan to support “no active tab” in the future.
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs.ts around lines 1-71, add a guard in
removeTab to handle IDs not present in state.tabs and avoid accessing nextTab.id
when closedIndex is -1: check findIndex result and if id not found simply return
current state (or no-op), and when computing the replacement active tab ensure
you pick a valid tab (e.g., tabs[closedIndex] || tabs[closedIndex - 1] ||
tabs[0] || tabs[tabs.length-1]) before reading .id; optionally simplify the
activeTabId type from string | null to string since the store always maintains
at least one tab and always sets a valid activeTabId.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/Tabs.tsx (1)
42-46: Space key handling is redundant for button elements.The
onKeyDownhandler checks for the space key, but HTML button elements already triggeronClickwhen space is pressed by default. The space key check can be removed.Apply this diff to simplify the keyboard handler:
- onKeyDown={(e) => { - if (e.key === "Enter" || e.key === " ") { - setActiveTab(id); - } - }} + onKeyDown={(e) => { + if (e.key === "Enter") { + setActiveTab(id); + } + }}Alternatively, you could remove the
onKeyDownhandler entirely and rely solely ononClick, as buttons handle both Enter and Space keys natively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/Tabs.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/TopBar.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/Tabs.tsx (1)
apps/desktop/src/renderer/stores/tabs.ts (1)
useTabsStore(24-71)
apps/desktop/src/renderer/screens/main/components/TopBar/TopBar.tsx (2)
apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/Tabs.tsx (1)
Tabs(70-98)apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/index.ts (1)
Tabs(1-1)
🔇 Additional comments (6)
apps/desktop/src/renderer/screens/main/components/TopBar/TopBar.tsx (2)
2-2: LGTM!The import correctly references the Tabs component from the sibling Tabs directory.
18-20: LGTM!The middle section correctly integrates the Tabs component with appropriate styling (
flex-1,overflow-hidden) and theno-dragclass to allow user interaction within the draggable window area.apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/Tabs.tsx (4)
1-5: LGTM!Imports are correctly structured, bringing in the necessary dependencies for the drag-and-drop tab functionality and icon rendering.
56-66: LGTM!The close button correctly uses
stopPropagationto prevent triggering the parent tab's activation handler. The opacity transition on hover provides good visual feedback.
89-95: LGTM!The add tab button correctly invokes the store's
addTabaction and provides appropriate hover states and visual feedback.
17-33: The original review comment is incorrect—DndProvider is already properly configured.The DndProvider is already set up in
MainScreen.tsxand wraps the entire component tree (including TopBar and Tabs). The drag-and-drop hooks will work correctly without any changes needed.Likely an incorrect or invalid review comment.
| /> | ||
| ))} | ||
| </div> | ||
| <div className="pointer-events-none absolute right-0 top-0 h-full w-8 bg-linear-to-l from-background to-transparent" /> |
There was a problem hiding this comment.
Fix invalid Tailwind CSS class.
The class bg-linear-to-l is not a valid Tailwind CSS class. It should be bg-gradient-to-l for a left-to-right gradient background.
Apply this diff to fix the gradient class:
- <div className="pointer-events-none absolute right-0 top-0 h-full w-8 bg-linear-to-l from-background to-transparent" />
+ <div className="pointer-events-none absolute right-0 top-0 h-full w-8 bg-gradient-to-l from-background to-transparent" />📝 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.
| <div className="pointer-events-none absolute right-0 top-0 h-full w-8 bg-linear-to-l from-background to-transparent" /> | |
| <div className="pointer-events-none absolute right-0 top-0 h-full w-8 bg-gradient-to-l from-background to-transparent" /> |
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/Tabs.tsx around
line 87, the Tailwind class `bg-linear-to-l` is invalid; replace it with the
correct `bg-gradient-to-l` so the element uses a leftward gradient, preserving
the other classes (`pointer-events-none absolute right-0 top-0 h-full w-8
from-background to-transparent`) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/ui/src/components/separator.tsx (1)
1-1: Optimize React import for type-only usage.The pipeline correctly flags that React is only used for type annotations here. Using
import typeavoids unnecessary runtime imports.Apply this diff:
-import * as React from "react"; +import type * as React from "react";apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/AddTabButton.tsx (2)
13-13: Consider removing layout adjustment from component.The
mt-2class suggests the button is being positioned relative to its container. This is typically better handled by the parent layout (Tabs component) to keep positioning concerns separated.
15-15: Consider using an icon component for consistency.Using a text "+" is less consistent with the icon usage throughout the UI (e.g., PanelLeft, PanelRight in SidebarControl). Consider importing
Plusfrom lucide-react for visual consistency.Apply this diff:
import { Button } from "@superset/ui/button"; +import { Plus } from "lucide-react"; import { useTabsStore } from "renderer/stores/tabs"; export function AddTabButton() { const { addTab } = useTabsStore(); return ( <Button variant="ghost" size="icon" onClick={addTab} aria-label="Add new tab" className="mt-2" > - <span className="text-lg">+</span> + <Plus className="size-4" /> </Button> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
apps/desktop/src/renderer/lib/utils.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/AddTabButton.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/TabItem.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx(1 hunks)apps/desktop/src/renderer/stores/app-state.ts(1 hunks)apps/desktop/src/renderer/stores/index.ts(1 hunks)packages/ui/package.json(2 hunks)packages/ui/src/components/separator.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/lib/utils.ts
- apps/desktop/src/renderer/screens/main/components/TopBar/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/stores/index.ts
🧰 Additional context used
🧬 Code graph analysis (7)
packages/ui/src/components/separator.tsx (1)
packages/ui/src/lib/utils.ts (1)
cn(4-6)
apps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsx (1)
apps/desktop/src/renderer/stores/app-state.ts (1)
useAppStore(10-25)
apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx (2)
apps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsx (1)
SidebarControl(5-23)apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/index.tsx (1)
Tabs(11-117)
apps/desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx (1)
apps/desktop/src/renderer/stores/app-state.ts (1)
useAppStore(10-25)
apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/TabItem.tsx (2)
apps/desktop/src/renderer/stores/tabs.ts (1)
useTabsStore(24-71)packages/ui/src/lib/utils.ts (1)
cn(4-6)
apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/AddTabButton.tsx (2)
apps/desktop/src/renderer/stores/tabs.ts (1)
useTabsStore(24-71)packages/ui/src/components/button.tsx (1)
Button(61-61)
apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/index.tsx (4)
apps/desktop/src/renderer/stores/tabs.ts (1)
useTabsStore(24-71)apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/TabItem.tsx (1)
TabItem(19-98)packages/ui/src/components/separator.tsx (1)
Separator(25-25)apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/AddTabButton.tsx (1)
AddTabButton(4-18)
🪛 GitHub Actions: CI
packages/ui/src/components/separator.tsx
[warning] 1-1: lint/style/useImportType: Importing React types via normal import. Consider using 'import type' to avoid runtime impact.
🔇 Additional comments (9)
packages/ui/package.json (1)
22-22: LGTM!The separator export and dependency addition follow the established patterns in this package.
Also applies to: 39-39
packages/ui/src/components/separator.tsx (1)
5-23: LGTM!The Separator component is well-implemented with proper defaults, accessible attributes, and orientation-based styling.
apps/desktop/src/renderer/stores/app-state.ts (1)
10-25: LGTM!The Zustand store is correctly implemented with devtools middleware and provides both toggle and explicit setter methods for flexibility.
apps/desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx (1)
8-67: LGTM!The sidebar animation implementation is well-structured with smooth transitions. The staggered timing (0.15s for opacity, 0.2s for width) creates a polished fade-out effect, and the
pointerEventstoggle properly disables interaction when closed.apps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsx (1)
5-23: LGTM!The SidebarControl component is well-implemented with appropriate accessibility labels, semantic icon choices, and the
no-dragclass for proper Electron window behavior.apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx (1)
2-3: LGTM!The TopBar integration cleanly incorporates the new SidebarControl and Tabs components with proper flexbox layout and spacing.
Also applies to: 10-21
apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/index.tsx (2)
20-59: LGTM!The effect correctly handles scroll detection and tab width calculation with proper cleanup of event listeners.
68-102: LGTM!The tab rendering logic correctly manages separator visibility based on active and hover states, creating smooth visual transitions.
apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/TabItem.tsx (1)
19-47: Core tab behavior and drag‑reorder integration look solidState wiring to
useTabsStoreand thereact-dndpattern of updatingitem.indexafter callingreorderTabs(item.index, index)are consistent and should keep the store and UI in sync. Activation and close behavior are also clear and side‑effect free apart from the intended store updates.Also applies to: 80-87
| {showStartFade && ( | ||
| <div className="pointer-events-none absolute left-0 top-0 h-full w-8 bg-linear-to-r from-background to-transparent" /> | ||
| )} | ||
| {showEndFade && ( | ||
| <div className="pointer-events-none absolute right-0 top-0 h-full w-8 bg-linear-to-l from-background to-transparent" /> | ||
| )} |
There was a problem hiding this comment.
Fix invalid Tailwind gradient classes.
The fade effects use bg-linear-to-r and bg-linear-to-l, which are not valid Tailwind classes. The correct syntax is bg-gradient-to-r and bg-gradient-to-l.
Apply this diff:
{showStartFade && (
- <div className="pointer-events-none absolute left-0 top-0 h-full w-8 bg-linear-to-r from-background to-transparent" />
+ <div className="pointer-events-none absolute left-0 top-0 h-full w-8 bg-gradient-to-r from-background to-transparent" />
)}
{showEndFade && (
- <div className="pointer-events-none absolute right-0 top-0 h-full w-8 bg-linear-to-l from-background to-transparent" />
+ <div className="pointer-events-none absolute right-0 top-0 h-full w-8 bg-gradient-to-r from-background to-transparent" />
)}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/index.tsx
around lines 106 to 111 the gradient utility classes are mistyped as
bg-linear-to-r and bg-linear-to-l; replace them with the correct Tailwind
classes bg-gradient-to-r and bg-gradient-to-l so the fade effects render
properly, keeping the rest of the element structure and class names unchanged.
| {/* Active tab bottom border overlay */} | ||
| {isActive && <div className="absolute bottom-0 left-0 right-0 h-px" />} | ||
|
|
There was a problem hiding this comment.
Active tab bottom border overlay lacks a visible style
The active indicator div has only positioning/size classes and no color/background, so it’s likely invisible and not distinguishing the active tab.
You can make it visibly match the rest of the design, e.g.:
- {/* Active tab bottom border overlay */}
- {isActive && <div className="absolute bottom-0 left-0 right-0 h-px" />}
+ {/* Active tab bottom border overlay */}
+ {isActive && (
+ <div className="absolute bottom-0 left-0 right-0 h-px bg-border" />
+ )}(or use whatever bg-* / border color token matches the tab theme).
📝 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.
| {/* Active tab bottom border overlay */} | |
| {isActive && <div className="absolute bottom-0 left-0 right-0 h-px" />} | |
| {/* Active tab bottom border overlay */} | |
| {isActive && ( | |
| <div className="absolute bottom-0 left-0 right-0 h-px bg-border" /> | |
| )} |
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/TabItem.tsx
around lines 53 to 55, the active tab bottom border overlay div currently only
has positioning and height classes so it’s invisible; update that element to
include a visible color (for example a Tailwind bg- or border- utility that
matches the design token used for active state, e.g. bg-[token] or
border-b-[token]) so the 1px indicator is visible, and ensure the chosen token
meets contrast/accessibility requirements for the theme.
| <Button | ||
| type="button" | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| removeTab(id); | ||
| }} | ||
| className={cn( | ||
| "mt-1 absolute right-1 top-1/2 -translate-y-1/2 size-5", | ||
| isActive ? "opacity-100" : "opacity-0 group-hover:opacity-100", | ||
| )} | ||
| aria-label="Close tab" | ||
| > | ||
| <X className="size-3" /> | ||
| </Button> |
There was a problem hiding this comment.
Invisible close buttons remain focusable and clickable
When a tab is inactive, the close Button is opacity-0 but still in the tab order and can receive clicks/focus, which is confusing for keyboard users and may intercept pointer events over “empty” space.
Consider also disabling pointer and keyboard interaction when the button is visually hidden, for example:
- className={cn(
- "mt-1 absolute right-1 top-1/2 -translate-y-1/2 size-5",
- isActive ? "opacity-100" : "opacity-0 group-hover:opacity-100",
- )}
+ className={cn(
+ "mt-1 absolute right-1 top-1/2 -translate-y-1/2 size-5",
+ isActive
+ ? "opacity-100"
+ : "pointer-events-none opacity-0 group-hover:opacity-100 group-hover:pointer-events-auto",
+ )}
+ tabIndex={isActive ? 0 : -1}This keeps the close button accessible when appropriate while avoiding focus on invisible controls.
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/TabItem.tsx
around lines 80–95, the close Button is only visually hidden via opacity-0 but
remains focusable and clickable; update the Button so when the tab is inactive
(isActive false) it is removed from the tab order and pointer events are
disabled: add conditional props/classes such as pointer-events-none and
aria-hidden="true" (or disabled) and set tabIndex to -1 when hidden, and restore
pointer events, tabIndex (or remove disabled/aria-hidden) when isActive is true
so the control is accessible only when visible.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/Sidebar/index.tsx (1)
52-62: Consider adding content or documenting the empty bottom section.The bottom
motion.divhas animation logic but no content. If this is a placeholder for future implementation (e.g., version info, user profile), consider adding a TODO comment for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/renderer/screens/main/components/Sidebar/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/TabItem.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/index.tsx(1 hunks)apps/desktop/src/renderer/stores/index.ts(1 hunks)apps/desktop/src/renderer/stores/sidebar-state.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/screens/main/components/Sidebar/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/renderer/screens/main/components/TopBar/SidebarControl.tsx
- apps/desktop/src/renderer/stores/index.ts
- apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/index.tsx
- apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/TabItem.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/desktop/src/renderer/screens/main/components/Sidebar/index.tsx (1)
apps/desktop/src/renderer/stores/sidebar-state.ts (1)
useSidebarStore(10-25)
🔇 Additional comments (2)
apps/desktop/src/renderer/stores/sidebar-state.ts (1)
1-25: LGTM! Clean Zustand store implementation.The sidebar state store is well-structured and follows Zustand best practices. The use of devtools middleware, functional updates in
toggleSidebar, and proper TypeScript typing all look good.apps/desktop/src/renderer/screens/main/components/Sidebar/index.tsx (1)
22-50: LGTM! Animation and layout are well-structured.The opacity animation on the scrollable content section works well with the sidebar collapse animation. The navigation placeholder structure is appropriate for initial implementation.
| <motion.aside | ||
| initial={false} | ||
| animate={{ | ||
| width: isSidebarOpen ? 256 : 0, | ||
| }} | ||
| transition={{ | ||
| duration: 0.2, | ||
| ease: "easeInOut", | ||
| }} | ||
| className="h-full border-r border-sidebar-border bg-sidebar flex flex-col overflow-hidden" | ||
| style={{ | ||
| pointerEvents: isSidebarOpen ? "auto" : "none", | ||
| }} | ||
| > |
There was a problem hiding this comment.
Add accessibility attributes for the collapsed state.
When isSidebarOpen is false, the sidebar content remains keyboard-accessible despite pointerEvents: "none". Keyboard users may tab into invisible elements, and screen readers will still announce the hidden content.
Apply this diff to improve accessibility:
<motion.aside
initial={false}
animate={{
width: isSidebarOpen ? 256 : 0,
}}
transition={{
duration: 0.2,
ease: "easeInOut",
}}
className="h-full border-r border-sidebar-border bg-sidebar flex flex-col overflow-hidden"
style={{
pointerEvents: isSidebarOpen ? "auto" : "none",
}}
+ aria-hidden={!isSidebarOpen}
>📝 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.
| <motion.aside | |
| initial={false} | |
| animate={{ | |
| width: isSidebarOpen ? 256 : 0, | |
| }} | |
| transition={{ | |
| duration: 0.2, | |
| ease: "easeInOut", | |
| }} | |
| className="h-full border-r border-sidebar-border bg-sidebar flex flex-col overflow-hidden" | |
| style={{ | |
| pointerEvents: isSidebarOpen ? "auto" : "none", | |
| }} | |
| > | |
| <motion.aside | |
| initial={false} | |
| animate={{ | |
| width: isSidebarOpen ? 256 : 0, | |
| }} | |
| transition={{ | |
| duration: 0.2, | |
| ease: "easeInOut", | |
| }} | |
| className="h-full border-r border-sidebar-border bg-sidebar flex flex-col overflow-hidden" | |
| style={{ | |
| pointerEvents: isSidebarOpen ? "auto" : "none", | |
| }} | |
| aria-hidden={!isSidebarOpen} | |
| > |
1 participant
Summary by CodeRabbit
New Features
UI Components
Chores
✏️ Tip: You can customize this high-level summary in your review settings.