feat: add split panes#2022
Conversation
Greptile SummaryThis PR adds split-pane support to the task view, introducing a
Confidence Score: 3/5Safe to review but has three issues that should be fixed before merging: a broken CSS concatenation in the PR filter button, a resource leak in snapshot restore, and an overbroad keyboard handler that can swallow input inside modals. The FilterButton in pr-view.tsx silently breaks active state styling due to a missing space in a string concatenation. The restoreSnapshot path in TabGroupManagerStore skips disposing the initial group's tab manager and leaves all prior auto-close reactions running. The global window keydown listener in use-arrow-key-navigation fires preventDefault on arrow keys even when an input inside a modal has focus. The rest of the split-pane architecture, migration path, and editor refactor are well-structured. pr-view.tsx (broken active class), tab-group-manager-store.ts (restoreSnapshot cleanup), use-arrow-key-navigation.ts (missing input focus guard), and workspace-view-model.tsx (DiffTabLifecycleStore wired to initial group only).
|
| Filename | Overview |
|---|---|
| src/renderer/features/projects/components/pr-view/pr-view.tsx | Trailing space removed from FilterButton className concatenation, causing malformed CSS class name when active=true. |
| src/renderer/features/tasks/tabs/tab-group-manager-store.ts | New store owning all per-pane TabManagerStore instances; restoreSnapshot leaks the initial group's tabManager and all auto-close reactions. |
| src/renderer/features/tasks/stores/workspace-view-model.tsx | Replaces TabManagerStore with TabGroupManagerStore; DiffTabLifecycleStore is wired to a single snapshot of focusedGroup rather than reacting to pane changes. |
| src/renderer/lib/hooks/use-arrow-key-navigation.ts | New hook for arrow-key list navigation; global window listener fires without checking if focus is inside an input, which can suppress cursor movement in modal inputs. |
| src/renderer/features/tasks/view/task-main-column.tsx | New file: extracts TaskMainColumn and introduces SplitPaneLayout with DnD support; DraggableResizeHandle moved here from main-panel.tsx. |
| src/renderer/features/tasks/view/pane-content.tsx | New file: per-pane renderer area with tab bar, drop zone overlay, and Activity-based panel switching; correctly scoped to TabGroupContext. |
| src/renderer/features/tasks/tabs/tab-group-context.tsx | New React context providing per-pane groupId and TabManagerStore; clean implementation with guard-throw hook. |
| src/renderer/features/tasks/editor/editor-provider.tsx | Switches from pool-lease pattern to direct Monaco editor creation per pane; cleanup on unmount looks correct. |
| src/shared/view-state.ts | Adds TabGroupsSnapshot; backward-compatible migration via deprecated tabManager field is well-structured. |
| src/renderer/lib/ui/button.tsx | Adds icon-md size (32px); shrinks icon-sm from 32px to 28px — many call sites updated accordingly. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
WVM[WorkspaceViewModel] --> TGM[TabGroupManagerStore]
TGM --> G1[TabGroupEntry #1]
TGM --> G2[TabGroupEntry #2]
TGM -->|splitRight| G2
WVM --> SPL[SplitPaneLayout]
SPL --> TGP1[TabGroupProvider #1]
SPL --> TGP2[TabGroupProvider #2]
TGP1 --> PC1[PaneContent]
TGP2 --> PC2[PaneContent]
TGM -->|focusedGroup| FMLS[FileModelLifecycleStore]
TGM -->|focusedGroup snapshot| DTLS[DiffTabLifecycleStore]
Comments Outside Diff (2)
-
src/renderer/features/projects/components/pr-view/pr-view.tsx, line 208-211 (link)Missing space causes malformed CSS class names
The trailing space was intentionally present to separate the static classes from the conditionally-appended ones. After this change, the concatenated string becomes
...disabled:cursor-not-allowedtext-foreground font-medium(no space betweencursor-not-allowedandtext-foreground) whenactiveis true, producing an invalid CSS class that Tailwind will not recognise. The filter button active styling will silently break.Prompt To Fix With AI
This is a comment left during a code review. Path: src/renderer/features/projects/components/pr-view/pr-view.tsx Line: 208-211 Comment: **Missing space causes malformed CSS class names** The trailing space was intentionally present to separate the static classes from the conditionally-appended ones. After this change, the concatenated string becomes `...disabled:cursor-not-allowedtext-foreground font-medium` (no space between `cursor-not-allowed` and `text-foreground`) when `active` is true, producing an invalid CSS class that Tailwind will not recognise. The filter button active styling will silently break. How can I resolve this? If you propose a fix, please make it concise.
-
src/renderer/features/tasks/tabs/tab-group-manager-store.ts, line 1737-1759 (link)restoreSnapshotleaks the initial group and all auto-close reactionsWhen
restoreSnapshotis called it disposes groups[1..n]but silently skipsgroups[0]— itsTabManagerStoreis never disposed. More importantly, none of the entries in_autoCloseDisposersfor the existing groups are cancelled before the groups array is wiped, leaving dangling MobX reactions that still hold a reference to the closing TabManagerStore. Thedispose()method shows the correct pattern: call disposers first, then dispose tab managers.restoreSnapshotshould mirror that cleanup before rebuilding the group list.Prompt To Fix With AI
This is a comment left during a code review. Path: src/renderer/features/tasks/tabs/tab-group-manager-store.ts Line: 1737-1759 Comment: **`restoreSnapshot` leaks the initial group and all auto-close reactions** When `restoreSnapshot` is called it disposes groups `[1..n]` but silently skips `groups[0]` — its `TabManagerStore` is never disposed. More importantly, none of the entries in `_autoCloseDisposers` for the existing groups are cancelled before the groups array is wiped, leaving dangling MobX reactions that still hold a reference to the closing TabManagerStore. The `dispose()` method shows the correct pattern: call disposers first, then dispose tab managers. `restoreSnapshot` should mirror that cleanup before rebuilding the group list. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 5 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 5
src/renderer/features/projects/components/pr-view/pr-view.tsx:208-211
**Missing space causes malformed CSS class names**
The trailing space was intentionally present to separate the static classes from the conditionally-appended ones. After this change, the concatenated string becomes `...disabled:cursor-not-allowedtext-foreground font-medium` (no space between `cursor-not-allowed` and `text-foreground`) when `active` is true, producing an invalid CSS class that Tailwind will not recognise. The filter button active styling will silently break.
### Issue 2 of 5
src/renderer/features/tasks/tabs/tab-group-manager-store.ts:1737-1759
**`restoreSnapshot` leaks the initial group and all auto-close reactions**
When `restoreSnapshot` is called it disposes groups `[1..n]` but silently skips `groups[0]` — its `TabManagerStore` is never disposed. More importantly, none of the entries in `_autoCloseDisposers` for the existing groups are cancelled before the groups array is wiped, leaving dangling MobX reactions that still hold a reference to the closing TabManagerStore. The `dispose()` method shows the correct pattern: call disposers first, then dispose tab managers. `restoreSnapshot` should mirror that cleanup before rebuilding the group list.
### Issue 3 of 5
src/renderer/lib/hooks/use-arrow-key-navigation.ts:1-33
**Global `window` keydown listener fires without focus guard**
The hook attaches directly to `window` without checking `document.activeElement`. When a modal (e.g., `addProjectModal` or `createConversationModal`) is open and the user presses ArrowUp/ArrowDown inside an `<input>` within it, `e.preventDefault()` fires from this handler, suppressing the cursor movement inside the input field. A guard such as `if (e.target instanceof HTMLInputElement || e.target instanceof HTMLTextAreaElement) return;` would prevent this interference.
### Issue 4 of 5
src/renderer/features/tasks/stores/workspace-view-model.tsx:280-285
**`DiffTabLifecycleStore` is wired to the initial focused group only**
`this.tabGroupManager.focusedGroup` is evaluated once at `initialize()` time and the resulting `TabManagerStore` reference is passed into `DiffTabLifecycleStore`. When the user creates a second pane via `splitRight()`, the new pane's `TabManagerStore` is never observed by `DiffTabLifecycleStore`, so auto-opened diff tabs will only land in the original pane.
### Issue 5 of 5
src/renderer/features/tasks/view/tab-bar/tab-bar-actions.tsx:60
**Tooltip copy: "New Conversations" should be singular**
The tooltip reads "New Conversations" (plural) but the action opens a single new conversation.
```suggestion
New Conversation <ShortcutHint settingsKey="newConversation" />
```
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { useEffect, useRef, useState } from 'react'; | ||
|
|
||
| export function useArrowKeyNavigation(count: number, onEnter: (index: number) => void) { | ||
| const [selectedIndex, setSelectedIndex] = useState(0); | ||
| const selectedIndexRef = useRef(0); | ||
| const onEnterRef = useRef(onEnter); | ||
|
|
||
| useEffect(() => { | ||
| selectedIndexRef.current = selectedIndex; | ||
| }, [selectedIndex]); | ||
|
|
||
| useEffect(() => { | ||
| onEnterRef.current = onEnter; | ||
| }, [onEnter]); | ||
|
|
||
| useEffect(() => { | ||
| const handler = (e: KeyboardEvent) => { | ||
| if (e.key === 'ArrowDown') { | ||
| e.preventDefault(); | ||
| setSelectedIndex((i) => (i + 1) % count); | ||
| } else if (e.key === 'ArrowUp') { | ||
| e.preventDefault(); | ||
| setSelectedIndex((i) => (i - 1 + count) % count); | ||
| } else if (e.key === 'Enter') { | ||
| onEnterRef.current(selectedIndexRef.current); | ||
| } | ||
| }; | ||
| window.addEventListener('keydown', handler); | ||
| return () => window.removeEventListener('keydown', handler); | ||
| }, [count]); | ||
|
|
||
| return { selectedIndex, setSelectedIndex }; | ||
| } |
There was a problem hiding this comment.
window keydown listener fires without focus guard
The hook attaches directly to window without checking document.activeElement. When a modal (e.g., addProjectModal or createConversationModal) is open and the user presses ArrowUp/ArrowDown inside an <input> within it, e.preventDefault() fires from this handler, suppressing the cursor movement inside the input field. A guard such as if (e.target instanceof HTMLInputElement || e.target instanceof HTMLTextAreaElement) return; would prevent this interference.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/lib/hooks/use-arrow-key-navigation.ts
Line: 1-33
Comment:
**Global `window` keydown listener fires without focus guard**
The hook attaches directly to `window` without checking `document.activeElement`. When a modal (e.g., `addProjectModal` or `createConversationModal`) is open and the user presses ArrowUp/ArrowDown inside an `<input>` within it, `e.preventDefault()` fires from this handler, suppressing the cursor movement inside the input field. A guard such as `if (e.target instanceof HTMLInputElement || e.target instanceof HTMLTextAreaElement) return;` would prevent this interference.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
|
|
||
| this._diffTabLifecycle = new DiffTabLifecycleStore( | ||
| this.tabManager, | ||
| this.tabGroupManager.focusedGroup, | ||
| workspace.git, | ||
| this.prStore, |
There was a problem hiding this comment.
DiffTabLifecycleStore is wired to the initial focused group only
this.tabGroupManager.focusedGroup is evaluated once at initialize() time and the resulting TabManagerStore reference is passed into DiffTabLifecycleStore. When the user creates a second pane via splitRight(), the new pane's TabManagerStore is never observed by DiffTabLifecycleStore, so auto-opened diff tabs will only land in the original pane.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/tasks/stores/workspace-view-model.tsx
Line: 280-285
Comment:
**`DiffTabLifecycleStore` is wired to the initial focused group only**
`this.tabGroupManager.focusedGroup` is evaluated once at `initialize()` time and the resulting `TabManagerStore` reference is passed into `DiffTabLifecycleStore`. When the user creates a second pane via `splitRight()`, the new pane's `TabManagerStore` is never observed by `DiffTabLifecycleStore`, so auto-opened diff tabs will only land in the original pane.
How can I resolve this? If you propose a fix, please make it concise.| </Button> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| New Conversations <ShortcutHint settingsKey="newConversation" /> |
There was a problem hiding this comment.
The tooltip reads "New Conversations" (plural) but the action opens a single new conversation.
| New Conversations <ShortcutHint settingsKey="newConversation" /> | |
| New Conversation <ShortcutHint settingsKey="newConversation" /> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/tasks/view/tab-bar/tab-bar-actions.tsx
Line: 60
Comment:
**Tooltip copy: "New Conversations" should be singular**
The tooltip reads "New Conversations" (plural) but the action opens a single new conversation.
```suggestion
New Conversation <ShortcutHint settingsKey="newConversation" />
```
How can I resolve this? If you propose a fix, please make it concise.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!
1 participant
No description provided.