refactor: remove auto-focus from newly created folders and consolidate navigation logic#195
refactor: remove auto-focus from newly created folders and consolidate navigation logic#195
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 02a5484 in 11 seconds. Click for details.
- Reviewed
141lines of code in4files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_8T8GGLRcoX1Iw82R
You can customize 
ellipsis-dev
bot
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a double-panel UI mode with store/state, dashboard and layout wiring, header/menu actions to open items in a left/right panel, modal rendering adjustments, navigation-only reactive navigation changes, and removal of automatic folder auto-focus/edit behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant WorkspaceHeader
participant UIStore as UI Store
participant DashboardLayout
participant ItemPanel as Item Panel Content
User->>WorkspaceHeader: choose "Open in Double Panel" / open item
WorkspaceHeader->>UIStore: openDoublePanel(itemId)
UIStore-->>WorkspaceHeader: set doublePanelActive, secondPanelItemId
UIStore->>DashboardLayout: state update (doublePanelActive, secondPanelItemId, maximizedItemId)
DashboardLayout->>ItemPanel: render left panel (secondPanelItemId) and right panel (maximizedItemId)
ItemPanel-->>User: display two resizable panels (isDoublePanelMode = true)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile OverviewGreptile SummaryThis PR successfully implements a double-panel mode for viewing two items side-by-side, refactors navigation logic to use reactive patterns, and removes auto-focus behavior from newly created folders. The changes span 18 files with 572 additions and 172 deletions. Key improvements:
Issues found:
The implementation is well-structured with proper state tracking for auto-selected items ( Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant WorkspaceHeader
participant UIStore
participant DashboardLayout
participant ItemPanelContent
User->>WorkspaceHeader: Click "Create Note"
WorkspaceHeader->>WorkspaceHeader: addItem("note")
WorkspaceHeader->>WorkspaceHeader: handleCreatedItems([itemId])
WorkspaceHeader->>UIStore: navigateToItem(itemId)
Note over UIStore: No auto-selection
User->>WorkspaceHeader: Open item (single click)
WorkspaceHeader->>UIStore: openPanel(itemId, 'replace')
UIStore->>UIStore: Set maximizedItemId
UIStore->>UIStore: Auto-select item (panelAutoSelectedCardIds)
UIStore-->>DashboardLayout: Render maximized item
User->>WorkspaceHeader: Toggle split view
WorkspaceHeader->>UIStore: toggleWorkspaceSplitView()
UIStore->>UIStore: Set workspaceSplitViewActive = true
UIStore-->>DashboardLayout: Render workspace + item panels
User->>SidebarCardList: Click "Open in Double Panel"
SidebarCardList->>UIStore: openDoublePanel(itemId)
UIStore->>UIStore: Set doublePanelActive = true
UIStore->>UIStore: Set secondPanelItemId
UIStore->>UIStore: Auto-select second item
UIStore-->>DashboardLayout: Render two items side-by-side
DashboardLayout->>ItemPanelContent: Render left panel (secondPanelItemId)
DashboardLayout->>ItemPanelContent: Render right panel (maximizedItemId)
User->>WorkspaceHeader: Close left panel
WorkspaceHeader->>UIStore: closeDoublePanelLeft()
UIStore->>UIStore: Clear secondPanelItemId
UIStore->>UIStore: Set doublePanelActive = false
UIStore->>UIStore: Deselect left item if auto-selected
UIStore-->>DashboardLayout: Revert to single panel
User->>User: Press Escape
User->>DashboardLayout: Escape key handler
DashboardLayout->>UIStore: exitDoublePanel()
UIStore->>UIStore: Clear all double-panel state
UIStore->>UIStore: Restore originalFolderId
UIStore->>UIStore: Deselect all auto-selected items
UIStore-->>DashboardLayout: Return to folder view
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| onCreateFolder: () => { if (addItem) addItem("folder"); }, | ||
| onCreateFolder: () => { | ||
| if (addItem) { | ||
| const itemId = addItem("folder"); |
There was a problem hiding this comment.
Type mismatch: addItem in this component has type string | void (line 37 of WorkspaceGrid.tsx), but WorkspaceHeader.tsx declares it as returning string. The null check if (itemId) guards against void, but the types should be consistent across components.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 8ee8b13 in 17 seconds. Click for details.
- Reviewed
35lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_3ArNRwxs367p6gtQ
You can customize
|
@greptile |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed d4ab59a in 13 seconds. Click for details.
- Reviewed
158lines of code in4files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_QaTVa3Inxqh9Jt3v
You can customize
| // Auto-select card when modal opens (read selection state imperatively to avoid infinite loops) | ||
| useEffect(() => { | ||
| // Only run when modal is open and we have an item | ||
| if (!isOpen || !item?.id) return; | ||
|
|
||
| // Check if card was already selected at the time of opening | ||
| const wasAlreadySelected = selectedCardIds.has(item.id); | ||
| const wasAlreadySelected = useUIStore.getState().selectedCardIds.has(item.id); | ||
|
|
||
| // If not already selected, select it now (adds it to context) | ||
| if (!wasAlreadySelected) { | ||
| toggleCardSelection(item.id); | ||
|
|
||
| // Only deselect on cleanup if we were the ones who selected it | ||
| return () => { | ||
| toggleCardSelection(item.id); | ||
| }; | ||
| } | ||
|
|
||
| // If it was already selected, don't change anything on cleanup | ||
| return undefined; | ||
| }, [isOpen, item?.id, selectedCardIds, toggleCardSelection]); | ||
| }, [isOpen, item?.id]); // eslint-disable-line react-hooks/exhaustive-deps | ||
|
|
There was a problem hiding this comment.
Effect cleanup may toggle wrong item
This effect uses toggleCardSelection but omits it from the dependency list (// eslint-disable-line react-hooks/exhaustive-deps). If the store/hook binding ever changes between open and cleanup (e.g., hot reload, provider changes), cleanup can call a stale toggleCardSelection reference.
Safer pattern here is to either include toggleCardSelection in deps or read the action from useUIStore.getState() inside the effect/cleanup so the same source is used throughout.
Additional Comments (1)
This needs a guard (or type alignment) so Also appears at: |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed da3926a in 27 seconds. Click for details.
- Reviewed
114lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_lvx74Ez4iKHHwfkY
You can customize
|
@greptile |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2f9bae5 in 16 seconds. Click for details.
- Reviewed
762lines of code in10files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_H6oKPAixd8BL1Qir
You can customize
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 11 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/components/workspace/SidebarCardList.tsx">
<violation number="1" location="src/components/workspace/SidebarCardList.tsx:193">
P2: The menu visibility uses `useUIStore.getState()` inside render, which won’t trigger re-renders when the UI store changes, so the "Open in Double Panel" option can get out of sync with the actual state. Use the zustand hook selector to subscribe to the relevant state instead.</violation>
</file>
<file name="src/components/workspace-canvas/WorkspaceCard.tsx">
<violation number="1" location="src/components/workspace-canvas/WorkspaceCard.tsx:1080">
P2: Using `useUIStore.getState()` inside the render means the menu visibility won’t update when split-view state changes, because `WorkspaceCard` is memoized on item props only. The “Open in Double Panel” option can become stale until an item prop changes. Subscribe to the store via the hook (or include these UI fields in the memoization logic) so UI state changes trigger re-render.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| )} | ||
| {/* Double Panel option - only for notes/PDFs when a panel is already open */} | ||
| {(item.type === 'note' || item.type === 'pdf') && (() => { | ||
| const uiState = useUIStore.getState(); |
There was a problem hiding this comment.
P2: The menu visibility uses useUIStore.getState() inside render, which won’t trigger re-renders when the UI store changes, so the "Open in Double Panel" option can get out of sync with the actual state. Use the zustand hook selector to subscribe to the relevant state instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/workspace/SidebarCardList.tsx, line 193:
<comment>The menu visibility uses `useUIStore.getState()` inside render, which won’t trigger re-renders when the UI store changes, so the "Open in Double Panel" option can get out of sync with the actual state. Use the zustand hook selector to subscribe to the relevant state instead.</comment>
<file context>
@@ -188,6 +188,22 @@ function SidebarItemButton({ item, allItems, workspaceName, workspaceIcon, works
)}
+ {/* Double Panel option - only for notes/PDFs when a panel is already open */}
+ {(item.type === 'note' || item.type === 'pdf') && (() => {
+ const uiState = useUIStore.getState();
+ return uiState.maximizedItemId && uiState.maximizedItemId !== item.id && !uiState.doublePanelActive;
+ })() && (
</file context>
| ) | ||
| } | ||
| {/* Double Panel option - only for notes/PDFs when split view is active */} | ||
| {(item.type === 'note' || item.type === 'pdf') && (() => { |
There was a problem hiding this comment.
P2: Using useUIStore.getState() inside the render means the menu visibility won’t update when split-view state changes, because WorkspaceCard is memoized on item props only. The “Open in Double Panel” option can become stale until an item prop changes. Subscribe to the store via the hook (or include these UI fields in the memoization logic) so UI state changes trigger re-render.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/workspace-canvas/WorkspaceCard.tsx, line 1080:
<comment>Using `useUIStore.getState()` inside the render means the menu visibility won’t update when split-view state changes, because `WorkspaceCard` is memoized on item props only. The “Open in Double Panel” option can become stale until an item prop changes. Subscribe to the store via the hook (or include these UI fields in the memoization logic) so UI state changes trigger re-render.</comment>
<file context>
@@ -1076,6 +1076,21 @@ function WorkspaceCard({
)
}
+ {/* Double Panel option - only for notes/PDFs when split view is active */}
+ {(item.type === 'note' || item.type === 'pdf') && (() => {
+ const state = useUIStore.getState();
+ return state.workspaceSplitViewActive && state.maximizedItemId && state.maximizedItemId !== item.id && !state.doublePanelActive;
</file context>
| onCreateNote: () => { | ||
| if (addItem) { | ||
| const itemId = addItem("note"); | ||
| if (handleCreatedItems && itemId) { | ||
| handleCreatedItems([itemId]); | ||
| } | ||
| handleCreatedItems([itemId]); | ||
| } |
There was a problem hiding this comment.
Passing possibly undefined ID
addItem is typed to return string, but several call sites pass its return value directly into handleCreatedItems([itemId]) (e.g. note/folder/flashcard). If addItem ever returns undefined (as noted elsewhere in this PR), useReactiveNavigation will set pendingNavigationId to undefined and then call navigateToItem(undefined), which is a runtime bug. Either make addItem consistently return a string everywhere, or guard before calling handleCreatedItems.
Also appears at src/components/workspace-canvas/WorkspaceSection.tsx:656-670.
| // Auto-select card when modal opens (read selection state imperatively to avoid infinite loops) | ||
| useEffect(() => { | ||
| if (!isOpen || !item?.id) return; | ||
|
|
||
| // Check if card was already selected at the time of opening | ||
| const wasAlreadySelected = selectedCardIds.has(item.id); | ||
| const wasAlreadySelected = useUIStore.getState().selectedCardIds.has(item.id); | ||
|
|
||
| // If not already selected, select it now | ||
| if (!wasAlreadySelected) { | ||
| toggleCardSelection(item.id); | ||
|
|
||
| // Only deselect on cleanup if we were the ones who selected it | ||
| return () => { | ||
| toggleCardSelection(item.id); | ||
| }; | ||
| } | ||
|
|
||
| return undefined; | ||
| }, [isOpen, item?.id]); // eslint-disable-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Cleanup uses stale action
The effect reads toggleCardSelection but omits it from the dependency list and suppresses the lint rule ([isOpen, item?.id]). If the store action reference changes between mount and cleanup (e.g. hot reload or provider changes), cleanup can toggle the wrong selection state. To keep open/cleanup consistent, include toggleCardSelection in deps or read the action via useUIStore.getState() inside the effect/cleanup.
Also appears in src/components/modals/PDFViewerModal.tsx:35-54.
| Move to | ||
| </DropdownMenuItem> | ||
| )} | ||
| {/* Double Panel option - only for notes/PDFs when a panel is already open */} | ||
| {(item.type === 'note' || item.type === 'pdf') && (() => { | ||
| const uiState = useUIStore.getState(); | ||
| return uiState.maximizedItemId && uiState.maximizedItemId !== item.id && !uiState.doublePanelActive; | ||
| })() && ( | ||
| <DropdownMenuItem | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| setIsDropdownOpen(false); | ||
| useUIStore.getState().openDoublePanel(item.id); | ||
| }} | ||
| > | ||
| <Columns className="mr-2 h-4 w-4" /> | ||
| Open in Double Panel | ||
| </DropdownMenuItem> | ||
| )} |
There was a problem hiding this comment.
Double panel opens from grid
The “Open in Double Panel” menu item here only checks maximizedItemId and !doublePanelActive. If a user has an item maximized in the modal (i.e. not in workspace split view), this action will still call openDoublePanel, which flips doublePanelActive on and makes DashboardLayout render two inline ItemPanelContents. That effectively forces double-panel mode from a modal-open state and bypasses the stated constraint “only when a panel is already open / split view is active”. This mismatch will produce confusing UI state transitions.
Additional Comments (2)
In the |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed f96bfd2 in 15 seconds. Click for details.
- Reviewed
58lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_U082AOt4il7yXiwO
You can customize
|
@greptile |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 70b7106 in 16 seconds. Click for details.
- Reviewed
76lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_ZL7vumYBKopxDMLq
You can customize
Additional Comments (1)
|
|
@greptile |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 404fced in 23 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_UpA2Q6IntDq5ZP4P
You can customize
Additional Comments (1)
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed b633239 in 22 seconds. Click for details.
- Reviewed
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_3O1rMw80JhaKzulQ
You can customize
|
@greptile |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 192ba5e in 19 seconds. Click for details.
- Reviewed
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_jRTsEd4Y5Cmafl2h
You can customize
|
@greptile |
|
|
||
| // Set pending navigation to trigger in useEffect once item is available in state | ||
| if (createdIds.length > 0) { | ||
| setPendingNavigationId(createdIds[0]); |
There was a problem hiding this comment.
accepts array but only uses first item
handleCreatedItems takes an array but setPendingNavigationId(createdIds[0]) only navigates to the first item. If multiple items are created simultaneously, only the first receives navigation focus.
| setPendingNavigationId(createdIds[0]); | |
| setPendingNavigationId(createdIds[createdIds.length - 1]); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| {/* Double Panel option - only for notes/PDFs when a panel is already open */} | ||
| {(item.type === 'note' || item.type === 'pdf') && (() => { | ||
| const uiState = useUIStore.getState(); | ||
| return uiState.maximizedItemId && uiState.maximizedItemId !== item.id && !uiState.doublePanelActive; | ||
| })() && ( |
There was a problem hiding this comment.
IIFE executes on every render
The condition (() => { const uiState = useUIStore.getState(); return uiState.maximizedItemId && ... })() runs on every component render, not just when the dropdown opens. Move the store read inside the onClick handler or use a proper hook selector.
| {/* Double Panel option - only for notes/PDFs when a panel is already open */} | |
| {(item.type === 'note' || item.type === 'pdf') && (() => { | |
| const uiState = useUIStore.getState(); | |
| return uiState.maximizedItemId && uiState.maximizedItemId !== item.id && !uiState.doublePanelActive; | |
| })() && ( | |
| {(item.type === 'note' || item.type === 'pdf') && ( | |
| <DropdownMenuItem | |
| onClick={(e) => { | |
| const uiState = useUIStore.getState(); | |
| if (!uiState.maximizedItemId || uiState.maximizedItemId === item.id || uiState.doublePanelActive) return; | |
| e.stopPropagation(); | |
| setIsDropdownOpen(false); | |
| uiState.openDoublePanel(item.id); | |
| }} | |
| > |
| const itemId = addItem("note"); | ||
| if (handleCreatedItems && itemId) { | ||
| handleCreatedItems([itemId]); | ||
| } | ||
| if (itemId) handleCreatedItems([itemId]); |
There was a problem hiding this comment.
properly guards against void return from addItem
The if (itemId) checks here correctly handle cases where addItem returns void, preventing handleCreatedItems([undefined]) from calling navigateToItem(undefined).
1shCha
force-pushed
the
sidebar_sv_fix
branch
from
46e5a8b to
c3b5236
Compare
|
@greptile |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
1 participant
Important
This PR introduces a double-panel mode for side-by-side item viewing, refactors navigation logic, and updates UI components to enhance user interaction and interface flexibility.
DashboardContentandDashboardLayout.FolderCardComponent.WorkspaceCard,WorkspaceContent, andWorkspaceGrid.WorkspaceHeaderto handle double-panel mode with panel titles and close buttons.SelectionActionBarfor responsive design.ui-store.tswith actions for opening, closing, and exiting double-panel mode.use-reactive-navigation.tsto handle navigation after item creation without auto-selection.This description was created by
for 192ba5e. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Style