Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a dual-panel split-view: UI store gains a 'dual' openPanel mode and selectors; DashboardLayout exposes split-related props and delegates dual rendering to ModalManager; ModalManager renders two resizable inline panels when split-view is active; workspace canvas excludes items open in panels from the grid. Changes
Sequence DiagramsequenceDiagram
participant User as "User"
participant WorkspaceCard as "WorkspaceCard\n(UI)"
participant UIStore as "UI Store"
participant DashboardLayout as "DashboardLayout"
participant ModalManager as "ModalManager"
User->>WorkspaceCard: right-click → select "Double Panel"
WorkspaceCard->>UIStore: openPanel(itemId, "dual")
UIStore->>UIStore: update openPanelIds, set workspaceSplitViewActive
UIStore-->>DashboardLayout: state change (openPanelIds, workspaceSplitViewActive)
DashboardLayout->>ModalManager: render modal area (delegates in dual mode)
ModalManager->>ModalManager: detect split active && openPanelIds.length == 2
ModalManager->>ModalManager: render ResizablePanel(left) | ResizableHandle | ResizablePanel(right)
ModalManager-->>User: display two inline resizable modals
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
![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 0a876c7 in 18 seconds. Click for details.
- Reviewed
945lines of code in5files - 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_aUTAccSfegy9xFpK
You can customize 
Greptile OverviewGreptile SummaryThis PR introduces a dual-panel “double panel” viewing mode by extending the UI store panel APIs ( Main integration risks are around state invariants: Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Card as WorkspaceCard
participant UI as useUIStore
participant Layout as DashboardLayout
participant Modals as ModalManager
participant Grid as WorkspaceContent
Card->>UI: openPanel(itemId,'dual')
UI-->>Layout: openPanelIds=[new,left], workspaceSplitViewActive=true
Layout->>Modals: render modalManager (dual branch)
Modals->>Modals: find item1/item2 by openPanelIds
Modals-->>User: shows two inline panels
Grid->>UI: read workspaceSplitViewActive, openPanelIds
Grid-->>User: filters out items in openPanelIds
User->>Modals: close left/right panel
Modals->>UI: setMaximizedItemId(null)
Modals->>UI: closePanel(itemId)
UI-->>Layout: openPanelIds=[remaining], workspaceSplitViewActive=(current impl true)
Grid-->>User: may still filter remaining item if split flag stays true
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| closePanel: (itemId) => { | ||
| set((state) => { | ||
| if (state.openPanelIds.length === 0) return {}; | ||
|
|
||
| // Workspace Split View actions | ||
| // Workspace Split View actions | ||
| toggleWorkspaceSplitView: () => set((state) => { | ||
| // Only allow enabling split view if an item is maximized | ||
| if (!state.workspaceSplitViewActive && !state.maximizedItemId) { | ||
| return { workspaceSplitViewActive: false }; | ||
| } | ||
| return { workspaceSplitViewActive: !state.workspaceSplitViewActive }; | ||
| }), | ||
| enableWorkspaceSplitView: () => set((state) => { | ||
| if (!state.maximizedItemId) return { workspaceSplitViewActive: false }; | ||
| return { workspaceSplitViewActive: true }; | ||
| }), | ||
| disableWorkspaceSplitView: () => set({ workspaceSplitViewActive: false }), | ||
| // Dual-panel mode: closing one panel should keep the other | ||
| if (state.openPanelIds.length === 2) { | ||
| const remainingPanelId = state.openPanelIds.find(id => id !== itemId); | ||
| if (!remainingPanelId) { | ||
| // Shouldn't happen, but fallback to closing all | ||
| const newSelectedCardIds = new Set(state.selectedCardIds); | ||
| state.panelAutoSelectedCardIds.forEach(id => newSelectedCardIds.delete(id)); | ||
| return { | ||
| openPanelIds: [], | ||
| maximizedItemId: null, | ||
| workspaceSplitViewActive: false, | ||
| selectedCardIds: newSelectedCardIds, | ||
| panelAutoSelectedCardIds: new Set(), | ||
| }; | ||
| } | ||
|
|
||
| // Remove the closed panel from selections but keep the remaining one | ||
| const newSelectedCardIds = new Set(state.selectedCardIds); | ||
| const newPanelAutoSelectedCardIds = new Set(state.panelAutoSelectedCardIds); | ||
| if (newPanelAutoSelectedCardIds.has(itemId)) { | ||
| newSelectedCardIds.delete(itemId); | ||
| newPanelAutoSelectedCardIds.delete(itemId); | ||
| } | ||
|
|
||
| return { | ||
| openPanelIds: [remainingPanelId], | ||
| maximizedItemId: remainingPanelId, | ||
| workspaceSplitViewActive: true, // Keep split view active with single panel | ||
| selectedCardIds: newSelectedCardIds, | ||
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | ||
| }; |
There was a problem hiding this comment.
Split-view stuck on close
closePanel keeps workspaceSplitViewActive: true when going from 2 panels → 1 (ui-store.ts:310-316). That makes the app behave as “split view active” even though there’s only a single panel left, which forces split-view-only UI (e.g. grid filtering in WorkspaceContent.tsx:151-156) and can hide the remaining card from the grid unexpectedly. Set workspaceSplitViewActive to false (or otherwise re-derive it from openPanelIds.length) when only one panel remains.
| {/* Dual-Panel Mode: Render both panels when openPanelIds.length === 2 and split view is active */} | ||
| {workspaceSplitViewActive && openPanelIds.length === 2 ? ( | ||
| <> | ||
| {/* First Panel (Left) */} | ||
| <ResizablePanel id="dual-panel-left" defaultSize={50} minSize={30}> |
There was a problem hiding this comment.
Resizable panels missing Group
In dual-panel mode you render <ResizablePanel> / <ResizableHandle> directly (ModalManager.tsx:71-135), but these components wrap react-resizable-panels’ Panel/Separator which must be children of a Group (src/components/ui/resizable.tsx:14-21). As-is, dual-panel rendering will error at runtime. Wrap this block in <ResizablePanelGroup orientation="horizontal">…</ResizablePanelGroup> (or ensure the caller passes a Group and ModalManager only renders panels/handles within it).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/modals/ModalManager.tsx (1)
58-67:⚠️ Potential issue | 🟡 Minor
handleClosemay cause a transient state inconsistency in dual-panel mode.
setMaximizedItemId(null)(line 63) will setworkspaceSplitViewActive: false(via the store logic at ui-store.ts line 355), and thenclosePanel(itemId)(line 66) will set it back totrueif there was a second panel remaining. While React 18 batching ensures only one re-render with the final state, the intermediate store state between the twoset()calls is inconsistent.Consider reversing the order (close panel first, then clear maximized state), or combining both into a single store action to avoid the flip-flop.
Option A: Reverse the call order
const handleClose = (itemId: string) => { onFlushPendingChanges(itemId); - setMaximizedItemId(null); closePanel(itemId); + setMaximizedItemId(null); };Note:
closePanelalready setsmaximizedItemIdto the remaining panel ID in dual-panel mode, so the subsequentsetMaximizedItemId(null)would then override it to null — which may not be desired either. A dedicatedclosePanelAndCleanupstore action would be cleaner.Option B: Dedicated store action (preferred)
Add a combined action in the store that handles close + flush in one atomic state update, avoiding intermediate inconsistencies.
🤖 Fix all issues with AI agents
In `@src/lib/stores/ui-store.ts`:
- Line 199: Replace the concise arrow callbacks that implicitly return
Set.prototype.delete(...) with block-bodied callbacks so they return undefined;
specifically update occurrences like state.panelAutoSelectedCardIds.forEach(id
=> newSelectedCardIds.delete(id)) to use a block (id => {
newSelectedCardIds.delete(id); }) to avoid returning the boolean from
Set.delete; apply the same change for the other five occurrences that call
.forEach and .delete.
- Around line 332-345: The closeAllPanels updater currently clears panels and
selections but does not reset workspaceSplitViewActive like closePanel does;
update closeAllPanels (and likewise update closeAllModals and the setter that
handles setOpenModalItemId(null)) to set workspaceSplitViewActive: false when
openPanelIds (or openModal state) becomes empty or when you're closing the last
panel/modal; mirror the exact pattern used in closePanel (set
workspaceSplitViewActive: false when clearing the last panel) and ensure you
update the returned state objects in closeAllPanels, closeAllModals, and the
code path for setOpenModalItemId(null) to include workspaceSplitViewActive:
false.
🧹 Nitpick comments (6)
src/lib/stores/ui-store.ts (3)
347-350:reorderPanelsis a no-op — returns unchanged state.This action is declared in the interface (line 72) and subscribed by consumers, but the implementation does nothing. If reordering isn't needed yet, consider removing both the interface declaration and the implementation to avoid dead code, or add a TODO comment explaining the future intent.
374-375: Duplicate comment.Lines 374–375 both say
// Workspace Split View actions.Proposed fix
// Workspace Split View actions - // Workspace Split View actions toggleWorkspaceSplitView: () => set((state) => {
258-270: Consider removing the unused'add'mode from the type signature and clarify the'dual'mode behavior.The type signature at line 69 includes
'add'mode, but the implementation has no explicit handler for it—it falls through to replace mode. This mode is never used anywhere in the codebase. Remove it to avoid confusion and clarify intent.Additionally, when
mode === 'dual'but the preconditionopenPanelIds.length === 1isn't met (lines 258–270), execution silently falls through to replace mode. Consider explicitly handling this case with either a log message or early return to make the fallback behavior clearer to callers, or document the behavior prominently.src/components/modals/ModalManager.tsx (1)
71-135: Dual-panel rendering logic looks structurally sound.The
ResizablePanel+ResizableHandlestructure is correctly composed to be a child of theResizablePanelGroupinDashboardLayout.tsx(line 168). TherenderInline={true}flag ensures both panels render inline rather than as modals.One suggestion: the PDF-vs-CardDetailModal branching is duplicated for both panels. Consider extracting a small helper to reduce repetition.
Optional: Extract a panel renderer helper
function renderPanelContent( item: Item, onClose: () => void, onUpdateItem: (updates: Partial<Item>) => void, onUpdateItemData: (updater: (prev: ItemData) => ItemData) => void, onFlushPendingChanges: (itemId: string) => void, ) { return item.type === 'pdf' ? ( <PDFViewerModal key={item.id} item={item} isOpen={true} onClose={onClose} onUpdateItem={onUpdateItem} renderInline={true} /> ) : ( <CardDetailModal key={item.id} item={item} isOpen={true} onClose={onClose} onUpdateItem={onUpdateItem} onUpdateItemData={onUpdateItemData} onFlushPendingChanges={onFlushPendingChanges} renderInline={true} /> ); }src/components/layout/DashboardLayout.tsx (1)
73-75: Mixed data sources:workspaceSplitViewActivefrom props vs.openPanelIdsfrom store.
openPanelIdsis read directly from the store (line 74), butworkspaceSplitViewActivecomes as a prop (line 71, defaulting tofalse). Since you've already importeduseUIStore, consider readingworkspaceSplitViewActivefrom the store as well to eliminate the possibility of prop/store desync and simplify the caller's responsibilities.Proposed change
// Subscribe to openPanelIds for dual-panel mode detection const openPanelIds = useUIStore((state) => state.openPanelIds); - const isDualPanel = workspaceSplitViewActive && openPanelIds.length === 2; + const storeWorkspaceSplitViewActive = useUIStore((state) => state.workspaceSplitViewActive); + const isDualPanel = storeWorkspaceSplitViewActive && openPanelIds.length === 2;Then replace other usages of the
workspaceSplitViewActiveprop with the store value, and remove the prop from the interface if it becomes fully redundant.src/components/workspace-canvas/WorkspaceContent.tsx (1)
88-90: Remove unusedmaximizedItemIdsubscription on line 88.The subscription to
maximizedItemIdis not referenced elsewhere in this component. Since the filtering logic usesopenPanelIdsinstead, this subscription can be removed to eliminate an unnecessary re-render source.
| } | ||
| // Remove only auto-selected cards, preserve manual selections | ||
| const newSelectedCardIds = new Set(state.selectedCardIds); | ||
| state.panelAutoSelectedCardIds.forEach(id => newSelectedCardIds.delete(id)); |
There was a problem hiding this comment.
Static analysis: forEach callback should not return a value.
Set.prototype.delete() returns a boolean, and the arrow shorthand implicitly returns it. While harmless at runtime, this triggers Biome's useIterableCallbackReturn lint error. Use a block body to suppress.
Proposed fix (apply to all 6 occurrences)
-state.panelAutoSelectedCardIds.forEach(id => newSelectedCardIds.delete(id));
+state.panelAutoSelectedCardIds.forEach(id => { newSelectedCardIds.delete(id); });Also applies to: 215-215, 235-235, 292-292, 321-321, 337-337
🧰 Tools
🪛 Biome (2.3.13)
[error] 199-199: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@src/lib/stores/ui-store.ts` at line 199, Replace the concise arrow callbacks
that implicitly return Set.prototype.delete(...) with block-bodied callbacks so
they return undefined; specifically update occurrences like
state.panelAutoSelectedCardIds.forEach(id => newSelectedCardIds.delete(id)) to
use a block (id => { newSelectedCardIds.delete(id); }) to avoid returning the
boolean from Set.delete; apply the same change for the other five occurrences
that call .forEach and .delete.
| closeAllPanels: () => { | ||
| set((state) => { | ||
| if (state.openPanelIds.length === 0) return {}; | ||
| // Remove only auto-selected cards, preserve manual selections | ||
| const newSelectedCardIds = new Set(state.selectedCardIds); | ||
| const newPanelAutoSelectedCardIds = new Set(state.panelAutoSelectedCardIds); | ||
| state.panelAutoSelectedCardIds.forEach(id => newSelectedCardIds.delete(id)); | ||
| return { | ||
| openPanelIds: [], | ||
| maximizedItemId: null, | ||
| selectedCardIds: newSelectedCardIds, | ||
| panelAutoSelectedCardIds: new Set(), | ||
| }; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
closeAllPanels does not reset workspaceSplitViewActive, unlike closePanel.
closePanel (line 325) sets workspaceSplitViewActive: false when closing the last panel, but closeAllPanels leaves it untouched. Similarly, closeAllModals (line 549) and setOpenModalItemId(null) (line 392) don't clear it either. This can leave stale split-view state when panels are bulk-closed.
Proposed fix for `closeAllPanels`
closeAllPanels: () => {
set((state) => {
if (state.openPanelIds.length === 0) return {};
// Remove only auto-selected cards, preserve manual selections
const newSelectedCardIds = new Set(state.selectedCardIds);
state.panelAutoSelectedCardIds.forEach(id => newSelectedCardIds.delete(id));
return {
openPanelIds: [],
maximizedItemId: null,
+ workspaceSplitViewActive: false,
selectedCardIds: newSelectedCardIds,
panelAutoSelectedCardIds: new Set(),
};
});
},Apply the same pattern to closeAllModals and setOpenModalItemId(null).
📝 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.
| closeAllPanels: () => { | |
| set((state) => { | |
| if (state.openPanelIds.length === 0) return {}; | |
| // Remove only auto-selected cards, preserve manual selections | |
| const newSelectedCardIds = new Set(state.selectedCardIds); | |
| const newPanelAutoSelectedCardIds = new Set(state.panelAutoSelectedCardIds); | |
| state.panelAutoSelectedCardIds.forEach(id => newSelectedCardIds.delete(id)); | |
| return { | |
| openPanelIds: [], | |
| maximizedItemId: null, | |
| selectedCardIds: newSelectedCardIds, | |
| panelAutoSelectedCardIds: new Set(), | |
| }; | |
| }); | |
| }, | |
| closeAllPanels: () => { | |
| set((state) => { | |
| if (state.openPanelIds.length === 0) return {}; | |
| // Remove only auto-selected cards, preserve manual selections | |
| const newSelectedCardIds = new Set(state.selectedCardIds); | |
| state.panelAutoSelectedCardIds.forEach(id => newSelectedCardIds.delete(id)); | |
| return { | |
| openPanelIds: [], | |
| maximizedItemId: null, | |
| workspaceSplitViewActive: false, | |
| selectedCardIds: newSelectedCardIds, | |
| panelAutoSelectedCardIds: new Set(), | |
| }; | |
| }); | |
| }, |
🧰 Tools
🪛 Biome (2.3.13)
[error] 337-337: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@src/lib/stores/ui-store.ts` around lines 332 - 345, The closeAllPanels
updater currently clears panels and selections but does not reset
workspaceSplitViewActive like closePanel does; update closeAllPanels (and
likewise update closeAllModals and the setter that handles
setOpenModalItemId(null)) to set workspaceSplitViewActive: false when
openPanelIds (or openModal state) becomes empty or when you're closing the last
panel/modal; mirror the exact pattern used in closePanel (set
workspaceSplitViewActive: false when clearing the last panel) and ensure you
update the returned state objects in closeAllPanels, closeAllModals, and the
code path for setOpenModalItemId(null) to include workspaceSplitViewActive:
false.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 5 files
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/lib/stores/ui-store.ts">
<violation number="1" location="src/lib/stores/ui-store.ts:69">
P2: The `add` mode is included in the type signature but is completely ignored in the implementation. It falls through to the default 'replace' behavior, which is misleading for consumers of this API.</violation>
<violation number="2" location="src/lib/stores/ui-store.ts:248">
P2: The `openPanel` function fails to focus an item if it is already open but not currently maximized (e.g. in split view or background). Clicking an open item in the sidebar should bring it to focus.</violation>
<violation number="3" location="src/lib/stores/ui-store.ts:272">
P1: The 'replace' mode in `openPanel` (which is also the default fallback) fails to clean up the selection state (`selectedCardIds` and `panelAutoSelectedCardIds`) of the panels being closed. This leads to "zombie" selections where cards remain highlighted on the canvas even after their panels are closed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| // Actions - Panels | ||
| openPanel: (itemId: string, mode: 'replace' | 'add') => void; | ||
| openPanel: (itemId: string, mode: 'replace' | 'add' | 'dual') => void; |
There was a problem hiding this comment.
P2: The add mode is included in the type signature but is completely ignored in the implementation. It falls through to the default 'replace' behavior, which is misleading for consumers of this API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/stores/ui-store.ts, line 69:
<comment>The `add` mode is included in the type signature but is completely ignored in the implementation. It falls through to the default 'replace' behavior, which is misleading for consumers of this API.</comment>
<file context>
@@ -66,7 +66,7 @@ interface UIState {
// Actions - Panels
- openPanel: (itemId: string, mode: 'replace' | 'add') => void;
+ openPanel: (itemId: string, mode: 'replace' | 'add' | 'dual') => void;
closePanel: (itemId: string) => void;
closeAllPanels: () => void;
</file context>
| openPanel: (itemId, mode) => { | ||
| set((state) => { | ||
| const isAlreadyOpen = state.openPanelIds.includes(itemId); | ||
| if (isAlreadyOpen) return {}; |
There was a problem hiding this comment.
P2: The openPanel function fails to focus an item if it is already open but not currently maximized (e.g. in split view or background). Clicking an open item in the sidebar should bring it to focus.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/stores/ui-store.ts, line 248:
<comment>The `openPanel` function fails to focus an item if it is already open but not currently maximized (e.g. in split view or background). Clicking an open item in the sidebar should bring it to focus.</comment>
<file context>
@@ -185,346 +185,396 @@ const initialState = {
+ openPanel: (itemId, mode) => {
+ set((state) => {
+ const isAlreadyOpen = state.openPanelIds.includes(itemId);
+ if (isAlreadyOpen) return {};
- reorderPanels: (fromIndex, toIndex) => set((state) => {
</file context>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 79a412b in 13 seconds. Click for details.
- Reviewed
48lines 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_kSQiVcfftQmNrdm6
You can customize
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/components/layout/DashboardLayout.tsx`:
- Around line 73-75: DashboardLayout currently uses the workspaceSplitViewActive
prop while ModalManager reads it from the store, which can desync; make the
source of truth consistent by reading workspaceSplitViewActive from the store in
DashboardLayout (use the same useUIStore selector you use for openPanelIds) and
compute isDualPanel from that store value plus openPanelIds, or alternatively
pass the prop down to ModalManager — prefer the first approach: remove reliance
on the workspaceSplitViewActive prop inside DashboardLayout, import/use
useUIStore to select workspaceSplitViewActive and openPanelIds, and keep
isDualPanel = workspaceSplitViewActive && openPanelIds.length === 2 so both
DashboardLayout and ModalManager use the same store-derived value.
In `@src/components/modals/ModalManager.tsx`:
- Around line 71-135: The CardDetailModal is receiving onFlushPendingChanges
without the item id so the upstream handler (ModalManager's
onFlushPendingChanges) gets undefined; update the dual-panel left and right
panel props where CardDetailModal is rendered (references: item1, item2,
CardDetailModal, onFlushPendingChanges) to wrap the prop in an arrow that calls
the manager callback with the corresponding item id (e.g., pass a function that
invokes onFlushPendingChanges(item1.id) for the left panel and
onFlushPendingChanges(item2.id) for the right panel), mirroring how
onUpdateItem/onUpdateItemData are forwarded.
- Around line 76-134: Duplicate JSX for left/right panels (openPanelIds[0] and
openPanelIds[1]) should be extracted into a helper to remove the IIFE and
repeated logic; create a helper function renderPanelItem(indexOrId) (or
renderPanelItemByIndex) that looks up the item from items using
openPanelIds[index], returns null if missing, and returns the same conditional
JSX that currently renders PDFViewerModal or CardDetailModal while forwarding
key props (item, isOpen, onClose -> handleClose(item.id), onUpdateItem ->
onUpdateItem(item.id, ...), onUpdateItemData -> onUpdateItemData(item.id, ...),
onFlushPendingChanges, renderInline). Replace both inlined blocks with calls to
renderPanelItem(openPanelIds[0]) and renderPanelItem(openPanelIds[1]) inside the
respective ResizablePanel elements.
| // Subscribe to openPanelIds for dual-panel mode detection | ||
| const openPanelIds = useUIStore((state) => state.openPanelIds); | ||
| const isDualPanel = workspaceSplitViewActive && openPanelIds.length === 2; |
There was a problem hiding this comment.
workspaceSplitViewActive comes from props while openPanelIds is read from the store — risk of desync.
DashboardLayout receives workspaceSplitViewActive as a prop (line 71), but ModalManager subscribes to the same flag directly from the store (line 48 in ModalManager.tsx). If the parent passes a stale or different value, the two components will disagree on whether dual-panel mode is active (e.g., DashboardLayout renders the dual wrapper while ModalManager falls into the single-panel branch, or vice-versa).
Consider reading workspaceSplitViewActive from the store here as well (consistent with openPanelIds), or conversely pass it down to ModalManager as a prop — pick one source of truth.
🤖 Prompt for AI Agents
In `@src/components/layout/DashboardLayout.tsx` around lines 73 - 75,
DashboardLayout currently uses the workspaceSplitViewActive prop while
ModalManager reads it from the store, which can desync; make the source of truth
consistent by reading workspaceSplitViewActive from the store in DashboardLayout
(use the same useUIStore selector you use for openPanelIds) and compute
isDualPanel from that store value plus openPanelIds, or alternatively pass the
prop down to ModalManager — prefer the first approach: remove reliance on the
workspaceSplitViewActive prop inside DashboardLayout, import/use useUIStore to
select workspaceSplitViewActive and openPanelIds, and keep isDualPanel =
workspaceSplitViewActive && openPanelIds.length === 2 so both DashboardLayout
and ModalManager use the same store-derived value.
| {/* Dual-Panel Mode: Render both panels when openPanelIds.length === 2 and split view is active */} | ||
| {workspaceSplitViewActive && openPanelIds.length === 2 ? ( | ||
| <ResizablePanelGroup orientation="horizontal" className="h-full w-full"> | ||
| {/* First Panel (Left) */} | ||
| <ResizablePanel id="dual-panel-left" defaultSize={50} minSize={30}> | ||
| {openPanelIds[0] && (() => { | ||
| const item1 = items.find(i => i.id === openPanelIds[0]); | ||
| if (!item1) return null; | ||
|
|
||
| return item1.type === 'pdf' ? ( | ||
| <PDFViewerModal | ||
| key={item1.id} | ||
| item={item1} | ||
| isOpen={true} | ||
| onClose={() => handleClose(item1.id)} | ||
| onUpdateItem={(updates) => onUpdateItem(item1.id, updates)} | ||
| renderInline={true} | ||
| /> | ||
| ) : ( | ||
| <CardDetailModal | ||
| key={item1.id} | ||
| item={item1} | ||
| isOpen={true} | ||
| onClose={() => handleClose(item1.id)} | ||
| onUpdateItem={(updates) => onUpdateItem(item1.id, updates)} | ||
| onUpdateItemData={(updater) => onUpdateItemData(item1.id, updater)} | ||
| onFlushPendingChanges={onFlushPendingChanges} | ||
| renderInline={true} | ||
| /> | ||
| ); | ||
| })()} | ||
| </ResizablePanel> | ||
|
|
||
| <ResizableHandle className="border-r border-sidebar-border" /> | ||
|
|
||
| {/* Second Panel (Right) */} | ||
| <ResizablePanel id="dual-panel-right" defaultSize={50} minSize={30}> | ||
| {openPanelIds[1] && (() => { | ||
| const item2 = items.find(i => i.id === openPanelIds[1]); | ||
| if (!item2) return null; | ||
|
|
||
| return item2.type === 'pdf' ? ( | ||
| <PDFViewerModal | ||
| key={item2.id} | ||
| item={item2} | ||
| isOpen={true} | ||
| onClose={() => handleClose(item2.id)} | ||
| onUpdateItem={(updates) => onUpdateItem(item2.id, updates)} | ||
| renderInline={true} | ||
| /> | ||
| ) : ( | ||
| <CardDetailModal | ||
| key={item2.id} | ||
| item={item2} | ||
| isOpen={true} | ||
| onClose={() => handleClose(item2.id)} | ||
| onUpdateItem={(updates) => onUpdateItem(item2.id, updates)} | ||
| onUpdateItemData={(updater) => onUpdateItemData(item2.id, updater)} | ||
| onFlushPendingChanges={onFlushPendingChanges} | ||
| renderInline={true} | ||
| /> | ||
| ); | ||
| })()} | ||
| </ResizablePanel> | ||
| </ResizablePanelGroup> |
There was a problem hiding this comment.
onFlushPendingChanges is passed without binding the item ID — flush will receive undefined.
CardDetailModal declares onFlushPendingChanges?: () => void and calls it with zero arguments. But ModalManager's prop signature is (itemId: string) => void. On lines 97 and 129 you pass it straight through, so when the modal fires the callback, itemId will be undefined and the flush will silently fail or target the wrong item.
Wrap it the same way you wrap onUpdateItem / onUpdateItemData:
Proposed fix
<CardDetailModal
key={item1.id}
item={item1}
isOpen={true}
onClose={() => handleClose(item1.id)}
onUpdateItem={(updates) => onUpdateItem(item1.id, updates)}
onUpdateItemData={(updater) => onUpdateItemData(item1.id, updater)}
- onFlushPendingChanges={onFlushPendingChanges}
+ onFlushPendingChanges={() => onFlushPendingChanges(item1.id)}
renderInline={true}
/>Apply the same fix for the right panel (item2):
<CardDetailModal
key={item2.id}
item={item2}
isOpen={true}
onClose={() => handleClose(item2.id)}
onUpdateItem={(updates) => onUpdateItem(item2.id, updates)}
onUpdateItemData={(updater) => onUpdateItemData(item2.id, updater)}
- onFlushPendingChanges={onFlushPendingChanges}
+ onFlushPendingChanges={() => onFlushPendingChanges(item2.id)}
renderInline={true}
/>Note: the same issue exists at Line 156 in the single-panel path, but that's pre-existing code.
🤖 Prompt for AI Agents
In `@src/components/modals/ModalManager.tsx` around lines 71 - 135, The
CardDetailModal is receiving onFlushPendingChanges without the item id so the
upstream handler (ModalManager's onFlushPendingChanges) gets undefined; update
the dual-panel left and right panel props where CardDetailModal is rendered
(references: item1, item2, CardDetailModal, onFlushPendingChanges) to wrap the
prop in an arrow that calls the manager callback with the corresponding item id
(e.g., pass a function that invokes onFlushPendingChanges(item1.id) for the left
panel and onFlushPendingChanges(item2.id) for the right panel), mirroring how
onUpdateItem/onUpdateItemData are forwarded.
| {openPanelIds[0] && (() => { | ||
| const item1 = items.find(i => i.id === openPanelIds[0]); | ||
| if (!item1) return null; | ||
|
|
||
| return item1.type === 'pdf' ? ( | ||
| <PDFViewerModal | ||
| key={item1.id} | ||
| item={item1} | ||
| isOpen={true} | ||
| onClose={() => handleClose(item1.id)} | ||
| onUpdateItem={(updates) => onUpdateItem(item1.id, updates)} | ||
| renderInline={true} | ||
| /> | ||
| ) : ( | ||
| <CardDetailModal | ||
| key={item1.id} | ||
| item={item1} | ||
| isOpen={true} | ||
| onClose={() => handleClose(item1.id)} | ||
| onUpdateItem={(updates) => onUpdateItem(item1.id, updates)} | ||
| onUpdateItemData={(updater) => onUpdateItemData(item1.id, updater)} | ||
| onFlushPendingChanges={onFlushPendingChanges} | ||
| renderInline={true} | ||
| /> | ||
| ); | ||
| })()} | ||
| </ResizablePanel> | ||
|
|
||
| <ResizableHandle className="border-r border-sidebar-border" /> | ||
|
|
||
| {/* Second Panel (Right) */} | ||
| <ResizablePanel id="dual-panel-right" defaultSize={50} minSize={30}> | ||
| {openPanelIds[1] && (() => { | ||
| const item2 = items.find(i => i.id === openPanelIds[1]); | ||
| if (!item2) return null; | ||
|
|
||
| return item2.type === 'pdf' ? ( | ||
| <PDFViewerModal | ||
| key={item2.id} | ||
| item={item2} | ||
| isOpen={true} | ||
| onClose={() => handleClose(item2.id)} | ||
| onUpdateItem={(updates) => onUpdateItem(item2.id, updates)} | ||
| renderInline={true} | ||
| /> | ||
| ) : ( | ||
| <CardDetailModal | ||
| key={item2.id} | ||
| item={item2} | ||
| isOpen={true} | ||
| onClose={() => handleClose(item2.id)} | ||
| onUpdateItem={(updates) => onUpdateItem(item2.id, updates)} | ||
| onUpdateItemData={(updater) => onUpdateItemData(item2.id, updater)} | ||
| onFlushPendingChanges={onFlushPendingChanges} | ||
| renderInline={true} | ||
| /> | ||
| ); | ||
| })()} | ||
| </ResizablePanel> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract repeated panel-rendering logic to eliminate duplication.
The left-panel block (lines 76–101) and right-panel block (lines 108–133) are nearly identical — the only difference is the index into openPanelIds. An IIFE inside JSX is also uncommon and harder to scan. Consider extracting a small helper:
Suggested refactor
+ // Render a single panel item (reused for left & right in dual mode)
+ const renderPanelItem = (panelId: string | undefined) => {
+ if (!panelId) return null;
+ const item = items.find(i => i.id === panelId);
+ if (!item) return null;
+
+ return item.type === 'pdf' ? (
+ <PDFViewerModal
+ key={item.id}
+ item={item}
+ isOpen={true}
+ onClose={() => handleClose(item.id)}
+ onUpdateItem={(updates) => onUpdateItem(item.id, updates)}
+ renderInline={true}
+ />
+ ) : (
+ <CardDetailModal
+ key={item.id}
+ item={item}
+ isOpen={true}
+ onClose={() => handleClose(item.id)}
+ onUpdateItem={(updates) => onUpdateItem(item.id, updates)}
+ onUpdateItemData={(updater) => onUpdateItemData(item.id, updater)}
+ onFlushPendingChanges={() => onFlushPendingChanges(item.id)}
+ renderInline={true}
+ />
+ );
+ };Then the dual-panel JSX collapses to:
<ResizablePanelGroup orientation="horizontal" className="h-full w-full">
<ResizablePanel id="dual-panel-left" defaultSize={50} minSize={30}>
{renderPanelItem(openPanelIds[0])}
</ResizablePanel>
<ResizableHandle className="border-r border-sidebar-border" />
<ResizablePanel id="dual-panel-right" defaultSize={50} minSize={30}>
{renderPanelItem(openPanelIds[1])}
</ResizablePanel>
</ResizablePanelGroup>🤖 Prompt for AI Agents
In `@src/components/modals/ModalManager.tsx` around lines 76 - 134, Duplicate JSX
for left/right panels (openPanelIds[0] and openPanelIds[1]) should be extracted
into a helper to remove the IIFE and repeated logic; create a helper function
renderPanelItem(indexOrId) (or renderPanelItemByIndex) that looks up the item
from items using openPanelIds[index], returns null if missing, and returns the
same conditional JSX that currently renders PDFViewerModal or CardDetailModal
while forwarding key props (item, isOpen, onClose -> handleClose(item.id),
onUpdateItem -> onUpdateItem(item.id, ...), onUpdateItemData ->
onUpdateItemData(item.id, ...), onFlushPendingChanges, renderInline). Replace
both inlined blocks with calls to renderPanelItem(openPanelIds[0]) and
renderPanelItem(openPanelIds[1]) inside the respective ResizablePanel elements.
|
@greptile |
| // Legacy compatibility | ||
| setOpenModalItemId: (id) => { | ||
| set((state) => { | ||
| if (id === null) { | ||
| if (state.openPanelIds.length === 0) return {}; | ||
| return { | ||
| openPanelIds: [], | ||
| maximizedItemId: null, | ||
| panelAutoSelectedCardIds: new Set(), | ||
| }; | ||
| } else { |
There was a problem hiding this comment.
Selection not cleared on close
In setOpenModalItemId(null), the code clears openPanelIds/maximizedItemId but does not remove any auto-selected panel cards from selectedCardIds. If a card was auto-selected when opening a panel, closing via this legacy path leaves it stuck as selected until manually deselected. Align this null-close branch with the other close paths (e.g. closePanel / _closePanelDirect) by subtracting panelAutoSelectedCardIds from selectedCardIds before clearing.
| closeAllPanels: () => { | ||
| set((state) => { | ||
| if (state.openPanelIds.length === 0) return {}; | ||
| // Remove only auto-selected cards, preserve manual selections | ||
| const newSelectedCardIds = new Set(state.selectedCardIds); | ||
| const newPanelAutoSelectedCardIds = new Set(state.panelAutoSelectedCardIds); | ||
| state.panelAutoSelectedCardIds.forEach(id => newSelectedCardIds.delete(id)); | ||
| return { | ||
| openPanelIds: [], | ||
| maximizedItemId: null, | ||
| selectedCardIds: newSelectedCardIds, | ||
| panelAutoSelectedCardIds: new Set(), | ||
| }; |
There was a problem hiding this comment.
Split-view state not reset
closeAllPanels() clears openPanelIds/maximizedItemId but leaves workspaceSplitViewActive unchanged. If this action is used while split view is on (e.g. AppChatHeader calls closeAllPanels() when opening the sidebar), the UI can remain in “split view active” with zero panels open, which breaks assumptions in components that key off workspaceSplitViewActive. Reset workspaceSplitViewActive to false when closing all panels (or derive it from open panel count).
|
@greptile |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 055f8e7 in 14 seconds. Click for details.
- Reviewed
29lines 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_boK5872WKmGPQXnG
You can customize
|
| maximizedItemId: null, | ||
| workspaceSplitViewActive: false, | ||
| selectedCardIds: newSelectedCardIds, | ||
| panelAutoSelectedCardIds: new Set(), |
There was a problem hiding this comment.
Split-view flag inconsistent
closePanel keeps workspaceSplitViewActive: true when transitioning from 2 open panels to 1. This leaves the UI thinking split view is still active even though dual-panel is no longer possible, which will incorrectly hide the remaining item from the grid (WorkspaceContent filters all openPanelIds whenever workspaceSplitViewActive is true). Consider setting workspaceSplitViewActive based on panel count (e.g., false when openPanelIds.length < 2) or explicitly disabling it in this 2→1 close path.
|
|
||
| closePanel: (itemId) => { | ||
| set((state) => { | ||
| if (state.openPanelIds.length === 0) return {}; | ||
| // Direct panel open/close used by useFolderUrl hook for URL → store sync | ||
| _openPanelDirect: (itemId) => set({ openPanelIds: [itemId], maximizedItemId: itemId }), | ||
| _closePanelDirect: () => set((state) => { | ||
| // Remove only auto-selected cards, preserve manual selections | ||
| const newSelectedCardIds = new Set(state.selectedCardIds); | ||
| state.panelAutoSelectedCardIds.forEach(id => newSelectedCardIds.delete(id)); | ||
| return { |
There was a problem hiding this comment.
Direct close leaves split flag
_closePanelDirect() clears openPanelIds/maximizedItemId but does not reset workspaceSplitViewActive. If URL-sync closes panels while split view is active, components keying off workspaceSplitViewActive (e.g. grid filtering) can behave as if a split is still active with zero panels. Reset the flag here as well (or derive split-view purely from openPanelIds.length).
| newPanelAutoSelectedCardIds.add(itemId); | ||
|
|
||
| // Handle different modes | ||
| if (mode === 'dual') { | ||
| // Dual-panel mode: add as second panel (only if we have exactly 1 panel open) | ||
| if (state.openPanelIds.length === 1) { | ||
| return { | ||
| openPanelIds: [itemId, state.openPanelIds[0]], // New panel becomes left (index 0) | ||
| maximizedItemId: itemId, // Focus on the newly opened panel | ||
| workspaceSplitViewActive: true, // Enable split view for dual-panel | ||
| selectedCardIds: newSelectedCardIds, | ||
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | ||
| }; | ||
| } | ||
| // If not exactly 1 panel open, fall through to replace mode | ||
| } | ||
|
|
||
| // In workspace split view, deselect the previously maximized card when switching to a new one | ||
| if (state.workspaceSplitViewActive && id !== null && state.maximizedItemId !== id) { | ||
| const newSelectedCardIds = new Set(state.selectedCardIds); | ||
| if (state.maximizedItemId) { | ||
| newSelectedCardIds.delete(state.maximizedItemId); | ||
| } | ||
| return { | ||
| maximizedItemId: id, | ||
| selectedCardIds: newSelectedCardIds | ||
| }; | ||
| } | ||
| // Replace mode (default): replace all open panels with this one | ||
| return { |
There was a problem hiding this comment.
openPanel ignores 'add'
The openPanel API still advertises an 'add' mode, but the implementation now only handles 'dual' specially and otherwise always replaces openPanelIds with [itemId]. Any existing callers passing 'add' will silently get replace behavior. Either implement 'add' (and cap at 2) or remove the mode from the type to avoid incorrect behavior.
Additional Comments (3)
In single-panel mode the modal is gated by
In
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/lib/stores/ui-store.ts`:
- Around line 354-373: In setMaximizedItemId, avoid deselecting the previous
maximized card if that panel is still open: when handling the branch with
state.workspaceSplitViewActive && id !== null && state.maximizedItemId !== id,
only remove state.maximizedItemId from state.selectedCardIds after checking that
state.maximizedItemId is not present in state.openPanelIds; keep the Set
conversion for selectedCardIds and return the updated maximizedItemId and
selectedCardIds, otherwise leave selectedCardIds unchanged.
- Around line 407-430: The non-null branch of setOpenModalItemId returns a
single-panel state but doesn't clear the legacy split-view flag; update the
returned object in that branch (the block that sets openPanelIds: [id],
maximizedItemId: id, selectedCardIds, panelAutoSelectedCardIds) to also set
workspaceSplitViewActive: false so collapsing from dual to single panel
defensively resets the split-view state.
- Around line 245-280: The openPanel handler currently ignores the 'add' mode
and lets 'dual' fall through leaving workspaceSplitViewActive stale; update the
openPanel function to explicitly handle three modes: 'add' should append itemId
to openPanelIds (if not present) and update selectedCardIds and
panelAutoSelectedCardIds without replacing existing panels; 'dual' should only
perform the dual-pane swap when state.openPanelIds.length === 1 and otherwise
fall back to a replace that also clears workspaceSplitViewActive=false; the
default/replace branch must explicitly set workspaceSplitViewActive=false and
update openPanelIds, maximizedItemId, selectedCardIds and
panelAutoSelectedCardIds to avoid inconsistent state.
- Around line 230-242: The _openPanelDirect and _closePanelDirect handlers must
also update workspaceSplitViewActive and panelAutoSelectedCardIds to avoid
inconsistent state after URL-driven hydration: modify _openPanelDirect (the
function named _openPanelDirect) to set workspaceSplitViewActive to false (or
the desired default) and add the opened itemId into panelAutoSelectedCardIds
(and into selectedCardIds if you want it auto-selected), and modify
_closePanelDirect (the function named _closePanelDirect) to clear
workspaceSplitViewActive (set to false) and ensure panelAutoSelectedCardIds is
cleared only for auto-selected ids related to the open action; keep the existing
logic that removes panelAutoSelectedCardIds from selectedCardIds but ensure
panelAutoSelectedCardIds includes itemId when opening so close cleans up
correctly.
🧹 Nitpick comments (4)
src/lib/stores/ui-store.ts (4)
310-317:closePanelkeepsworkspaceSplitViewActive: trueafter reducing to a single panel.Line 313 intentionally keeps split view active when closing one of two panels. While
selectIsDualPanelActiveguards against this (requireslength === 2), leavingworkspaceSplitViewActive: truewith only one panel is semantically inconsistent and could surprise future consumers that readworkspaceSplitViewActivedirectly.If the intent is to allow quickly re-entering dual mode, consider a separate flag (e.g.,
splitViewPreferred) rather than leaving the "active" flag in a state that doesn't match reality.
348-351:reorderPanelsis a no-op — returns the unchanged state in all cases.The comment says "No reordering in single view", but the function does nothing in dual view either. If the intent is to swap two panels, the implementation is missing. If it's a placeholder, consider adding a
TODOor throwing to avoid silent misuse.Proposed implementation for dual-panel swap
reorderPanels: (fromIndex, toIndex) => set((state) => { - // No reordering in single view - return state; + if (state.openPanelIds.length < 2) return state; + const reordered = [...state.openPanelIds]; + const [moved] = reordered.splice(fromIndex, 1); + reordered.splice(toIndex, 0, moved); + return { openPanelIds: reordered }; }),
375-376: Duplicate comment.// Workspace Split View actions - // Workspace Split View actions
633-637:selectSelectedCardIdsArraycreates a new array reference on every call, defeating its stated purpose.The comment says this "prevents unnecessary re-renders when Set contents haven't changed," but
Array.from(...).sort()always returns a new reference, so===equality (Zustand's default) will always trigger a re-render. You need external memoization oruseShallowat the call site for this to work.
| // Direct panel open/close used by useFolderUrl hook for URL → store sync | ||
| _openPanelDirect: (itemId) => set({ openPanelIds: [itemId], maximizedItemId: itemId }), | ||
| _closePanelDirect: () => set((state) => { | ||
| // Remove only auto-selected cards, preserve manual selections | ||
| const newSelectedCardIds = new Set(state.selectedCardIds); | ||
| state.panelAutoSelectedCardIds.forEach(id => newSelectedCardIds.delete(id)); | ||
| return { | ||
| openPanelIds: [], | ||
| maximizedItemId: null, | ||
| workspaceSplitViewActive: false, // Disable split view when closing | ||
| selectedCardIds: newSelectedCardIds, | ||
| panelAutoSelectedCardIds: new Set(), | ||
| }; | ||
| }); | ||
| }, | ||
| }), |
There was a problem hiding this comment.
_openPanelDirect and _closePanelDirect don't manage workspaceSplitViewActive or selection tracking.
_openPanelDirect sets openPanelIds: [itemId] but doesn't reset workspaceSplitViewActive (could be true from a prior session). It also doesn't update panelAutoSelectedCardIds, so a subsequent closePanel won't properly clean up selections for that item.
If these are used for URL → store hydration on navigation, the store can start in an inconsistent state.
Proposed fix
- _openPanelDirect: (itemId) => set({ openPanelIds: [itemId], maximizedItemId: itemId }),
+ _openPanelDirect: (itemId) => set((state) => {
+ const newSelectedCardIds = new Set(state.selectedCardIds);
+ const newPanelAutoSelectedCardIds = new Set(state.panelAutoSelectedCardIds);
+ newSelectedCardIds.add(itemId);
+ newPanelAutoSelectedCardIds.add(itemId);
+ return {
+ openPanelIds: [itemId],
+ maximizedItemId: itemId,
+ workspaceSplitViewActive: false,
+ selectedCardIds: newSelectedCardIds,
+ panelAutoSelectedCardIds: newPanelAutoSelectedCardIds,
+ };
+ }),🧰 Tools
🪛 Biome (2.3.13)
[error] 235-235: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@src/lib/stores/ui-store.ts` around lines 230 - 242, The _openPanelDirect and
_closePanelDirect handlers must also update workspaceSplitViewActive and
panelAutoSelectedCardIds to avoid inconsistent state after URL-driven hydration:
modify _openPanelDirect (the function named _openPanelDirect) to set
workspaceSplitViewActive to false (or the desired default) and add the opened
itemId into panelAutoSelectedCardIds (and into selectedCardIds if you want it
auto-selected), and modify _closePanelDirect (the function named
_closePanelDirect) to clear workspaceSplitViewActive (set to false) and ensure
panelAutoSelectedCardIds is cleared only for auto-selected ids related to the
open action; keep the existing logic that removes panelAutoSelectedCardIds from
selectedCardIds but ensure panelAutoSelectedCardIds includes itemId when opening
so close cleans up correctly.
| openPanel: (itemId, mode) => { | ||
| set((state) => { | ||
| const isAlreadyOpen = state.openPanelIds.includes(itemId); | ||
| if (isAlreadyOpen) return {}; | ||
|
|
||
| reorderPanels: (fromIndex, toIndex) => set((state) => { | ||
| // No reordering in single view | ||
| return state; | ||
| }), | ||
| const newSelectedCardIds = new Set(state.selectedCardIds); | ||
| const newPanelAutoSelectedCardIds = new Set(state.panelAutoSelectedCardIds); | ||
|
|
||
| setItemPrompt: (prompt) => set({ itemPrompt: prompt }), | ||
| setMaximizedItemId: (id) => set((state) => { | ||
| // Disable workspace split view when item is un-maximized | ||
| if (id === null && state.workspaceSplitViewActive) { | ||
| return { maximizedItemId: id, workspaceSplitViewActive: false }; | ||
| } | ||
| // Add to selections and track as auto-selected | ||
| newSelectedCardIds.add(itemId); | ||
| newPanelAutoSelectedCardIds.add(itemId); | ||
|
|
||
| // Handle different modes | ||
| if (mode === 'dual') { | ||
| // Dual-panel mode: add as second panel (only if we have exactly 1 panel open) | ||
| if (state.openPanelIds.length === 1) { | ||
| return { | ||
| openPanelIds: [itemId, state.openPanelIds[0]], // New panel becomes left (index 0) | ||
| maximizedItemId: itemId, // Focus on the newly opened panel | ||
| workspaceSplitViewActive: true, // Enable split view for dual-panel | ||
| selectedCardIds: newSelectedCardIds, | ||
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | ||
| }; | ||
| } | ||
| // If not exactly 1 panel open, fall through to replace mode | ||
| } | ||
|
|
||
| // In workspace split view, deselect the previously maximized card when switching to a new one | ||
| if (state.workspaceSplitViewActive && id !== null && state.maximizedItemId !== id) { | ||
| const newSelectedCardIds = new Set(state.selectedCardIds); | ||
| if (state.maximizedItemId) { | ||
| newSelectedCardIds.delete(state.maximizedItemId); | ||
| } | ||
| return { | ||
| maximizedItemId: id, | ||
| selectedCardIds: newSelectedCardIds | ||
| }; | ||
| } | ||
| // Replace mode (default): replace all open panels with this one | ||
| return { | ||
| openPanelIds: [itemId], | ||
| maximizedItemId: itemId, | ||
| selectedCardIds: newSelectedCardIds, | ||
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | ||
| }; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
openPanel: 'add' mode is declared but silently falls through to 'replace'; 'dual' fallthrough can leave stale split-view state.
Two issues:
-
The
modeparameter accepts'add'(line 69), but there is no branch handling it — it silently falls through to the replace path. This is a dead API contract that will confuse callers expecting additive behavior. -
When
mode === 'dual'butopenPanelIds.length !== 1(e.g., already 2 panels open), the code falls through to the replace block (lines 272-278) which does not resetworkspaceSplitViewActive. If split view was previously active, the store ends up withopenPanelIds: [itemId](single panel) butworkspaceSplitViewActive: true— an inconsistent state.
Proposed fix
// Handle different modes
if (mode === 'dual') {
// Dual-panel mode: add as second panel (only if we have exactly 1 panel open)
if (state.openPanelIds.length === 1) {
return {
openPanelIds: [itemId, state.openPanelIds[0]], // New panel becomes left (index 0)
maximizedItemId: itemId, // Focus on the newly opened panel
workspaceSplitViewActive: true, // Enable split view for dual-panel
selectedCardIds: newSelectedCardIds,
panelAutoSelectedCardIds: newPanelAutoSelectedCardIds,
};
}
// If not exactly 1 panel open, fall through to replace mode
}
- // Replace mode (default): replace all open panels with this one
+ if (mode === 'add' && state.openPanelIds.length < 2) {
+ return {
+ openPanelIds: [...state.openPanelIds, itemId],
+ maximizedItemId: itemId,
+ selectedCardIds: newSelectedCardIds,
+ panelAutoSelectedCardIds: newPanelAutoSelectedCardIds,
+ };
+ }
+
+ // Replace mode (default / fallback): replace all open panels with this one
return {
openPanelIds: [itemId],
maximizedItemId: itemId,
+ workspaceSplitViewActive: false,
selectedCardIds: newSelectedCardIds,
panelAutoSelectedCardIds: newPanelAutoSelectedCardIds,
};📝 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.
| openPanel: (itemId, mode) => { | |
| set((state) => { | |
| const isAlreadyOpen = state.openPanelIds.includes(itemId); | |
| if (isAlreadyOpen) return {}; | |
| reorderPanels: (fromIndex, toIndex) => set((state) => { | |
| // No reordering in single view | |
| return state; | |
| }), | |
| const newSelectedCardIds = new Set(state.selectedCardIds); | |
| const newPanelAutoSelectedCardIds = new Set(state.panelAutoSelectedCardIds); | |
| setItemPrompt: (prompt) => set({ itemPrompt: prompt }), | |
| setMaximizedItemId: (id) => set((state) => { | |
| // Disable workspace split view when item is un-maximized | |
| if (id === null && state.workspaceSplitViewActive) { | |
| return { maximizedItemId: id, workspaceSplitViewActive: false }; | |
| } | |
| // Add to selections and track as auto-selected | |
| newSelectedCardIds.add(itemId); | |
| newPanelAutoSelectedCardIds.add(itemId); | |
| // Handle different modes | |
| if (mode === 'dual') { | |
| // Dual-panel mode: add as second panel (only if we have exactly 1 panel open) | |
| if (state.openPanelIds.length === 1) { | |
| return { | |
| openPanelIds: [itemId, state.openPanelIds[0]], // New panel becomes left (index 0) | |
| maximizedItemId: itemId, // Focus on the newly opened panel | |
| workspaceSplitViewActive: true, // Enable split view for dual-panel | |
| selectedCardIds: newSelectedCardIds, | |
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | |
| }; | |
| } | |
| // If not exactly 1 panel open, fall through to replace mode | |
| } | |
| // In workspace split view, deselect the previously maximized card when switching to a new one | |
| if (state.workspaceSplitViewActive && id !== null && state.maximizedItemId !== id) { | |
| const newSelectedCardIds = new Set(state.selectedCardIds); | |
| if (state.maximizedItemId) { | |
| newSelectedCardIds.delete(state.maximizedItemId); | |
| } | |
| return { | |
| maximizedItemId: id, | |
| selectedCardIds: newSelectedCardIds | |
| }; | |
| } | |
| // Replace mode (default): replace all open panels with this one | |
| return { | |
| openPanelIds: [itemId], | |
| maximizedItemId: itemId, | |
| selectedCardIds: newSelectedCardIds, | |
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | |
| }; | |
| }); | |
| }, | |
| openPanel: (itemId, mode) => { | |
| set((state) => { | |
| const isAlreadyOpen = state.openPanelIds.includes(itemId); | |
| if (isAlreadyOpen) return {}; | |
| const newSelectedCardIds = new Set(state.selectedCardIds); | |
| const newPanelAutoSelectedCardIds = new Set(state.panelAutoSelectedCardIds); | |
| // Add to selections and track as auto-selected | |
| newSelectedCardIds.add(itemId); | |
| newPanelAutoSelectedCardIds.add(itemId); | |
| // Handle different modes | |
| if (mode === 'dual') { | |
| // Dual-panel mode: add as second panel (only if we have exactly 1 panel open) | |
| if (state.openPanelIds.length === 1) { | |
| return { | |
| openPanelIds: [itemId, state.openPanelIds[0]], // New panel becomes left (index 0) | |
| maximizedItemId: itemId, // Focus on the newly opened panel | |
| workspaceSplitViewActive: true, // Enable split view for dual-panel | |
| selectedCardIds: newSelectedCardIds, | |
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | |
| }; | |
| } | |
| // If not exactly 1 panel open, fall through to replace mode | |
| } | |
| if (mode === 'add' && state.openPanelIds.length < 2) { | |
| return { | |
| openPanelIds: [...state.openPanelIds, itemId], | |
| maximizedItemId: itemId, | |
| selectedCardIds: newSelectedCardIds, | |
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | |
| }; | |
| } | |
| // Replace mode (default / fallback): replace all open panels with this one | |
| return { | |
| openPanelIds: [itemId], | |
| maximizedItemId: itemId, | |
| workspaceSplitViewActive: false, | |
| selectedCardIds: newSelectedCardIds, | |
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | |
| }; | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In `@src/lib/stores/ui-store.ts` around lines 245 - 280, The openPanel handler
currently ignores the 'add' mode and lets 'dual' fall through leaving
workspaceSplitViewActive stale; update the openPanel function to explicitly
handle three modes: 'add' should append itemId to openPanelIds (if not present)
and update selectedCardIds and panelAutoSelectedCardIds without replacing
existing panels; 'dual' should only perform the dual-pane swap when
state.openPanelIds.length === 1 and otherwise fall back to a replace that also
clears workspaceSplitViewActive=false; the default/replace branch must
explicitly set workspaceSplitViewActive=false and update openPanelIds,
maximizedItemId, selectedCardIds and panelAutoSelectedCardIds to avoid
inconsistent state.
| setMaximizedItemId: (id) => set((state) => { | ||
| // Disable workspace split view when item is un-maximized | ||
| if (id === null && state.workspaceSplitViewActive) { | ||
| return { maximizedItemId: id, workspaceSplitViewActive: false }; | ||
| } | ||
|
|
||
| // In workspace split view, deselect the previously maximized card when opening a new one | ||
| if (state.workspaceSplitViewActive && state.maximizedItemId && state.maximizedItemId !== id) { | ||
| // In workspace split view, deselect the previously maximized card when switching to a new one | ||
| if (state.workspaceSplitViewActive && id !== null && state.maximizedItemId !== id) { | ||
| const newSelectedCardIds = new Set(state.selectedCardIds); | ||
| if (state.maximizedItemId) { | ||
| newSelectedCardIds.delete(state.maximizedItemId); | ||
| newPanelAutoSelectedCardIds.delete(state.maximizedItemId); | ||
| } | ||
|
|
||
| // Add to selections and track as auto-selected | ||
| newSelectedCardIds.add(id); | ||
| newPanelAutoSelectedCardIds.add(id); | ||
|
|
||
| return { | ||
| openPanelIds: [id], | ||
| maximizedItemId: id, | ||
| selectedCardIds: newSelectedCardIds, | ||
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | ||
| selectedCardIds: newSelectedCardIds | ||
| }; | ||
| } | ||
| }); | ||
| }, | ||
|
|
||
| setShowVersionHistory: (show) => set({ showVersionHistory: show }), | ||
| setShowCreateWorkspaceModal: (show) => set({ showCreateWorkspaceModal: show }), | ||
| setShowSheetModal: (show) => set({ showSheetModal: show }), | ||
|
|
||
| // UI Preferences actions | ||
| setShowJsonView: (show) => set({ showJsonView: show }), | ||
| setSearchQuery: (query) => set({ searchQuery: query }), | ||
|
|
||
| setSelectedActions: (actions) => set({ selectedActions: actions }), | ||
| clearSelectedActions: () => set({ selectedActions: [] }), | ||
| setSelectedModelId: (modelId) => set({ selectedModelId: modelId }), | ||
|
|
||
| // Text selection actions | ||
| setInMultiSelectMode: (inMultiMode) => set({ inMultiSelectMode: inMultiMode }), | ||
| setInSingleSelectMode: (inSingleMode) => set({ inSingleSelectMode: inSingleMode }), | ||
| setTooltipVisible: (visible) => set({ tooltipVisible: visible }), | ||
| setSelectedHighlightColorId: (colorId) => set({ selectedHighlightColorId: colorId }), | ||
| enterMultiSelectMode: () => set({ inMultiSelectMode: true, inSingleSelectMode: false, tooltipVisible: true }), | ||
| exitMultiSelectMode: () => set({ inMultiSelectMode: false, tooltipVisible: false }), | ||
| enterSingleSelectMode: () => set({ inSingleSelectMode: true, inMultiSelectMode: false, tooltipVisible: true }), | ||
| exitSingleSelectMode: () => set({ inSingleSelectMode: false, tooltipVisible: false }), | ||
|
|
||
| // Card selection actions | ||
| toggleCardSelection: (id) => set((state) => { | ||
| const newSet = new Set(state.selectedCardIds); | ||
| const newPanelAutoSelectedCardIds = new Set(state.panelAutoSelectedCardIds); | ||
|
|
||
| if (newSet.has(id)) { | ||
| newSet.delete(id); | ||
| newPanelAutoSelectedCardIds.delete(id); | ||
| } else { | ||
| newSet.add(id); | ||
| } | ||
| return { | ||
| selectedCardIds: newSet, | ||
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | ||
| }; | ||
| }), | ||
|
|
||
| clearCardSelection: () => set({ | ||
| selectedCardIds: new Set<string>(), | ||
| panelAutoSelectedCardIds: new Set<string>(), | ||
| }), | ||
| return { maximizedItemId: id }; | ||
| }), |
There was a problem hiding this comment.
setMaximizedItemId deselects the previous panel's card in split view, even though it's still open.
Lines 361-365: When switching the maximized item in split view, the previously maximized card is removed from selectedCardIds. However, that card may still be open as the other panel (it's still in openPanelIds). This could cause a visual mismatch where an open panel's card appears deselected in the workspace grid.
Consider only deselecting it if it's no longer in openPanelIds:
Proposed fix
if (state.workspaceSplitViewActive && id !== null && state.maximizedItemId !== id) {
const newSelectedCardIds = new Set(state.selectedCardIds);
- if (state.maximizedItemId) {
+ if (state.maximizedItemId && !state.openPanelIds.includes(state.maximizedItemId)) {
newSelectedCardIds.delete(state.maximizedItemId);
}
return {
maximizedItemId: id,
selectedCardIds: newSelectedCardIds
};
}🤖 Prompt for AI Agents
In `@src/lib/stores/ui-store.ts` around lines 354 - 373, In setMaximizedItemId,
avoid deselecting the previous maximized card if that panel is still open: when
handling the branch with state.workspaceSplitViewActive && id !== null &&
state.maximizedItemId !== id, only remove state.maximizedItemId from
state.selectedCardIds after checking that state.maximizedItemId is not present
in state.openPanelIds; keep the Set conversion for selectedCardIds and return
the updated maximizedItemId and selectedCardIds, otherwise leave selectedCardIds
unchanged.
| } else { | ||
| const isAlreadyOpen = state.openPanelIds.length === 1 && state.openPanelIds[0] === id; | ||
| if (isAlreadyOpen) return {}; | ||
|
|
||
| const newSelectedCardIds = new Set(state.selectedCardIds); | ||
| const newPanelAutoSelectedCardIds = new Set(state.panelAutoSelectedCardIds); | ||
|
|
||
| // In workspace split view, deselect the previously maximized card when opening a new one | ||
| if (state.workspaceSplitViewActive && state.maximizedItemId && state.maximizedItemId !== id) { | ||
| newSelectedCardIds.delete(state.maximizedItemId); | ||
| newPanelAutoSelectedCardIds.delete(state.maximizedItemId); | ||
| } | ||
|
|
||
| // Add to selections and track as auto-selected | ||
| newSelectedCardIds.add(id); | ||
| newPanelAutoSelectedCardIds.add(id); | ||
|
|
||
| return { | ||
| openPanelIds: [id], | ||
| maximizedItemId: id, | ||
| selectedCardIds: newSelectedCardIds, | ||
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | ||
| }; | ||
| } |
There was a problem hiding this comment.
setOpenModalItemId (non-null path) doesn't reset workspaceSplitViewActive when collapsing from dual to single panel.
When called with a non-null id while two panels are open, this sets openPanelIds: [id] but doesn't clear workspaceSplitViewActive. Since this is a legacy compatibility shim that callers may use without awareness of the dual-panel feature, it should defensively reset split-view state.
Proposed fix
return {
openPanelIds: [id],
maximizedItemId: id,
+ workspaceSplitViewActive: false,
selectedCardIds: newSelectedCardIds,
panelAutoSelectedCardIds: newPanelAutoSelectedCardIds,
};📝 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.
| } else { | |
| const isAlreadyOpen = state.openPanelIds.length === 1 && state.openPanelIds[0] === id; | |
| if (isAlreadyOpen) return {}; | |
| const newSelectedCardIds = new Set(state.selectedCardIds); | |
| const newPanelAutoSelectedCardIds = new Set(state.panelAutoSelectedCardIds); | |
| // In workspace split view, deselect the previously maximized card when opening a new one | |
| if (state.workspaceSplitViewActive && state.maximizedItemId && state.maximizedItemId !== id) { | |
| newSelectedCardIds.delete(state.maximizedItemId); | |
| newPanelAutoSelectedCardIds.delete(state.maximizedItemId); | |
| } | |
| // Add to selections and track as auto-selected | |
| newSelectedCardIds.add(id); | |
| newPanelAutoSelectedCardIds.add(id); | |
| return { | |
| openPanelIds: [id], | |
| maximizedItemId: id, | |
| selectedCardIds: newSelectedCardIds, | |
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | |
| }; | |
| } | |
| } else { | |
| const isAlreadyOpen = state.openPanelIds.length === 1 && state.openPanelIds[0] === id; | |
| if (isAlreadyOpen) return {}; | |
| const newSelectedCardIds = new Set(state.selectedCardIds); | |
| const newPanelAutoSelectedCardIds = new Set(state.panelAutoSelectedCardIds); | |
| // In workspace split view, deselect the previously maximized card when opening a new one | |
| if (state.workspaceSplitViewActive && state.maximizedItemId && state.maximizedItemId !== id) { | |
| newSelectedCardIds.delete(state.maximizedItemId); | |
| newPanelAutoSelectedCardIds.delete(state.maximizedItemId); | |
| } | |
| // Add to selections and track as auto-selected | |
| newSelectedCardIds.add(id); | |
| newPanelAutoSelectedCardIds.add(id); | |
| return { | |
| openPanelIds: [id], | |
| maximizedItemId: id, | |
| workspaceSplitViewActive: false, | |
| selectedCardIds: newSelectedCardIds, | |
| panelAutoSelectedCardIds: newPanelAutoSelectedCardIds, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@src/lib/stores/ui-store.ts` around lines 407 - 430, The non-null branch of
setOpenModalItemId returns a single-panel state but doesn't clear the legacy
split-view flag; update the returned object in that branch (the block that sets
openPanelIds: [id], maximizedItemId: id, selectedCardIds,
panelAutoSelectedCardIds) to also set workspaceSplitViewActive: false so
collapsing from dual to single panel defensively resets the split-view state.
1 participant
Important
This pull request adds a dual-panel view mode for side-by-side item viewing, updates state management for panel actions, and enhances UI components for improved workspace management.
DashboardLayout.tsxandModalManager.tsxfor side-by-side item viewing.WorkspaceCard.tsxfor split view.WorkspaceContent.tsxhides items shown in open panels.ui-store.tsto manage dual-panel state and panel actions.openPanelmode 'dual' to handle dual-panel opening.DashboardLayout.tsxandModalManager.tsxfor dual and single-panel workflows.This description was created by
for 055f8e7. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit