Refactor chat into a composable workspace shell#2011
Refactor chat into a composable workspace shell#2011juliusmarminge wants to merge 2 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.
| // 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()); |
There was a problem hiding this comment.
Unused state variable nowTick added to ChatView
Low Severity
The state variable nowTick and its setter setNowTick are declared but never read or called anywhere in the component. This appears to be accidentally committed scaffolding for a feature that was never wired up, adding dead code to an already large component.
Reviewed by Cursor Bugbot for commit d3d096c. Configure 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.
| params: buildThreadRouteParams(threadRef), | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Sidebar thread context menu navigates after split open
Medium Severity
The "Open in split right/down" and "Open in new tab" context menu handlers call openThreadInSplit/openThreadInNewTab to create a new workspace pane, then immediately navigate the route to that thread. The navigation triggers the route view's useEffect which calls openThreadSurface with "focus-or-tab" disposition, potentially refocusing the surface into the original pane instead of the newly created split, undermining the user's intent to keep both panes visible.
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).
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.
Reviewed by Cursor Bugbot for commit 5c8504d. Configure 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.
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
High Risk
Large UI refactor that changes chat routing, focus/keyboard handling, and terminal rendering/persistence; regressions could affect navigation, shortcuts, and local workspace state across sessions.
Overview
Refactors the chat UI to run inside a new workspace shell that manages multi-pane split layout, tabbed surfaces, drag/drop placement, and route-to-focused-surface syncing instead of rendering
ChatViewdirectly from routes.Moves terminal UI from an in-thread drawer to a dedicated terminal surface (
ThreadTerminalSurface) controlled by workspace state, and updatesChatView/shortcuts to toggle and launch terminals via workspace APIs.Expands keybindings and command palette to support workspace-level commands (pane split/close/zoom, focus navigation, resize/move, terminal open variants), adjusts defaults (e.g.,
diff.toggletomod+alt+d), and adds sidebar actions/drag support to open threads in new tabs or splits.Adds localStorage persistence helpers and tests for saving/loading the workspace document, plus small focus/perf fixes in composer/editor interactions to behave correctly with pane activation.
Reviewed by Cursor Bugbot for commit 5c8504d. 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 and drag-and-drop support
ChatViewrendering with surface-based lifecycle management.ChatViewdirectly; instead they callopenThreadSurfaceto register surfaces in the workspace, and the newWorkspaceShellrenders all active surfaces.diff.togglemoves frommod+dtomod+alt+d.WorkspaceRouteSynckeeps the focused workspace surface and the router URL in sync bidirectionally.diff.togglekeybinding changes for all users; terminal drawer visibility is now controlled by workspace-level state rather than component-local state, which may affect existing terminal session behavior.Macroscope summarized 5c8504d.