feat(chat): add multi-chat workspaces and workspace tabs - #4372
feat(chat): add multi-chat workspaces and workspace tabs#4372azizmejri1 wants to merge 4 commits into
Conversation
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge The pane-scoping refactor ( The blocker is not in the workspace feature itself but in a regression it introduces to the single-chat path: Issues Summary
🟢 Low Priority Notes (5 items)
Notes on confidence: the diff is complete (not truncated), but the workspace UI was reviewed statically — I did not run the app, so the pane-sizing and tab-capacity findings are reasoned from the CSS and arithmetic rather than observed. Generated by Dyadbot persona-based code review |
github-actions
Bot
added
the
needs-human:review-issue
🤖 Claude Code Review SummaryPR Confidence: 4/5All trusted review findings are fixed and explicitly resolved, and targeted tests, lint, type checks, and the production build pass; confidence is held below 5 because the multi-pane UI flow was not exercised by a dedicated E2E test in this run. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions; the review findings were concrete correctness, resilience, accessibility-copy, and usability issues rather than ambiguous product trade-offs. 🤖 Generated by Claude Code |
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge This is a well-structured feature: the identity scoping ( The blocking problem is the one the PR's own plan flagged as the top risk: mounting several Coverage is also thinner than the change warrants: there are unit tests for the atoms and tab helpers, but no hybrid/integration or E2E test mounting two panes and proving focus, draft, and action isolation, which is what the plan's Phase 1/3 called for and what would have caught the issues above. The diff was complete (not truncated), so confidence in the file-level findings is good; the focus-hijack and terminal-focus findings rely on reading unchanged code in Issues Summary
🟢 Low Priority Notes (6 items)
Generated by Dyadbot persona-based code review |
🤖 Claude Code Review SummaryPR Confidence: 4/5All trusted review findings are fixed and explicitly resolved, and targeted tests, lint, type checks, and the production build pass; confidence is held below 5 because the multi-pane UI flow was not exercised by a dedicated E2E test in this run. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions; the review findings were concrete correctness, resilience, accessibility-copy, and usability issues rather than ambiguous product trade-offs. 🤖 Generated by Claude Code |
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge The pane-scoping work is the strong part of this PR: The blocker is that focusing a pane still drives the global single-chat presentation transition, which is built on document-level DOM lookups and shared preview/panel atoms. In a multi-pane workspace that transition targets the wrong pane, so clicking a background pane scrolls a different pane and can pop the shared preview open. Two related lifecycle and rendering gaps and the absence of any test that mounts the workspace are noted below. The diff was complete (no truncated patches), so confidence in the diff-level findings is good. Verification of the HIGH finding is based on reading the current Issues Summary
🟢 Low Priority Notes (6 items)
Generated by Dyadbot persona-based code review |
- isolate workspace focus from single-chat presentation transitions - recover id-less routes and prune transferred workspace tabs - cover multi-pane isolation and the packaged workspace workflow
🤖 Claude Code Review SummaryPR Confidence: 5/5All trusted review findings were fixed, explicitly resolved, and covered by passing unit, hybrid integration, packaged-build, and targeted Playwright checks. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions 🤖 Generated by Claude Code |
| }, [chatId, setSelectedChatId]); | ||
|
|
||
| useEffect(() => { | ||
| if (selectedAppId === null || loading) return; |
There was a problem hiding this comment.
🟡 MEDIUM
Failed chats query silently wipes persisted workspace membership
The prune effect only guards on loading, which is TanStack Query v5's isPending && isFetching. If ipc.chat.getChats fails on a cold load (no cached data), the query settles into the error state with loading === false and data === undefined, so useChats returns chats: [] and validChatIds is empty. pruneChatWorkspace then clears every visible chat id for the selected app, and because chatWorkspaceByAppIdAtom is an atomWithStorage, that empty state is immediately written to localStorage. The user's workspace is permanently lost from a transient IPC failure, with no error surfaced.
💡 Suggestion: Skip pruning unless the chats query actually succeeded (e.g. return the query's isSuccess/isError from useChats and require isSuccess before calling pruneChatWorkspace).
| className={cn( | ||
| "min-h-0 flex-1 overflow-hidden", | ||
| isMultiChatWorkspace && | ||
| "scrollbar-on-hover grid auto-rows-[minmax(320px,1fr)] grid-cols-[repeat(auto-fit,minmax(min(100%,320px),1fr))] gap-1 overflow-auto bg-border p-1", |
There was a problem hiding this comment.
🟡 MEDIUM
Workspace mounts every member pane with no cap or compaction
The grid renders one full ChatPanel per workspace member with auto-fit columns and overflow-auto, and nothing limits membership: showChatInWorkspaceAtom appends unconditionally and the tab context menu lets a user add every open chat. Each pane mounts a Virtuoso message list, issues its own ipc.chat.getChat fetch, and subscribes to its own stream state, so a user who adds 10-20 chats gets 10-20 live chat panels at once. The plan committed in this PR lists compact/minimized presentation for excess panes as MVP scope and requires profiling at 4/6/10 chats before scalability claims; neither compaction nor any pane budget landed here.
💡 Suggestion: Add a pane budget (render full panels up to N and a compact/minimized row beyond it), or at minimum cap workspace membership and surface why further adds are refused.
| </div> | ||
| <ChatPaneProvider chatId={chatId}> | ||
| <div className="relative flex h-full flex-col overflow-hidden"> | ||
| <ChatHeader |
There was a problem hiding this comment.
🟡 MEDIUM
Panes show no chat title and duplicate app-level header chrome
ChatHeader renders no chat title - it renders app-level controls (New chat, version history, terminal toggle, preview toggle) plus the branch and uncommitted-files banners. In a multi-pane workspace every pane therefore looks identical apart from message content: the only chat identification is the invisible aria-label on the wrapping <section>, so a sighted user comparing two chats side by side cannot tell which pane is which. At the same time the app-scoped 'New chat' button, version-history button, branch warning, and uncommitted-files banner are repeated once per pane, and clicking 'New chat' in a background pane navigates the whole view away.
💡 Suggestion: Render a pane-local title bar with the chat title (and status) when the panel is in workspace mode, and hoist or hide the app-scoped header controls and banners for non-focused panes.
| })} | ||
| </span> | ||
| </div> | ||
| {isMultiChatWorkspace && ( |
There was a problem hiding this comment.
🟡 MEDIUM
Single-chat workspace hides both exit and remove affordances
isMultiChatWorkspace requires workspaceChatIds.length > 1, and it gates both the 'Open focused chat' button and the per-pane onRemoveFromWorkspace X. A workspace that contains exactly one chat (the state right after the first 'Add to workspace', or after removing down to the last pane) therefore renders a header strip that says '1 chat in workspace' with no control at all: no way to leave the workspace route and no way to remove the last member from within the view. The only escape is clicking an unrelated chat tab and then right-clicking the chat tab to remove it.
💡 Suggestion: Gate 'Open focused chat' on isWorkspaceView rather than isMultiChatWorkspace, and keep the pane remove action available for the last remaining pane (or add an explicit 'Close workspace' action on the workspace tab).
| @@ -1424,6 +1678,13 @@ export function ChatTabs({ selectedChatId }: ChatTabsProps) { | |||
| }) | |||
| .filter((record): record is ClosedTabRecord => record !== null); | |||
|
|
|||
There was a problem hiding this comment.
🟡 MEDIUM
Closing a tab drops workspace membership that reopen never restores
The close path calls hideChatFromWorkspace for every closed record, but reopenClosedTab only restores the tab - it never calls showChatInWorkspace. A user who closes a tab that was part of a workspace and then uses 'Reopen closed tab' gets the chat back but silently loses its workspace membership and its position in the pane order, with no indication that closing the tab also changed the workspace. This is inconsistent with the PR's stated model that tab ownership and workspace membership are separate concepts.
💡 Suggestion: Record workspace membership (and index) in the ClosedTabRecord and re-apply it in reopenClosedTab, the same way the cross-window transfer path already captures wasInWorkspace for rollback.
| appId: number, | ||
| update: (chatIds: number[]) => number[], | ||
| ): ChatWorkspaceByAppId { | ||
| const visibleChatIds = update(workspaces[appId]?.visibleChatIds ?? []); |
There was a problem hiding this comment.
🟡 MEDIUM
hideChatFromWorkspace always writes, creating empty stored entries
updateWorkspace unconditionally returns a new object with [appId]: { visibleChatIds }, so hideChatFromWorkspaceAtom writes a fresh atom value even when the chat was never in a workspace. Because it is called for every closed tab and every cross-window transfer, ordinary tab closing creates { appId: { visibleChatIds: [] } } entries for apps that never had a workspace, persists a JSON localStorage write each time (setItem keeps empty arrays through normalizeChatWorkspaceByAppId), and re-renders every chatWorkspaceByAppIdAtom consumer including the tab strip and all mounted panes. The empty entries accumulate and are never garbage collected.
💡 Suggestion: Return the previous workspaces object unchanged when the update is a no-op, and drop app entries whose visibleChatIds becomes empty.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues The identity-scoping half of this PR is solid: No merge-blocking defects were found. The issues below are lifecycle/resource/UX gaps in the new workspace surface rather than correctness bugs in the scoping work. Notably, the committed plan ( The diff was provided in full ( Issues Summary
🟢 Low Priority Notes (7 items)
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results❌ Some tests failed
Summary: 287 passed, 7 failed, 1 flaky, 12 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/chat_history.spec.ts \
e2e-tests/local_agent_advanced.spec.ts \
e2e-tests/local_agent_auto.spec.ts \
e2e-tests/local_agent_explore_code.spec.ts \
e2e-tests/mention_files.spec.ts \
e2e-tests/plan_mode.spec.ts
|
1 participant
Summary
Adds a per-app multi-chat workspace that lets users view and interact with several chats at once while preserving the existing standalone-chat experience. Workspace membership remains explicit and window-session scoped, and this review revision makes focus changes presentation-safe and covers the real multi-pane workflow.
/chatroute keeps rendering its shell while app/chat redirects settle, and route-provided app identity now self-heals before redirecting.How to use
Behavior