feat(web): unify workspace navigation - #7153
Conversation
|
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
vouch:trusted
github-actions
Bot
added
the
size:XL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
There was a problem hiding this comment.
UI Consistency
Reviewed the changed web UI surfaces (shared WorkspacePageHeader/WorkspacePageContainer extraction, sidebar row/footer rework, settings layout widths, toggle/segmented variants, context-menu fallback icons, composer chip metrics, themed toolbar variable swap).
Verified as safe:
WorkspacePageHeaderreproduces the previous per-surface geometry:--workspace-topbar-heightresolves to52pxand toenv(titlebar-area-height)under.wco,drag-regionis still gated on Electron,COLLAPSED_SIDEBAR_TITLEBAR_INSET_CLASSis retained,data-chat-headerstill forwards (so the themed toolbar rules keep applying), andclassNameis merged last so call sites keep ownership. PassingreserveNativeControlsin the browser is inert becausewco:only matches Electron's.wcoroot.--toolbar-control-foregroundis defined in both the default token block and thehtml[data-theme-id]bridge, so theindex.cssswap does not drop a variable.SettingsPageContainerkeeps the fade/padding pairing:[--topbar-scroll-fade-height:1.5rem] sm:[...]is required to neutralise the utility's@variant smbump, andpt-6matches the 1.5rem band, same as the pull-requests page.separatorBeforeis honoured by both renderers ofContextMenuItem(Electron template +showContextMenuFallback), and the newcopysubmenu parent id is never dispatched.
Three consistency findings below; no visual or behavioural regression was identified.
Posted via Macroscope — UI Consistency
| function SegmentedTab({ | ||
| selected, | ||
| density = "default", | ||
| className, | ||
| ...props | ||
| }: { | ||
| selected: boolean; | ||
| density?: "default" | "compact"; | ||
| } & Omit<ComponentProps<typeof Toggle>, "aria-pressed" | "pressed" | "size" | "type" | "variant">) { | ||
| return ( | ||
| <Toggle | ||
| type="button" | ||
| pressed={selected} | ||
| size={density === "compact" ? "segmented-compact" : "segmented"} | ||
| variant="segmented" | ||
| className={className} | ||
| {...props} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| export { SegmentedTab, SegmentedTabList }; |
There was a problem hiding this comment.
SegmentedTab/SegmentedTabList have no call sites anywhere in the repo, and the segmented / segmented-compact toggle size+variant added in toggle.tsx are referenced only from this file. A shared primitive landed ahead of its first consumer can't have its contract validated against real usage (density naming, role="group" rather than tab semantics, pressed/hover tones), and it becomes another variant future call sites may or may not pick up.
Suggest landing this together with the screen that needs it, or dropping it from this PR.
Posted via Macroscope — UI Consistency
ApprovabilityVerdict: Needs human review This PR introduces behavioral changes to thread creation (project scoping) and new shared UI infrastructure. The sidebar logic change affects when threads are created directly vs through the picker. Six open review comments flag incomplete migrations and unused code paths that should be addressed. You can customize Macroscope's approvability policy. Learn more. |
93043b7 to
f181757
Compare
![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 using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f181757. Configure here.
There was a problem hiding this comment.
Reviewed the changed web UI surfaces for shared-primitive, Tailwind-ownership and layout-preservation consistency. The header/container consolidation (WorkspacePageHeader/WorkspacePageContainer) preserves topbar height, WCO inset, drag region, safe-area padding and the collapsed-sidebar transition, the new micro button size correctly extends the primitive contract instead of hand-rolling the sidebar row actions, and the themed toolbar-control rule change is consistent with the existing [data-chat-header] control rule. Two smaller items below.
Posted via Macroscope — UI Consistency
f181757 to
27bb7f4
Compare
Dismissing prior approval to re-evaluate 27bb7f4
There was a problem hiding this comment.
UI consistency review found three items in the changed web UI scope. Details inline.
Posted via Macroscope — UI Consistency
| rightPanelAvailable: boolean; | ||
| rightPanelOpen: boolean; | ||
| rightPanelShortcutLabel: string | null; | ||
| rightPanelUnavailableLabel?: string; |
There was a problem hiding this comment.
rightPanelUnavailableLabel has no call site — the only references in the repo are this declaration, the default on line 30, and the read on line 99. A prop added to a shared control ahead of its first consumer can't have its contract validated (who overrides it, and with what copy), and it widens the component API without changing any current behavior.
Suggest landing this together with the surface that needs the alternate label, or keeping the literal until then.
Posted via Macroscope — UI Consistency
e20c680 to
bc08174
Compare
There was a problem hiding this comment.
Two consistency findings on the changed lines. Details inline.
Posted via Macroscope — UI Consistency
|
|
||
| export const COMPOSER_INLINE_SKILL_CHIP_CLASS_NAME = | ||
| "inline-flex max-w-full select-none items-center gap-[0.33em] rounded-[0.5em] border border-fuchsia-500/25 bg-fuchsia-500/12 px-[0.5em] py-[0.08em] font-medium text-[0.86em] leading-[1.1] text-fuchsia-700 align-middle dark:text-fuchsia-300"; | ||
| "inline-flex h-[1.41em] max-w-full select-none items-center gap-[0.33em] rounded-[0.5em] border border-fuchsia-500/25 bg-fuchsia-500/12 px-[0.5em] font-medium text-[0.86em] leading-none text-fuchsia-700 align-middle dark:text-fuchsia-300"; |
There was a problem hiding this comment.
The skill chip repeats the base chip's geometry verbatim — inline-flex h-[1.41em] max-w-full items-center gap-[0.33em] rounded-[0.5em] px-[0.5em] font-medium leading-none align-middle — so the new fixed h-[1.41em]/leading-none contract that replaced the removed py+top-[0.15em] offset now has two owners. Tuning one (the height is what keeps glyphs centered at the composer's user-configurable prompt size) silently misaligns the other, and these strings are raw-concatenated rather than merged through cn, so a call site cannot correct it.
Suggest hoisting the shared geometry into one constant (e.g. INLINE_CHIP_GEOMETRY_CLASS_NAME) that both INLINE_CHIP_CLASS_NAME and the skill chip build on, leaving only the fuchsia border/background/text tone here. Related: COMPOSER_INLINE_SKILL_CHIP_LABEL_CLASS_NAME (line 22) is now an exact alias of COMPOSER_INLINE_CHIP_LABEL_CLASS_NAME; pointing its call sites at the base constant would drop the dead indirection.
Posted via Macroscope — UI Consistency
bc08174 to
8f1ea22
Compare
There was a problem hiding this comment.
Reviewed the UI changes for shared-component and Tailwind-ownership consistency. Two findings, both about the new shared contracts introduced here; the WorkspacePageHeader extraction, the Button size="micro"/icon-micro migration of the sidebar row actions, and the themed --toolbar-control-foreground alignment otherwise look correct and preserve focus-visible, coarse hit targets, drag regions and titlebar insets.
Posted via Macroscope — UI Consistency
| /** Shared top-bar geometry for every full-width workspace surface. */ | ||
| export function WorkspacePageHeader({ |
There was a problem hiding this comment.
The doc comment claims this owns the top bar for "every full-width workspace surface", but apps/web/src/components/usage/UsagePage.tsx:107-131 still hand-rolls the exact {!isElectron && <header>} / {isElectron && <div>} twin this PR deletes from settings.tsx and ProjectSettingsPanel.tsx, with the pre-refactor strings (px-3 sm:px-5, h-[52px], wco:pr-[calc(100vw-env(titlebar-area-width)-env(titlebar-area-x)+1em)]).
After this change the migrated surfaces use safe-area padding, --workspace-topbar-height, --workspace-native-controls-inset and gap-3, while Usage keeps the literal values — so its top bar sits at a different inset/height from every sibling surface, which is the drift the shared header exists to prevent.
Suggest migrating that header here too (<WorkspacePageHeader electron={isElectron}> around the single breadcrumb), or narrowing the doc comment to the surfaces actually covered. apps/web/src/routes/_chat.pull-requests.tsx:1841-1850 repeats the same geometry and could take reserveNativeControls={!rightPanelOpen}.
Posted via Macroscope — UI Consistency
8f1ea22 to
d8a25c9
Compare
There was a problem hiding this comment.
Two icon-consistency findings in the changed context-menu code. Everything else in this revision looks consistent: the new WorkspacePageHeader reproduces the Electron/WCO geometry it replaces (--workspace-topbar-height resolves to env(titlebar-area-height, 52px) under .wco, and wco: selectors outrank the new sm: padding on specificity), micro now shares icon-micro's radius/icon contract, the sidebar hover actions all run through Button variant="ghost-muted", the fallback divider matches MenuSeparator, and every SettingsPageContainer caller now uses the width prop instead of a call-site max-w-*.
Posted via Macroscope — UI Consistency
d8a25c9 to
904f059
Compare
904f059 to
ba1df53
Compare
There was a problem hiding this comment.
One finding on the new fallback context-menu divider: it is the only renderer of the separatorBefore sections on web, and it currently has no coverage — the module's own test double cannot execute the new branch. Everything else in this revision (micro/ghost-muted migrations in the sidebar row, the shared WorkspacePageHeader/WorkspacePageContainer geometry, the --toolbar-control-foreground switch, and the fallback icon/chevron sizing) matched the shared primitives and theme contracts on review.
Posted via Macroscope — UI Consistency
| "max-height:min(24rem,70vh);min-width:0;max-width:24rem;overflow-x:hidden;overflow-y:auto;padding:0.25rem;"; | ||
|
|
||
| for (const item of entries) { | ||
| if (item.separatorBefore === true && inner.childElementCount > 0) { |
There was a problem hiding this comment.
This divider is invisible to this module's own harness: FakeElement in contextMenuFallback.test.ts implements neither childElementCount (so the guard evaluates undefined > 0 and no divider is ever appended) nor setAttribute (so line 289 would throw the moment a test passes separatorBefore) — the same test-double gap that forced the createElementNS guard added just above. Net effect: the web fallback's section grouping ships untested, while the Electron template got two dedicated cases for the same contract.
Suggest using the array-compatible children.length here, then adding setAttribute to FakeElement plus a case asserting a divider lands between sections (a separatorBefore assertion in threadActionMenu.logic.test.ts would cover the item side cheaply too).
| if (item.separatorBefore === true && inner.childElementCount > 0) { | |
| if (item.separatorBefore === true && inner.children.length > 0) { |
Posted via Macroscope — UI Consistency
ba1df53 to
883bf98
Compare
There was a problem hiding this comment.
UI consistency review: one finding — a shared component's API is widened with a prop that has no consumer in this PR. Everything else in the changed web UI looks consistent with the shared component system (the fallback menu divider now matches MenuSeparator, the fallback icon sizes match MenuItem's responsive contract, the sidebar lifecycle actions are on Button size="icon-micro"|"micro" variant="ghost-muted", micro shares icon-micro's radius, and WorkspacePageHeader/WorkspacePageContainer preserve the WCO inset, safe-area padding, drag region and collapsed-sidebar inset of the headers they replace).
Posted via Macroscope — UI Consistency
| export function WorkspaceBreadcrumbSeparator({ className }: { readonly className?: string }) { | ||
| return ( | ||
| <li aria-hidden="true" className="flex shrink-0 items-center text-icon-muted"> | ||
| <li aria-hidden="true" className={cn("flex shrink-0 items-center text-icon-muted", className)}> |
There was a problem hiding this comment.
className has no call site: all five WorkspaceBreadcrumbSeparator usages (ChatHeader.tsx:261, SettingsBreadcrumb.tsx:26, ProjectSettingsPanel.tsx:215, _chat.pull-requests.tsx:1861) render it bare. A pass-through added to a shared breadcrumb part ahead of its first consumer can't have its contract validated — which class is expected to be layered on, and whether it should be allowed to override the separator's shrink-0/text-icon-muted tone — and it widens the API without changing behavior.
Suggest landing this with the surface that needs the override.
| export function WorkspaceBreadcrumbSeparator({ className }: { readonly className?: string }) { | |
| return ( | |
| <li aria-hidden="true" className="flex shrink-0 items-center text-icon-muted"> | |
| <li aria-hidden="true" className={cn("flex shrink-0 items-center text-icon-muted", className)}> | |
| export function WorkspaceBreadcrumbSeparator() { | |
| return ( | |
| <li aria-hidden="true" className="flex shrink-0 items-center text-icon-muted"> |
Posted via Macroscope — UI Consistency
883bf98 to
a323f96
Compare
There was a problem hiding this comment.
One finding: the new shared page frame lands with a single consumer while a near-identical literal frame remains in the pull-requests route.
Posted via Macroscope — UI Consistency
| return ( | ||
| <div | ||
| className={cn( | ||
| "mx-auto flex w-full flex-col gap-6 px-5 pt-6 pb-12 sm:px-6", |
There was a problem hiding this comment.
WorkspacePageContainer ships with one consumer (SettingsPageContainer), while apps/web/src/routes/_chat.pull-requests.tsx:1923 still hand-rolls the same frame verbatim — mx-auto flex w-full max-w-4xl flex-col gap-4 px-5 pt-6 pb-12, inside the identical topbar-scroll-fade … [--topbar-scroll-fade-height:1.5rem] sm:[--topbar-scroll-fade-height:1.5rem] scroller and carrying the same "top padding is the fade band's own height" comment this container's pt-6 now encodes.
Two owners for one geometry drift immediately: this container adds sm:px-6, so above sm the settings frame insets 1.5rem while the pull-requests frame stays at 1.25rem, and the next tweak to the shared frame reaches only one of the two pages.
Suggest migrating that call site in this PR so the frame has a single owner:
- <div className="mx-auto flex w-full max-w-4xl flex-col gap-4 px-5 pt-6 pb-12">
+ <WorkspacePageContainer className="gap-4">Posted via Macroscope — UI Consistency
Keep Theo's AccountLimits hover card on the Usage button while adopting Maria's SidebarUtilityMenu refactor. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
What changed
Screenshots
Direct parent on the left; this PR on the right. Same viewport and copied application state.
Settings layout
Thread navigation and context menu
These shared primitives and navigation changes were mixed into a much larger UI refresh. This is the dependency-free base layer so the following Usage, Pull Requests, terminal, composer, and tool-activity changes can be reviewed on their own.
Validation
Stack order
Built with GPT-5.6-sol in the Codex harness.
Note
Medium Risk
Wide UI/navigation surface (headers, sidebar creation, menus) with user-visible behavior changes; copy actions are nested but IDs are unchanged. Low security risk.
Overview
Introduces
WorkspacePageHeaderandWorkspacePageContaineras shared top-bar and content-width primitives, replacing duplicated header markup on chat, empty-thread, settings, project settings, and onboarding routes. Settings pages pick up configurable widths (readable/wide/expanded) through the same container.Sidebar navigation is consolidated via
SidebarUtilityMenu: settings uses the same footer utility row as the main sidebar (Back on any/settingsroute, with history when available), and scoped-project New thread creates directly in that project without opening the picker.Thread context menus gain
separatorBeforeon the contract and in Electron/web renderers, icons on more actions, and a Copy submenu (path / branch / thread ID). Sidebar row lifecycle controls move to sharedButtonsizes (micro/icon-micro).Minor themed-toolbar icon color tweak for panel layout toggles.
Reviewed by Cursor Bugbot for commit a323f96. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Unify workspace navigation header and sidebar footer across all views
WorkspacePageContainerandWorkspacePageHeaderin WorkspacePageContainer.tsx as shared layout primitives, replacing ad-hoc header markup across chat, settings, and onboarding views.SidebarUtilityMenucomponent that appears on all settings routes and uses browser history for back navigation.separatorBeforesupport to theContextMenuItemIPC contract, propagating through the web fallback renderer and Electron native menu builder.Copyparent in the thread action menu, with icons added to most menu items.SettingsPageContainergains awidthprop (readable|wide|expanded) backed byWorkspacePageContainer.Macroscope summarized a323f96.