Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a multi-pane, resizable Projects workspace with split/close/focus pane operations, workspace state model and persistence, keyboard/tmux-like commands, UI wrappers for resizable panels, route/context updates to surface workspace via URL, plus tests and a dev dependency for resizable panels. Changes
Sequence DiagramsequenceDiagram
participant User
participant ProjectsWorkspace
participant WorkspaceProvider
participant WorkspaceState
participant ResizablePanels
participant ProjectBoardView
User->>ProjectsWorkspace: load (initialProjectId, routeWorkspaceState)
ProjectsWorkspace->>WorkspaceProvider: wrap UI (hydrate state)
WorkspaceProvider->>WorkspaceState: create/hydrate state
WorkspaceState-->>WorkspaceProvider: workspace tree
WorkspaceProvider-->>ProjectsWorkspace: context ready
ProjectsWorkspace->>ResizablePanels: render layout from workspace tree
ResizablePanels->>ProjectBoardView: render leaf pane(s)
User->>ProjectsWorkspace: trigger workspace command (split/close/focus)
ProjectsWorkspace->>WorkspaceProvider: dispatch command
WorkspaceProvider->>WorkspaceState: apply transform (split/close/focus)
WorkspaceState-->>WorkspaceProvider: updated tree
WorkspaceProvider->>localStorage: persist serialized state
WorkspaceProvider-->>ProjectsWorkspace: update -> re-render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Docstrings generation was requested by @matfire. * #11 (comment) The following files were modified: * `src/components/command-palette/command-palette.tsx` * `src/components/project-board/project-board-view.tsx` * `src/components/project-sidebar/project-sidebar.tsx` * `src/components/projects-workspace/projects-workspace.tsx` * `src/components/projects-workspace/workspace-commands.ts` * `src/components/projects-workspace/workspace-context.tsx` * `src/components/projects-workspace/workspace-state.ts` * `src/components/ui/resizable.tsx` * `src/components/ui/sidebar.tsx` * `src/contexts/projects-context.tsx` * `src/contexts/tasks-context.tsx` * `src/routes/projects.$projectId.tsx` * `src/routes/projects.tsx`
|
Note Docstrings generation - SUCCESS |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/components/project-board/project-board-view.tsx (1)
69-75: UsetaskByIdMap for O(1) lookup instead of.find().
taskById(line 57) already indexes tasks by ID. Using it here avoids a linear scan.♻️ Proposed fix
const selectedTask = useMemo(() => { if (!selectedTaskId) { return null; } - return sortedProjectTasks.find((task) => task.id === selectedTaskId) ?? null; - }, [selectedTaskId, sortedProjectTasks]); + return taskById.get(selectedTaskId) ?? null; + }, [selectedTaskId, taskById]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/project-board/project-board-view.tsx` around lines 69 - 75, selectedTask currently does an O(n) search over sortedProjectTasks using .find(); instead use the existing taskById index for O(1) lookup by replacing the find with a lookup into taskById (e.g., taskById.get(selectedTaskId) or taskById[selectedTaskId] depending on its type) and return null if no entry exists. Update the useMemo dependency array to [selectedTaskId, taskById] (remove sortedProjectTasks) so the memo invalidates correctly when the index or selection changes.src/contexts/tasks-context.tsx (1)
144-153: Consider callback reference stability forgetProjectTasks.Since
getProjectTasksdepends onprojectTasksByPath, its reference changes whenever any project's tasks update. Components using this callback in effects or as props may re-run/re-render unnecessarily.If this becomes a performance concern, consider exposing
projectTasksByPathdirectly and letting consumers derive tasks inline, or using a selector pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/tasks-context.tsx` around lines 144 - 153, getProjectTasks currently recreates whenever projectTasksByPath changes, causing unstable callback references; to fix, make the getter stable by either (A) exposing projectTasksByPath directly so consumers derive tasks themselves, or (B) keep a stable getProjectTasks callback by storing projectTasksByPath in a ref and updating that ref whenever projectTasksByPath changes (e.g., useEffect to sync ref), then have getProjectTasks (the useCallback) read from the ref instead of closing over projectTasksByPath; reference symbols: getProjectTasks and projectTasksByPath.src/components/projects-workspace/workspace-state.test.ts (1)
88-100: Test relies on hardcoded pane IDs from internal implementation.This test assumes
createWorkspaceStategenerates"pane-1"and subsequent splits produce"pane-2","pane-3"etc. While this works if ID generation is deterministic, it couples the test to internal implementation details. Consider extracting pane IDs dynamically from the state structure instead:♻️ Suggested improvement
test("cycles to the next pane in leaf order", () => { const initialState = createWorkspaceState("alpha"); const splitState = splitWorkspacePane(initialState, initialState.focusedPaneId, "horizontal"); const rightPaneId = splitState.focusedPaneId; const stackedState = splitWorkspacePane(splitState, rightPaneId, "vertical"); - const firstCycle = focusNextWorkspacePane(focusWorkspacePane(stackedState, "pane-1")); + // Extract first pane ID from state structure instead of hardcoding + const firstPaneId = stackedState.root.kind === "split" && stackedState.root.children[0].kind === "pane" + ? stackedState.root.children[0].id + : initialState.focusedPaneId; + const firstCycle = focusNextWorkspacePane(focusWorkspacePane(stackedState, firstPaneId)); const secondCycle = focusNextWorkspacePane(firstCycle); const thirdCycle = focusNextWorkspacePane(secondCycle); - expect(firstCycle.focusedPaneId).toBe("pane-2"); - expect(secondCycle.focusedPaneId).toBe("pane-3"); - expect(thirdCycle.focusedPaneId).toBe("pane-1"); + // Assert cycling behavior without hardcoding IDs + expect(firstCycle.focusedPaneId).not.toBe(firstPaneId); + expect(secondCycle.focusedPaneId).not.toBe(firstCycle.focusedPaneId); + expect(thirdCycle.focusedPaneId).toBe(firstPaneId); // Should wrap around });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/projects-workspace/workspace-state.test.ts` around lines 88 - 100, The test hardcodes pane IDs ("pane-1", "pane-2", "pane-3") which couples it to ID generation; instead, after calling createWorkspaceState and splitWorkspacePane, derive the pane IDs from the returned state (use the workspace tree/leaves or the pane list) and use those variables in focusWorkspacePane and the expectations. Update the test to call createWorkspaceState, perform the two splits with splitWorkspacePane, then collect the ordered leaf pane ids from the resulting state (referencing the state shape used by createWorkspaceState), use those ids when calling focusWorkspacePane and focusNextWorkspacePane, and assert against those dynamic id variables rather than hardcoded strings.src/routes/projects.tsx (1)
40-53: Effect may cause unnecessary navigation on every render while projectId is being cleared.The effect navigates to clear
projectIdwheneversearch.projectIdis truthy. However, sincenavigateis in the dependency array and navigation is asynchronous, there's a brief window where this effect runs before the URL updates. This should be fine, but consider adding a guard to prevent the effect from re-running if the navigation is already in progress.The current implementation works because
validateSearchreturnsundefinedafter the URL is updated, breaking theif (!search.projectId)condition. Just ensure the navigation completes synchronously enough that this doesn't cause a flash of re-rendering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/projects.tsx` around lines 40 - 53, The effect unconditionally calls navigate when search.projectId is truthy which can retrigger the effect before the URL update completes—add a guard ref to avoid duplicate navigations: create a ref like clearingProjectIdRef, check it at the top of the useEffect and return early if true, set it to true immediately before calling navigate in the block that clears projectId, and reset it when search.projectId becomes undefined (or in a follow-up effect) so subsequent clear operations are allowed; reference useEffect, navigate, search.projectId, and the new clearingProjectIdRef to locate where to add the guard.src/components/projects-workspace/projects-workspace.tsx (1)
149-170: Clarify aria-label terminology for split direction.The aria-labels use "vertically"/"horizontally" but the axis passed to
splitPaneis "horizontal"/"vertical". This is inverted from the visual outcome:
splitPane(node.id, "horizontal")creates a side-by-side layout (visually a vertical divider)splitPane(node.id, "vertical")creates a stacked layout (visually a horizontal divider)Consider aligning terminology with the visual result to reduce confusion for screen reader users.
♻️ Suggested fix
<Button - aria-label="Split pane vertically" + aria-label="Split pane side by side" onClick={() => { splitPane(node.id, "horizontal"); }} ... > <Button - aria-label="Split pane horizontally" + aria-label="Stack pane vertically" onClick={() => { splitPane(node.id, "vertical"); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/projects-workspace/projects-workspace.tsx` around lines 149 - 170, The aria-labels for the split buttons are inverted relative to the visual result produced by splitPane: splitPane(node.id, "horizontal") produces a side-by-side layout (vertical divider) and splitPane(node.id, "vertical") produces a stacked layout (horizontal divider). Update the aria-label strings on the two Button components (the ones that call splitPane and render Columns2/Rows2) so they describe the visual outcome (e.g., "Split pane side-by-side" or "Split pane vertically" for the Columns2 button that calls splitPane(..., "horizontal"), and "Split pane stacked" or "Split pane horizontally" for the Rows2 button that calls splitPane(..., "vertical")), keeping titles/tooltips consistent with the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/projects-workspace/projects-workspace.tsx`:
- Around line 43-50: The two TMUX constants (TMUX_SPLIT_SIDE_BY_SIDE_SEQUENCE,
TMUX_SPLIT_STACKED_SEQUENCE) are double-cast to HotkeySequence while the others
use a single cast, which hides a real type mismatch with the `@tanstack/hotkeys`
HotkeySequence type; check the library docs for the correct key-token format
(modifier vs combined "Control+B" vs separate tokens and how special chars are
represented), then update TMUX_SPLIT_SIDE_BY_SIDE_SEQUENCE and
TMUX_SPLIT_STACKED_SEQUENCE to use the same correct HotkeySequence shape as the
other TMUX_* constants (remove the unsafe "as unknown as HotkeySequence" and
either adjust the string tokens to the library-approved names or declare the
constant with an explicit HotkeySequence type), ensuring all TMUX_* constants
are consistent and type-safe.
---
Nitpick comments:
In `@src/components/project-board/project-board-view.tsx`:
- Around line 69-75: selectedTask currently does an O(n) search over
sortedProjectTasks using .find(); instead use the existing taskById index for
O(1) lookup by replacing the find with a lookup into taskById (e.g.,
taskById.get(selectedTaskId) or taskById[selectedTaskId] depending on its type)
and return null if no entry exists. Update the useMemo dependency array to
[selectedTaskId, taskById] (remove sortedProjectTasks) so the memo invalidates
correctly when the index or selection changes.
In `@src/components/projects-workspace/projects-workspace.tsx`:
- Around line 149-170: The aria-labels for the split buttons are inverted
relative to the visual result produced by splitPane: splitPane(node.id,
"horizontal") produces a side-by-side layout (vertical divider) and
splitPane(node.id, "vertical") produces a stacked layout (horizontal divider).
Update the aria-label strings on the two Button components (the ones that call
splitPane and render Columns2/Rows2) so they describe the visual outcome (e.g.,
"Split pane side-by-side" or "Split pane vertically" for the Columns2 button
that calls splitPane(..., "horizontal"), and "Split pane stacked" or "Split pane
horizontally" for the Rows2 button that calls splitPane(..., "vertical")),
keeping titles/tooltips consistent with the change.
In `@src/components/projects-workspace/workspace-state.test.ts`:
- Around line 88-100: The test hardcodes pane IDs ("pane-1", "pane-2", "pane-3")
which couples it to ID generation; instead, after calling createWorkspaceState
and splitWorkspacePane, derive the pane IDs from the returned state (use the
workspace tree/leaves or the pane list) and use those variables in
focusWorkspacePane and the expectations. Update the test to call
createWorkspaceState, perform the two splits with splitWorkspacePane, then
collect the ordered leaf pane ids from the resulting state (referencing the
state shape used by createWorkspaceState), use those ids when calling
focusWorkspacePane and focusNextWorkspacePane, and assert against those dynamic
id variables rather than hardcoded strings.
In `@src/contexts/tasks-context.tsx`:
- Around line 144-153: getProjectTasks currently recreates whenever
projectTasksByPath changes, causing unstable callback references; to fix, make
the getter stable by either (A) exposing projectTasksByPath directly so
consumers derive tasks themselves, or (B) keep a stable getProjectTasks callback
by storing projectTasksByPath in a ref and updating that ref whenever
projectTasksByPath changes (e.g., useEffect to sync ref), then have
getProjectTasks (the useCallback) read from the ref instead of closing over
projectTasksByPath; reference symbols: getProjectTasks and projectTasksByPath.
In `@src/routes/projects.tsx`:
- Around line 40-53: The effect unconditionally calls navigate when
search.projectId is truthy which can retrigger the effect before the URL update
completes—add a guard ref to avoid duplicate navigations: create a ref like
clearingProjectIdRef, check it at the top of the useEffect and return early if
true, set it to true immediately before calling navigate in the block that
clears projectId, and reset it when search.projectId becomes undefined (or in a
follow-up effect) so subsequent clear operations are allowed; reference
useEffect, navigate, search.projectId, and the new clearingProjectIdRef to
locate where to add the guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59cc614d-3b4e-4765-a161-632d534aa925
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
package.jsonsrc/bun-test.d.tssrc/components/command-palette/command-palette.tsxsrc/components/project-board/index.tssrc/components/project-board/project-board-view.tsxsrc/components/project-sidebar/project-sidebar.tsxsrc/components/projects-workspace/index.tssrc/components/projects-workspace/projects-workspace.tsxsrc/components/projects-workspace/workspace-commands.tssrc/components/projects-workspace/workspace-context.tsxsrc/components/projects-workspace/workspace-state.test.tssrc/components/projects-workspace/workspace-state.tssrc/components/ui/resizable.tsxsrc/components/ui/sidebar.tsxsrc/contexts/projects-context.tsxsrc/contexts/tasks-context.tsxsrc/routes/projects.$projectId.tsxsrc/routes/projects.tsx
Summary
Adds project workspace multiplexing so a single /projects window can be split into multiple independent panes. Each pane can show a different project, pane layouts persist per window, and the workspace now supports tmux-inspired pane shortcuts plus matching command-palette actions.
What Changed
Added a multiplexed project workspace with nested horizontal and vertical splits
Made each pane independent, with its own assigned project and board view
Persisted workspace layout, focused pane, and pane-to-project assignments per Tauri window
Kept separate project windows working, while allowing those windows to be split too
Refactored project board rendering into a reusable pane-level board view
Updated task data access so multiple project boards can render at the same time
Integrated workspace state with TanStack Router for /projects and /projects/$projectId
Added tmux-style pane shortcuts:
Ctrl+B, % for side-by-side split
Ctrl+B, " for stacked split
Ctrl+B, x to close the focused pane
Ctrl+B, o to cycle panes
Ctrl+B plus arrows to move focus
Added matching workspace actions to the command palette
Moved the sidebar toggle shortcut to Ctrl+T to avoid conflicting with the multiplexer prefix
Testing
bun test src/components/projects-workspace/workspace-state.test.ts
bunx tsc --noEmit
bun run build
bun run lint
Summary by CodeRabbit
New Features
Refactor
Tests
Chores