Refactor chat into a composable workspace shell#2011
Refactor chat into a composable workspace shell#2011juliusmarminge wants to merge 3 commits intomainfrom
Conversation
- Add workspace document storage and store tests - Route terminal and command-palette actions through workspace surfaces - Update chat layout, sidebar, and thread terminal handling
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 4 issues found in the latest run.
- ✅ Fixed: Unused state variable
nowTickadded to ChatView- Removed the unused
nowTickandsetNowTickstate declaration from ChatView.tsx as it was never referenced anywhere in the component.
- Removed the unused
- ✅ Fixed:
WorkspaceRouteSynccomponent exported but never used- Deleted the entire WorkspaceRouteSync.tsx file since the component was never imported or rendered anywhere in the codebase.
- ✅ Fixed: Unused
terminalFocusRequestIdstate after refactor- Removed the unused
terminalFocusRequestIdandsetTerminalFocusRequestIdstate declaration from ChatView.tsx as all consumers were removed in a prior refactor.
- Removed the unused
- ✅ Fixed: Sidebar thread context menu navigates after split open
- Removed the redundant
router.navigate()calls afteropenThreadInSplit/openThreadInNewTabin the sidebar context menu handlers, which were causing the route component'suseEffectto callopenThreadSurfacewithfocus-or-tabdisposition and potentially refocus the surface into the wrong pane.
- Removed the redundant
Or push these changes by commenting:
@cursor push dc4ee0b932
Preview (dc4ee0b932)
diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx
--- a/apps/web/src/components/ChatView.tsx
+++ b/apps/web/src/components/ChatView.tsx
@@ -498,8 +498,6 @@
// When set, the thread-change reset effect will open the sidebar instead of closing it.
// Used by "Implement in a new thread" to carry the sidebar-open intent across navigation.
const planSidebarOpenOnNextThreadRef = useRef(false);
- const [terminalFocusRequestId, setTerminalFocusRequestId] = useState(0);
- const [nowTick, setNowTick] = useState(() => Date.now());
const [pullRequestDialogState, setPullRequestDialogState] =
useState<PullRequestDialogState | null>(null);
const [attachmentPreviewHandoffByMessageId, setAttachmentPreviewHandoffByMessageId] = useState<
diff --git a/apps/web/src/components/Sidebar.tsx b/apps/web/src/components/Sidebar.tsx
--- a/apps/web/src/components/Sidebar.tsx
+++ b/apps/web/src/components/Sidebar.tsx
@@ -1654,28 +1654,16 @@
if (clicked === "open-new-tab") {
openThreadInNewTab(serverThreadSurfaceInput(threadRef));
- void router.navigate({
- to: "/$environmentId/$threadId",
- params: buildThreadRouteParams(threadRef),
- });
return;
}
if (clicked === "open-split-right") {
openThreadInSplit(serverThreadSurfaceInput(threadRef), "x");
- void router.navigate({
- to: "/$environmentId/$threadId",
- params: buildThreadRouteParams(threadRef),
- });
return;
}
if (clicked === "open-split-down") {
openThreadInSplit(serverThreadSurfaceInput(threadRef), "y");
- void router.navigate({
- to: "/$environmentId/$threadId",
- params: buildThreadRouteParams(threadRef),
- });
return;
}
@@ -1729,7 +1717,6 @@
openThreadInNewTab,
openThreadInSplit,
project.cwd,
- router,
],
);
diff --git a/apps/web/src/components/workspace/WorkspaceRouteSync.tsx b/apps/web/src/components/workspace/WorkspaceRouteSync.tsx
deleted file mode 100644
--- a/apps/web/src/components/workspace/WorkspaceRouteSync.tsx
+++ /dev/null
@@ -1,130 +1,0 @@
-import { useLocation, useNavigate, useParams } from "@tanstack/react-router";
-import { useEffect, useMemo, useRef } from "react";
-
-import { useComposerDraftStore } from "../../composerDraftStore";
-import { resolveThreadRouteTarget } from "../../threadRoutes";
-import { useFocusedWorkspaceRouteTarget, useWorkspaceStore } from "../../workspace/store";
-import type { ThreadSurfaceInput } from "../../workspace/types";
-
-function sameRouteTarget(
- left: ReturnType<typeof resolveThreadRouteTarget>,
- right: ReturnType<typeof resolveThreadRouteTarget>,
-): boolean {
- if (!left && !right) {
- return true;
- }
- if (!left || !right || left.kind !== right.kind) {
- return false;
- }
- if (left.kind === "server" && right.kind === "server") {
- return (
- left.threadRef.environmentId === right.threadRef.environmentId &&
- left.threadRef.threadId === right.threadRef.threadId
- );
- }
- if (left.kind === "draft" && right.kind === "draft") {
- return left.draftId === right.draftId;
- }
- return false;
-}
-
-export function WorkspaceRouteSync() {
- const navigate = useNavigate();
- const openThreadSurface = useWorkspaceStore((state) => state.openThreadSurface);
- const currentRouteTarget = useParams({
- strict: false,
- select: (params) => resolveThreadRouteTarget(params),
- });
- const focusedRouteTarget = useFocusedWorkspaceRouteTarget();
- const pathname = useLocation({
- select: (location) => location.pathname,
- });
- const previousPathnameRef = useRef(pathname);
- const draftSession = useComposerDraftStore((store) =>
- currentRouteTarget?.kind === "draft" ? store.getDraftSession(currentRouteTarget.draftId) : null,
- );
- const currentRouteSurfaceInput = useMemo<ThreadSurfaceInput | null>(() => {
- if (!currentRouteTarget) {
- return null;
- }
- if (currentRouteTarget.kind === "server") {
- return {
- scope: "server",
- threadRef: currentRouteTarget.threadRef,
- };
- }
- if (!draftSession) {
- return null;
- }
- return {
- scope: "draft",
- draftId: currentRouteTarget.draftId,
- environmentId: draftSession.environmentId,
- threadId: draftSession.threadId,
- };
- }, [currentRouteTarget, draftSession]);
-
- useEffect(() => {
- const pathnameChanged = previousPathnameRef.current !== pathname;
- previousPathnameRef.current = pathname;
-
- if (currentRouteTarget) {
- if (!currentRouteSurfaceInput) {
- return;
- }
-
- if (!focusedRouteTarget || pathnameChanged) {
- openThreadSurface(currentRouteSurfaceInput, "focus-or-tab");
- return;
- }
-
- if (sameRouteTarget(currentRouteTarget, focusedRouteTarget)) {
- return;
- }
-
- void navigateToRouteTarget(navigate, focusedRouteTarget);
- return;
- }
-
- if (!focusedRouteTarget) {
- return;
- }
-
- void navigateToRouteTarget(navigate, focusedRouteTarget);
- }, [
- currentRouteSurfaceInput,
- currentRouteTarget,
- focusedRouteTarget,
- navigate,
- openThreadSurface,
- pathname,
- ]);
-
- return null;
-}
-
-function navigateToRouteTarget(
- navigate: ReturnType<typeof useNavigate>,
- target: NonNullable<ReturnType<typeof resolveThreadRouteTarget>>,
-) {
- if (target.kind === "server") {
- return navigate({
- to: "/$environmentId/$threadId",
- params: {
- environmentId: target.threadRef.environmentId,
- threadId: target.threadRef.threadId,
- },
- replace: true,
- search: {},
- });
- }
-
- return navigate({
- to: "/draft/$draftId",
- params: {
- draftId: target.draftId,
- },
- replace: true,
- search: {},
- });
-}
\ No newline at end of fileYou can send follow-ups to the cloud agent here.
| replace: true, | ||
| search: {}, | ||
| }); | ||
| } |
There was a problem hiding this comment.
WorkspaceRouteSync component exported but never used
Low Severity
WorkspaceRouteSync is a fully implemented component (130 lines) that is exported but never imported or rendered anywhere in the codebase. The route-to-workspace synchronization it would perform is instead duplicated across WorkspaceRouteFallback in WorkspaceShell.tsx and the individual route view components. This is dead code that adds maintenance burden.
Reviewed by Cursor Bugbot for commit d3d096c. Configure here.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| if (!activeThreadRef) { | ||
| return; | ||
| } | ||
| storeSetTerminalLaunchContext(activeThreadRef, { |
There was a problem hiding this comment.
🟢 Low components/ChatView.tsx:1390
When a draft thread's environment is changed via onEnvironmentChange, routeThreadRef (using route environmentId) diverges from activeThreadRef (using draftThread.environmentId). The terminal launch context is written and cleared using activeThreadRef (lines 1390, 1851, 1868) but read using routeThreadRef at line 531. This causes the clearing logic to target a different store key than the reading logic, leaving the launch context orphaned and never visible to the component that needs it.
- storeSetTerminalLaunchContext(activeThreadRef, {
+ storeSetTerminalLaunchContext(routeThreadRef, {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/components/ChatView.tsx around line 1390:
When a draft thread's environment is changed via `onEnvironmentChange`, `routeThreadRef` (using route `environmentId`) diverges from `activeThreadRef` (using `draftThread.environmentId`). The terminal launch context is written and cleared using `activeThreadRef` (lines 1390, 1851, 1868) but read using `routeThreadRef` at line 531. This causes the clearing logic to target a different store key than the reading logic, leaving the launch context orphaned and never visible to the component that needs it.
Evidence trail:
- apps/web/src/components/ChatView.tsx lines 403-406: routeThreadRef definition uses route environmentId
- apps/web/src/components/ChatView.tsx lines 572-575: activeThreadRef definition uses activeThread.environmentId
- apps/web/src/components/ChatView.tsx line 564: activeThread = localDraftThread for draft threads
- apps/web/src/components/ChatView.tsx lines 1239-1250: onEnvironmentChange calls setDraftThreadContext but does not navigate
- apps/web/src/composerDraftStore.ts lines 1919-1963: setDraftThreadContext updates draftThread.environmentId
- apps/web/src/components/ChatView.tsx line 1390: storeSetTerminalLaunchContext(activeThreadRef, ...)
- apps/web/src/components/ChatView.tsx line 532: reads from scopedThreadKey(routeThreadRef)
- apps/web/src/components/ChatView.tsx lines 1851-1853, 1868-1870: storeClearTerminalLaunchContext(activeThreadRef)
- apps/web/src/terminalStateStore.ts lines 264-266: terminalThreadKey calls scopedThreadKey
- packages/client-runtime/src/scoped.ts lines 20-22: scopedRefKey produces `{environmentId}:{localId}` keys
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
- add workspace pane focus, split, resize, and zoom commands - wire chat composer focus repair across workspace activation - enable dragging threads into workspace surfaces
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Workspace commands bypass command palette open check
- Moved the
useCommandPaletteStore.getState().openguard to run immediately after resolving the shortcut command, before both the workspace command dispatch and standalone terminal command handling.
- Moved the
Or push these changes by commenting:
@cursor push fc52f7f4cf
Preview (fc52f7f4cf)
diff --git a/apps/web/src/routes/_chat.tsx b/apps/web/src/routes/_chat.tsx
--- a/apps/web/src/routes/_chat.tsx
+++ b/apps/web/src/routes/_chat.tsx
@@ -42,6 +42,10 @@
terminalOpen,
},
});
+ if (useCommandPaletteStore.getState().open) {
+ return;
+ }
+
const isFocusedStandaloneTerminal = focusedWorkspaceSurface?.kind === "terminal";
if (command && isWorkspaceCommandId(command)) {
event.preventDefault();
@@ -73,10 +77,6 @@
}
}
- if (useCommandPaletteStore.getState().open) {
- return;
- }
-
if (event.key === "Escape" && selectedThreadKeysSize > 0) {
event.preventDefault();
clearSelection();You can send follow-ups to the cloud agent here.
| event.stopPropagation(); | ||
| void executeWorkspaceCommand(command); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Workspace commands bypass command palette open check
Medium Severity
In ChatRouteGlobalShortcuts, workspace commands (via isWorkspaceCommandId) are dispatched before the useCommandPaletteStore.getState().open guard on line 76. This means shortcuts like Mod+W (workspace.pane.close), Mod+[/Mod+] (focus previous/next), and resize/move commands will execute even while the command palette dialog is open. The palette check needs to come before the workspace command dispatch, consistent with how other commands are guarded.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5c8504d. Configure here.
| return { | ||
| document: { | ||
| ...document, | ||
| rootNodeId: nextTree.rootNodeId, | ||
| nodesById: nextTree.nodesById, | ||
| windowsById: nextWindowsById, | ||
| focusedWindowId: fallbackWindowId, | ||
| mobileActiveWindowId: fallbackWindowId, | ||
| }, | ||
| sourceWindowId: located.windowId, | ||
| surface, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟠 High workspace/store.ts:1010
When a window is removed because its last surface was detached, focusedWindowId and mobileActiveWindowId are unconditionally overwritten with fallbackWindowId. If the removed window was not the currently focused window, this incorrectly shifts focus away from the user's actual focused window.
return {
document: {
...document,
rootNodeId: nextTree.rootNodeId,
nodesById: nextTree.nodesById,
windowsById: nextWindowsById,
- focusedWindowId: fallbackWindowId,
- mobileActiveWindowId: fallbackWindowId,
+ focusedWindowId:
+ document.focusedWindowId === located.windowId
+ ? fallbackWindowId
+ : document.focusedWindowId,
+ mobileActiveWindowId:
+ document.mobileActiveWindowId === located.windowId
+ ? fallbackWindowId
+ : document.mobileActiveWindowId,
},
sourceWindowId: located.windowId,
surface,
};🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/workspace/store.ts around lines 1010-1022:
When a window is removed because its last surface was detached, `focusedWindowId` and `mobileActiveWindowId` are unconditionally overwritten with `fallbackWindowId`. If the removed window was not the currently focused window, this incorrectly shifts focus away from the user's actual focused window.
Evidence trail:
apps/web/src/workspace/store.ts lines 963-1022 (function `detachSurfaceFromWindow`), specifically lines 1005-1022 where `focusedWindowId: fallbackWindowId` and `mobileActiveWindowId: fallbackWindowId` are set unconditionally without checking if `located.windowId === document.focusedWindowId`. Compare with how the same pattern exists in `closeSurfaceById` at lines 917-924.
- Move thread terminal into an inline chat drawer - Add split/paper workspace layout switching and scrolling - Update route sync, command palette, and tests for new state
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Missing focus bump after closing terminal tab
- Added setTerminalFocusRequestId bump after storeCloseTerminal for non-final terminal tabs, restoring the focus behavior that was accidentally dropped during the refactor.
- ✅ Fixed: toggleOpen clears workspaceTarget when opening palette
- Refactored toggleOpen to compute
const next = !state.openand usenext ? {} : { workspaceTarget: null }, making the predicate structure identical to setOpen and eliminating the maintenance hazard.
- Refactored toggleOpen to compute
Or push these changes by commenting:
@cursor push c0aed107dc
Preview (c0aed107dc)
diff --git a/apps/web/src/commandPaletteStore.ts b/apps/web/src/commandPaletteStore.ts
--- a/apps/web/src/commandPaletteStore.ts
+++ b/apps/web/src/commandPaletteStore.ts
@@ -18,10 +18,10 @@
workspaceTarget: null,
setOpen: (open) => set({ open, ...(open ? {} : { workspaceTarget: null }) }),
toggleOpen: () =>
- set((state) => ({
- open: !state.open,
- ...(!state.open ? {} : { workspaceTarget: null }),
- })),
+ set((state) => {
+ const next = !state.open;
+ return { open: next, ...(next ? {} : { workspaceTarget: null }) };
+ }),
openWorkspaceTarget: (target) => set({ open: true, workspaceTarget: target }),
clearWorkspaceTarget: () => set({ workspaceTarget: null }),
}));
diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx
--- a/apps/web/src/components/ChatView.tsx
+++ b/apps/web/src/components/ChatView.tsx
@@ -1371,6 +1371,9 @@
if (activeThreadRef) {
storeCloseTerminal(activeThreadRef, terminalId);
}
+ if (!isFinalTerminal) {
+ setTerminalFocusRequestId((current) => current + 1);
+ }
},
[
activeThreadId,You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit b5281f9. Configure here.
| terminalState.terminalIds.length, | ||
| ], | ||
| ); | ||
| const handleAddTerminalContext = useCallback( |
There was a problem hiding this comment.
Missing focus bump after closing terminal tab
Medium Severity
The closeTerminal callback no longer calls setTerminalFocusRequestId after calling storeCloseTerminal. In the old code (both the PersistentThreadTerminalDrawer version and the inline ChatView version), this increment was present. Without it, after closing a non-final terminal tab, the remaining terminal won't receive keyboard focus — the user must manually click on it to interact.
Reviewed by Cursor Bugbot for commit b5281f9. Configure here.
| set((state) => ({ | ||
| open: !state.open, | ||
| ...(!state.open ? {} : { workspaceTarget: null }), | ||
| })), |
There was a problem hiding this comment.
toggleOpen clears workspaceTarget when opening palette
Low Severity
The toggleOpen logic uses ...(!state.open ? {} : { workspaceTarget: null }) which clears workspaceTarget when the palette is currently open (closing). But setOpen uses ...(open ? {} : { workspaceTarget: null }) — the condition reads naturally as "clear when closing." In toggleOpen, !state.open is the next open state, so the condition !state.open means "about to open," and the else branch (clearing) fires when "about to close." The logic is technically correct but the inverted predicate compared to setOpen makes it easy to misread and fragile to future changes.
Reviewed by Cursor Bugbot for commit b5281f9. Configure here.
1 participant
CleanShot.2026-04-13.at.19.42.25.mp4
CleanShot.2026-04-13.at.21.41.01.mp4
Summary
Testing
bun fmt,bun lint,bun typecheck, andbun run testbefore merge.Note
Medium Risk
Large UI/state refactor that changes how threads/terminals are mounted, focused, and persisted, plus new default keybindings (including moving
diff.toggle) that can break existing workflows or introduce focus/shortcut regressions.Overview
Refactors chat routing/rendering around a new workspace shell that owns a persisted
WorkspaceDocument(windows/nodes/surfaces, focus, zoom) and renders thread + terminal surfaces via shared workspace state instead of routes directly mountingChatView.Adds workspace-aware interactions: split/resize/equalize/move/focus/zoom pane commands (wired into global shortcuts and the command palette), sidebar thread drag-and-drop and context-menu “open in split” actions, and a standalone
ThreadTerminalSurfacefor terminals in workspace panes. Updates default keybindings to include workspace commands and movesdiff.toggleoffmod+d.Introduces browser persistence helpers for workspace documents (JSON localStorage with tests), adds
WorkspaceRouteSyncto keep URL and focused workspace surface aligned, and adjusts terminal/composer focus behavior (newactivationFocusRequestId, shared composer handle binding, and safer prompt/selection updates).Reviewed by Cursor Bugbot for commit b5281f9. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Refactor chat routes into a composable workspace shell with split-pane layout
ChatViewrendering with a centralWorkspaceShellmounted at the/_chatlayout level; thread and draft routes now open surfaces viaopenThreadSurfaceinstead of renderingChatViewdirectlyWorkspaceRouteSyncto keep the URL and focused workspace surface in sync, andThreadTerminalSurfacefor mounting terminal sessions inside workspace panesuseWorkspaceCommandExecutor;diff.togglemoves frommod+dtomod+alt+ddiff.toggleshortcut change is a breaking default keybinding change for existing usersMacroscope summarized b5281f9.