Refactor session tab bar#258
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRefactors chat tabs to a session-centric system with new UI components (SessionTabBar, SessionTab, ClosedSessionsPopover), updates session sendMessage to include agentHarness and persist initial harness, adds an experimental "Apps" flag, enhances file-browser theming and file-mobile detection, and adjusts related hooks and exports. Changes
Sequence Diagram(s)sequenceDiagram
participant Composer as Composer (UI)
participant Hook as useSendMessage/useSessionComposer
participant Service as SessionService
participant Backend as Backend DB / Commands
Composer->>Hook: sendMessage(sessionId, content, model, agentHarness)
Hook->>Service: sendMessage(sessionId, content, model, agentHarness)
Service->>Backend: persist initial agent_harness if message_count==0
Backend-->>Service: success / error
Service-->>Hook: message result or error
Hook-->>Composer: update UI (message added / error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 2/5 reviews remaining, refill in 35 minutes and 9 seconds. Comment |
![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
🧹 Nitpick comments (4)
apps/web/src/features/session/ui/tabs/SessionTab.tsx (1)
24-36: Consider usingts-patternfor status icon rendering.Per coding guidelines,
ts-patternis preferred for discriminated unions. While this is a simple tri-state conditional, usingmatchwould provide exhaustiveness checking if additional states are added later.♻️ Optional refactor with ts-pattern
+import { match } from "ts-pattern"; -function renderStatusIcon(tab: ChatTab, isWorking: boolean, isUnread: boolean) { - if (isWorking) { - return <CircularPixelGrid variant="generating" size={14} resolution={8} />; - } - - if (isUnread) { - return <span className="bg-accent-gold h-2 w-2 rounded-full" />; - } - - const LogoComponent = getAgentLogo(tab.agentHarness); - if (!LogoComponent) return null; - return <LogoComponent className={cn(AGENT_ICON_SIZE, "shrink-0")} />; -} +function renderStatusIcon(tab: ChatTab, isWorking: boolean, isUnread: boolean) { + return match({ isWorking, isUnread }) + .with({ isWorking: true }, () => ( + <CircularPixelGrid variant="generating" size={14} resolution={8} /> + )) + .with({ isUnread: true }, () => ( + <span className="bg-accent-gold h-2 w-2 rounded-full" /> + )) + .otherwise(() => { + const LogoComponent = getAgentLogo(tab.agentHarness); + if (!LogoComponent) return null; + return <LogoComponent className={cn(AGENT_ICON_SIZE, "shrink-0")} />; + }); +}As per coding guidelines:
**/*.{ts,tsx}: Preferts-patternfor discriminated unions. Use.exhaustive()for exhaustiveness checking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/session/ui/tabs/SessionTab.tsx` around lines 24 - 36, Refactor the conditional logic in renderStatusIcon to use ts-pattern's match for exhaustiveness: import { match } from "ts-pattern" and replace the if/else chain with match({ isWorking, isUnread })...pattern that returns the CircularPixelGrid when isWorking is true, the unread dot span when isUnread is true, and otherwise uses getAgentLogo(tab.agentHarness) and returns the LogoComponent with className={cn(AGENT_ICON_SIZE, "shrink-0")} (or null if no logo); finish the chain with .exhaustive() to satisfy the coding guideline. Ensure you keep references to CircularPixelGrid, getAgentLogo, AGENT_ICON_SIZE, and cn unchanged and preserve current JSX outputs.apps/web/src/app/layouts/useChatTabs.ts (2)
152-164: Type coercion in tab patch may cause type mismatch.When patching the default tab with session info, the spread
...updated[defaultIdx]could be aPendingChatTab, but the result is assigned directly. While TypeScript may allow this due to structural typing, the explicitkindandsessionIdassignment overrides correctly make it a validSessionChatTab. This is acceptable but could be clearer.♻️ Optional: More explicit type construction
const updated = [...tabs]; + const oldTab = updated[defaultIdx]; updated[defaultIdx] = { - ...updated[defaultIdx], kind: "session", id: `tab-${activeSessionId}`, sessionId: activeSessionId, + label: oldTab.label, + agentHarness: oldTab.agentHarness, + hasStarted: oldTab.hasStarted, + initialModel: oldTab.initialModel, - }; + } satisfies SessionChatTab; return updated;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/layouts/useChatTabs.ts` around lines 152 - 164, The patch to the default tab may mix types (a PendingChatTab spread then turned into a SessionChatTab); update the setMainTabs callback so instead of blindly spreading updated[defaultIdx], explicitly construct a SessionChatTab object (or perform a clear type-safe cast) with fields: kind: "session", id: `tab-${activeSessionId}`, sessionId: activeSessionId and copy only compatible fields (e.g., title or metadata) from the original; ensure you reference setMainTabs, defaultIdx, "tab-default", and activeSessionId when locating and replacing the tab to make the conversion explicit and type-safe.
375-384: Consider extractingisTextFieldFocusedoutside the component.This utility function has no dependencies on component state and could be extracted to a shared utility module for reuse and testability. Also, consider checking for
[role="searchbox"]which is another ARIA text input role.♻️ Optional: Extract utility
// In a shared utility file, e.g., `@/shared/lib/dom.ts` export function isTextFieldFocused(): boolean { const activeElement = document.activeElement as HTMLElement | null; if (!activeElement) return false; const tagName = activeElement.tagName; if (tagName === "INPUT" || tagName === "TEXTAREA") return true; if (activeElement.isContentEditable) return true; const role = activeElement.getAttribute("role"); return role === "textbox" || role === "searchbox"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/layouts/useChatTabs.ts` around lines 375 - 384, Extract the isTextFieldFocused function out of the component into a shared utility (e.g., export from a shared/dom or shared/lib module) because it has no component dependencies; update useChatTabs to import and use that utility. In the extracted function (keep the original name isTextFieldFocused) preserve the same logic but early-return on null activeElement, check tagName === "INPUT" or "TEXTAREA", check activeElement.isContentEditable, and extend the aria-role check to treat role === "textbox" || role === "searchbox" as true. Finally, replace the local definition in useChatTabs with an import of the new utility.apps/web/src/features/session/ui/tabs/SessionTabBar.tsx (1)
179-203: Simplify onClick handler.Since
onTabAddtakes no arguments, the wrapper arrow function is unnecessary and creates a new function reference on each render.♻️ Suggested simplification
<button type="button" aria-label="New chat tab" - onClick={() => onTabAdd()} + onClick={onTabAdd} className={cn(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/session/ui/tabs/SessionTabBar.tsx` around lines 179 - 203, The onClick handler for the "New chat" button wraps onTabAdd in an unnecessary arrow which creates a new function each render; change the button's onClick from onClick={() => onTabAdd()} to onClick={onTabAdd} in SessionTabBar (the button inside the TooltipTrigger) to pass the handler reference directly and avoid allocating a new function each render.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/features/session/ui/tabs/SessionTab.tsx`:
- Around line 62-91: The close button's onClick currently calls onClose directly
and lets clicks bubble into the sortable container; change the onClick handler
in SessionTab (the button element that uses onClick={onClose}) to accept the
click event, call event.stopPropagation(), then invoke the onClose prop
(preserving optional chaining if used) so the close click won't trigger parent
drag/selection handlers; update the handler signature/type to accept a
React.MouseEvent if you need to satisfy TypeScript.
In `@apps/web/src/features/session/ui/tabs/SortableSessionTab.tsx`:
- Around line 16-37: useSortable is destructured without capturing and applying
the returned attributes, so ARIA accessibility attributes are missing; update
the useSortable call to also destructure attributes and spread them onto the
draggable container (the same element using setNodeRef and {...listeners}) so
screen-reader ARIA props like aria-roledescription and aria-describedby are
applied; specifically modify the useSortable return destructuring to include
attributes and add {...attributes} to the <div> that uses setNodeRef and
{...listeners}.
---
Nitpick comments:
In `@apps/web/src/app/layouts/useChatTabs.ts`:
- Around line 152-164: The patch to the default tab may mix types (a
PendingChatTab spread then turned into a SessionChatTab); update the setMainTabs
callback so instead of blindly spreading updated[defaultIdx], explicitly
construct a SessionChatTab object (or perform a clear type-safe cast) with
fields: kind: "session", id: `tab-${activeSessionId}`, sessionId:
activeSessionId and copy only compatible fields (e.g., title or metadata) from
the original; ensure you reference setMainTabs, defaultIdx, "tab-default", and
activeSessionId when locating and replacing the tab to make the conversion
explicit and type-safe.
- Around line 375-384: Extract the isTextFieldFocused function out of the
component into a shared utility (e.g., export from a shared/dom or shared/lib
module) because it has no component dependencies; update useChatTabs to import
and use that utility. In the extracted function (keep the original name
isTextFieldFocused) preserve the same logic but early-return on null
activeElement, check tagName === "INPUT" or "TEXTAREA", check
activeElement.isContentEditable, and extend the aria-role check to treat role
=== "textbox" || role === "searchbox" as true. Finally, replace the local
definition in useChatTabs with an import of the new utility.
In `@apps/web/src/features/session/ui/tabs/SessionTab.tsx`:
- Around line 24-36: Refactor the conditional logic in renderStatusIcon to use
ts-pattern's match for exhaustiveness: import { match } from "ts-pattern" and
replace the if/else chain with match({ isWorking, isUnread })...pattern that
returns the CircularPixelGrid when isWorking is true, the unread dot span when
isUnread is true, and otherwise uses getAgentLogo(tab.agentHarness) and returns
the LogoComponent with className={cn(AGENT_ICON_SIZE, "shrink-0")} (or null if
no logo); finish the chain with .exhaustive() to satisfy the coding guideline.
Ensure you keep references to CircularPixelGrid, getAgentLogo, AGENT_ICON_SIZE,
and cn unchanged and preserve current JSX outputs.
In `@apps/web/src/features/session/ui/tabs/SessionTabBar.tsx`:
- Around line 179-203: The onClick handler for the "New chat" button wraps
onTabAdd in an unnecessary arrow which creates a new function each render;
change the button's onClick from onClick={() => onTabAdd()} to
onClick={onTabAdd} in SessionTabBar (the button inside the TooltipTrigger) to
pass the handler reference directly and avoid allocating a new function each
render.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a603051-2d0f-4cb7-9a75-de74e0a2cbe1
📒 Files selected for processing (13)
apps/web/src/app/layouts/ChatArea.tsxapps/web/src/app/layouts/useChatTabs.tsapps/web/src/features/session/index.tsapps/web/src/features/session/ui/index.tsapps/web/src/features/session/ui/tabs/ClosedSessionsPopover.tsxapps/web/src/features/session/ui/tabs/SessionTab.tsxapps/web/src/features/session/ui/tabs/SessionTabBar.tsxapps/web/src/features/session/ui/tabs/SortableSessionTab.tsxapps/web/src/features/session/ui/tabs/index.tsapps/web/src/features/session/ui/tabs/types.tsapps/web/src/features/workspace/ui/MainContentTabs.tsxapps/web/src/features/workspace/ui/SortableTab.tsxapps/web/src/features/workspace/ui/index.ts
💤 Files with no reviewable changes (3)
- apps/web/src/features/workspace/ui/index.ts
- apps/web/src/features/workspace/ui/SortableTab.tsx
- apps/web/src/features/workspace/ui/MainContentTabs.tsx
Owner
Author
Greenlight — Round 1Fixed (1): Close-button pointer/click propagation is stopped before closing the tab. |
Owner
Author
Greenlight — Round 1Fixed CodeRabbit's sortable accessibility comment by spreading The close-button propagation comment was already handled in the current code via both Local validation passed: targeted ESLint, web TypeScript, and backend TypeScript. Full repo lint still has unrelated pre-existing failures outside this branch. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/web/src/features/workspace/hooks/useWorkspaceIsMobileProject.ts (1)
24-45: Usets-patternwith.exhaustive()for theProjectFileNodediscriminated union.The
node.typecheck at line 33 should use ts-pattern per the repo guideline for discriminated unions. Additionally, add an explicit type guard on the recursive call at line 39 sincechildrenonly exists on directory nodes.Suggested refactor
+import { match } from "ts-pattern"; ... function hasMobileProjectMarker(files: ProjectFileNode[] | undefined): boolean { for (const node of files ?? []) { const name = node.name.toLowerCase(); const pathSegments = node.path.toLowerCase().split("/"); - - if ( - MOBILE_FILE_MARKERS.has(name) || - name.endsWith(".xcodeproj") || - name.endsWith(".xcworkspace") || - (node.type === "directory" && MOBILE_DIRECTORY_MARKERS.has(name)) || - pathSegments.some((segment) => MOBILE_DIRECTORY_MARKERS.has(segment)) - ) { + const hasMarker = match(node) + .with({ type: "file" }, () => + MOBILE_FILE_MARKERS.has(name) || + name.endsWith(".xcodeproj") || + name.endsWith(".xcworkspace") || + pathSegments.some((segment) => MOBILE_DIRECTORY_MARKERS.has(segment)) + ) + .with({ type: "directory" }, () => + MOBILE_DIRECTORY_MARKERS.has(name) || + pathSegments.some((segment) => MOBILE_DIRECTORY_MARKERS.has(segment)) + ) + .exhaustive(); + + if (hasMarker) { return true; } - - if (hasMobileProjectMarker(node.children)) { + if (node.type === "directory" && hasMobileProjectMarker(node.children)) { return true; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/workspace/hooks/useWorkspaceIsMobileProject.ts` around lines 24 - 45, Replace the ad-hoc discriminated-union check in hasMobileProjectMarker with ts-pattern: use match(node) and a pattern branch for { type: "directory" } to check MOBILE_DIRECTORY_MARKERS and to access node.children, and include .exhaustive() to ensure all ProjectFileNode variants are handled; also add an explicit type guard before the recursive call so hasMobileProjectMarker(node.children) is only invoked when node is a directory and children is defined (i.e., inside the { type: "directory" } branch), keeping the existing checks for name markers and pathSegments (MOBILE_FILE_MARKERS, MOBILE_DIRECTORY_MARKERS).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/src/services/agent/commands.ts`:
- Around line 315-322: The current in-memory check using session.message_count
=== 0 can race; change the update to be conditional in SQL so only a session
still at message_count = 0 (and optionally with the expected old agent_harness)
is updated: replace the unconditional UPDATE in commands.ts that uses sessionId
and agentHarness with an UPDATE that includes WHERE id = ? AND message_count = 0
(and/or AND agent_harness = ?) and then check the statement's run result/changes
to detect whether the row was actually modified; this ensures session,
message_count, agent_harness binding is atomic and race-safe.
In `@apps/web/src/features/session/hooks/useSessionComposer.ts`:
- Around line 57-64: fallbackComposer is currently memoized only on initialModel
and defaultThinking, so the same object can be reused across sessions; ensure
each session gets a fresh instance by incorporating sessionId into the key or
creating the fallback inside the effect. Update the useMemo for fallbackComposer
to include sessionId in its dependency list (or move the
emptyComposer(initialModel, defaultThinking) call into the useEffect) so
sessionComposerActions.seedIfAbsent(sessionId, fallbackComposer) always receives
a unique fallback per sessionId; reference symbols: fallbackComposer,
emptyComposer, initialModel, defaultThinking, useEffect,
sessionComposerActions.seedIfAbsent, sessionId.
In `@apps/web/src/features/session/ui/tabs/ClosedSessionsPopover.tsx`:
- Line 48: The displayed shortcut text in ClosedSessionsPopover currently shows
a Mac-only symbol "⌘⇧T" while the handler supports Ctrl+Shift+T cross-platform;
update the JSX in the ClosedSessionsPopover component to show a cross-platform
hint (e.g., "Restore closed session (Ctrl/Cmd+Shift+T)" or render
platform-specific text using a quick runtime check like navigator.platform) so
Windows/Linux users see the correct shortcut; modify the string rendered where
"Restore closed session (⌘⇧T)" appears to the chosen cross-platform form to
match the existing handler.
---
Nitpick comments:
In `@apps/web/src/features/workspace/hooks/useWorkspaceIsMobileProject.ts`:
- Around line 24-45: Replace the ad-hoc discriminated-union check in
hasMobileProjectMarker with ts-pattern: use match(node) and a pattern branch for
{ type: "directory" } to check MOBILE_DIRECTORY_MARKERS and to access
node.children, and include .exhaustive() to ensure all ProjectFileNode variants
are handled; also add an explicit type guard before the recursive call so
hasMobileProjectMarker(node.children) is only invoked when node is a directory
and children is defined (i.e., inside the { type: "directory" } branch), keeping
the existing checks for name markers and pathSegments (MOBILE_FILE_MARKERS,
MOBILE_DIRECTORY_MARKERS).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc578598-78d2-41a5-97b5-99f32de19639
📒 Files selected for processing (13)
apps/backend/src/services/agent/commands.tsapps/web/src/app/layouts/ContentTabBar.tsxapps/web/src/app/layouts/useChatTabs.tsapps/web/src/features/file-browser/api/useFiles.tsapps/web/src/features/file-browser/lib/fileTreeTheme.tsapps/web/src/features/session/api/session.queries.tsapps/web/src/features/session/api/session.service.tsapps/web/src/features/session/hooks/useSessionComposer.tsapps/web/src/features/session/ui/tabs/ClosedSessionsPopover.tsxapps/web/src/features/session/ui/tabs/SortableSessionTab.tsxapps/web/src/features/session/ui/tabs/types.tsapps/web/src/features/workspace/hooks/index.tsapps/web/src/features/workspace/hooks/useWorkspaceIsMobileProject.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/features/workspace/hooks/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/features/session/ui/tabs/types.ts
- apps/web/src/features/session/ui/tabs/SortableSessionTab.tsx
This was referenced May 9, 2026
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.
Moves the chat tab row out of features/workspace into features/session/ui/tabs and replaces the old nullable tab payload with explicit ChatTab and ClosedSessionTab types for pending and session-backed tabs. Updates useChatTabs and ChatArea to use the new model, preserve closed-tab metadata, and wire focus restoration plus keyboard navigation through the new SessionTabBar. The refactor also fixes the new-tab click event leak, removes the extra DnD wrapper button semantics, and guards Cmd+W when a text field is focused. Verified with ./node_modules/.bin/tsc --noEmit --project tsconfig.web.json, targeted ESLint, and an agent-browser smoke test for create/close/restore/reorder; bun run test:backend still has three integration-suite failures from a local better-sqlite3 ABI mismatch.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores