feat: main panel terminal pinning#1736
Open
tom-nash wants to merge 5 commits intogeneralaction:mainfrom
Open
Conversation
Contributor
Greptile SummaryThis PR adds first-class terminal pinning to the main panel: pinned terminals appear as tabs in the Confidence Score: 4/5Safe to merge after addressing the two P2 correctness items; no data-loss or crash risk in the primary user path. All three comments are P2: the SelectItem padding overlap is a visual regression for long terminal names, the canDelete/pinned-terminal guard is an edge-case that requires a specific persisted-state sequence to trigger, and the 250 ms suppression is fragile but practically fine. None block the core feature. Score of 4 reflects that the developer should review these before merge rather than after. src/renderer/components/ui/select.tsx (action button padding) and src/renderer/components/TaskTerminalPanel.tsx (canDelete guard, suppressSelectionUntilRef)
|
| Filename | Overview |
|---|---|
| src/renderer/lib/taskTerminalsStore.ts | Adds pinned field to TaskTerminal, with setPinned / togglePinned store functions and persistence via localStorage serialization. Logic is clean and the updateTaskState guard correctly handles all-terminals-pinned edge cases. |
| src/renderer/components/ui/select.tsx | Extends SelectItem with an optional action slot rendered at absolute right-7. The existing pr-8 padding (32 px) doesn't account for the extra 14 px button width, so the pin icon can overlap terminal title text for longer names. |
| src/renderer/components/TaskTerminalPanel.tsx | Adds pinned-terminal filtering, cross-panel focus dispatching via FOCUS_PINNED_TERMINAL_EVENT, and pin-toggle UI inside the Select dropdown. The suppressSelectionUntilRef timestamp approach for blocking spurious onValueChange fires is sound but fragile; canDelete edge-case with pinned selection is worth guarding. |
| src/renderer/components/ChatInterface.tsx | Adds pinned-terminal state, PinnedTerminalTabButton, CMD+N tab selection for pinned terminals, and cross-panel event listener. Key logic (cleanup effect, active terminal plumbing, auto-start guard) all look correct. |
Sequence Diagram
sequenceDiagram
participant User
participant TaskTerminalPanel
participant Store as taskTerminalsStore
participant Window as window (custom event)
participant ChatInterface
User->>TaskTerminalPanel: clicks Pin button on Terminal T1
TaskTerminalPanel->>Store: togglePinned(T1.id)
Store-->>TaskTerminalPanel: state updated (T1.pinned = true)
TaskTerminalPanel->>TaskTerminalPanel: handleTogglePinned → selection.onChange(T2)
User->>TaskTerminalPanel: selects pinned T1 from dropdown
TaskTerminalPanel->>TaskTerminalPanel: handleSelectionChange detects pinned
TaskTerminalPanel->>Window: dispatchEvent(FOCUS_PINNED_TERMINAL_EVENT, {taskId, scope, terminalId})
TaskTerminalPanel->>TaskTerminalPanel: selection.setIsOpen(false)
Window-->>ChatInterface: onFocusPinned listener fires
ChatInterface->>ChatInterface: handleSelectPinnedTerminal → setActivePinnedTerminalKey(T1.key)
ChatInterface-->>User: PinnedTerminalTabButton becomes active, TerminalPane switches to T1
User->>ChatInterface: clicks PinOff on pinned tab
ChatInterface->>Store: sidebarTaskTerminals.setPinned(T1.id, false)
Store-->>ChatInterface: state updated (T1.pinned = false)
ChatInterface->>ChatInterface: setActivePinnedTerminalKey(null) → reverts to agent chat
Comments Outside Diff (1)
-
src/renderer/components/TaskTerminalPanel.tsx, line 833-874 (link)canDeletecheck can enable deletion of a pinned terminalcanDeleteis based onvisibleTaskTerminals.length > 1, but the actual delete call usesselection.activeTerminalId, which is derived fromselection.parsed.id. In the specific scenario where the store's persistedactiveIdis a pinned terminal on initial mount,selection.parsed.idwill be that pinned terminal's ID whilevisibleTaskTerminalsmay still contain other terminals. IfvisibleTaskTerminals.length > 1in that state, the X button becomes enabled and clicking it callscloseTerminalon the pinned terminal, deleting it and its main-panel tab.Adding a pinned-membership guard makes this unconditionally safe:
const isSelectedPinned = (selection.parsed?.mode === 'task' && pinnedTaskTerminalIds.has(selection.activeTerminalId ?? '')) || (selection.parsed?.mode === 'global' && pinnedGlobalTerminalIds.has(selection.activeTerminalId ?? '')); const canDelete = !isSelectedPinned && (selection.parsed?.mode === 'task' ? visibleTaskTerminals.length > 1 : selection.parsed?.mode === 'global' ? visibleGlobalTerminals.length > 1 : false);
Prompt To Fix With AI
This is a comment left during a code review. Path: src/renderer/components/TaskTerminalPanel.tsx Line: 833-874 Comment: **`canDelete` check can enable deletion of a pinned terminal** `canDelete` is based on `visibleTaskTerminals.length > 1`, but the actual delete call uses `selection.activeTerminalId`, which is derived from `selection.parsed.id`. In the specific scenario where the store's persisted `activeId` is a pinned terminal on initial mount, `selection.parsed.id` will be that pinned terminal's ID while `visibleTaskTerminals` may still contain other terminals. If `visibleTaskTerminals.length > 1` in that state, the X button becomes enabled and clicking it calls `closeTerminal` on the pinned terminal, deleting it and its main-panel tab. Adding a pinned-membership guard makes this unconditionally safe: ```ts const isSelectedPinned = (selection.parsed?.mode === 'task' && pinnedTaskTerminalIds.has(selection.activeTerminalId ?? '')) || (selection.parsed?.mode === 'global' && pinnedGlobalTerminalIds.has(selection.activeTerminalId ?? '')); const canDelete = !isSelectedPinned && (selection.parsed?.mode === 'task' ? visibleTaskTerminals.length > 1 : selection.parsed?.mode === 'global' ? visibleGlobalTerminals.length > 1 : false); ``` How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/renderer/components/ui/select.tsx
Line: 109-128
Comment:
**Action button overlaps terminal title for longer names**
The item always gets `pr-8` (32 px of right padding), but the action `<span>` is `absolute right-7` (28 px from the right edge) and the button inside is `w-3.5` (14 px wide). The button's left edge therefore sits at 42 px from the right edge — 10 px past the text-content boundary — so terminal names long enough to reach that area get clipped behind the pin icon.
```suggestion
const SelectItem = React.forwardRef<React.ElementRef<typeof SelectPrimitive.Item>, SelectItemProps>(
({ className, children, action, ...props }, ref) => (
<SelectPrimitive.Item
ref={ref}
className={cn(
'relative flex w-full cursor-pointer select-none items-center rounded-sm py-1.5 pl-2 text-sm outline-none focus:bg-accent focus:text-accent-foreground data-[disabled]:pointer-events-none data-[disabled]:opacity-50',
action ? 'pr-12' : 'pr-8',
className
)}
{...props}
>
{action ? <span className="absolute right-7 flex items-center">{action}</span> : null}
<span className="absolute right-2 flex h-3.5 w-3.5 items-center justify-center">
<SelectPrimitive.ItemIndicator>
<Check className="h-4 w-4" />
</SelectPrimitive.ItemIndicator>
</span>
<SelectPrimitive.ItemText>{children}</SelectPrimitive.ItemText>
</SelectPrimitive.Item>
)
);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/renderer/components/TaskTerminalPanel.tsx
Line: 833-874
Comment:
**`canDelete` check can enable deletion of a pinned terminal**
`canDelete` is based on `visibleTaskTerminals.length > 1`, but the actual delete call uses `selection.activeTerminalId`, which is derived from `selection.parsed.id`. In the specific scenario where the store's persisted `activeId` is a pinned terminal on initial mount, `selection.parsed.id` will be that pinned terminal's ID while `visibleTaskTerminals` may still contain other terminals. If `visibleTaskTerminals.length > 1` in that state, the X button becomes enabled and clicking it calls `closeTerminal` on the pinned terminal, deleting it and its main-panel tab.
Adding a pinned-membership guard makes this unconditionally safe:
```ts
const isSelectedPinned =
(selection.parsed?.mode === 'task' &&
pinnedTaskTerminalIds.has(selection.activeTerminalId ?? '')) ||
(selection.parsed?.mode === 'global' &&
pinnedGlobalTerminalIds.has(selection.activeTerminalId ?? ''));
const canDelete =
!isSelectedPinned &&
(selection.parsed?.mode === 'task'
? visibleTaskTerminals.length > 1
: selection.parsed?.mode === 'global'
? visibleGlobalTerminals.length > 1
: false);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/renderer/components/TaskTerminalPanel.tsx
Line: 283-290
Comment:
**250 ms suppression window may be too short on slow devices**
`suppressSelectionUntilRef.current = Date.now() + 250` suppresses the `onValueChange` that Radix Select fires after a pointer interaction on a `SelectItem`. On a heavily loaded renderer the synthetic click (and therefore the `onValueChange` call) can arrive more than 250 ms after the `pointerdown`, leaving the guard expired and causing the select to switch to the pinned terminal's value anyway.
A more deterministic alternative is a simple boolean ref that is cleared once `handleSelectionChange` fires:
```ts
const pinButtonClickedRef = useRef(false);
// in onPointerDown:
pinButtonClickedRef.current = true;
// in handleSelectionChange:
if (pinButtonClickedRef.current) {
pinButtonClickedRef.current = false;
return;
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(ui): remove stray terminal pin icon ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hello 👋🏻
Summary
Adds support for pinning terminals (worktree/project) into the main panel as first-class tabs, so users can quickly switch between agent chats and pinned terminals in one place and with the existing CMD+ keyboard shortcuts.
This PR introduces:
chat tabs.
Snapshot
Add screenshots, GIFs, or videos demonstrating the changes (if applicable)
Demo
Screenshot
Type of change
Mandatory Tasks
Checklist