DO MERGE: ui fixes - #6658
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
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 4 blocking correctness issues found. This PR introduces behavioral changes to thread creation with project scoping and IPC contract modifications, not just styling fixes. Multiple Medium severity findings about incorrect thread environment selection and other bugs require resolution before approval. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
UI consistency review found three items, all on lines this PR adds. Two are pointer-affordance regressions in the new terminal sidebar tabs; one is a shared-primitive choice in the pull request action slot.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Two consistency findings in the changed web UI. The earlier notes (terminal tab cursor-pointer, conflict action as a destructive-outline Button) are addressed.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
UI Consistency: 1 finding
The agent-spawn CTA in the chat timeline was rebuilt as a card that copies ChangedFilesCard's shell, but at the geometry this same PR moved that card away from — so the two cards now render side by side in the same timeline column with different radii and padding. Details inline.
Everything else reviewed (shared WorkspacePageHeader/WorkspacePageContainer extraction, the new SegmentedTab/segmented toggle variant, Button migrations in the sidebar, terminal drawer toolbar, and PR filter menu, and the new live-activity-focus mask utilities in index.css) keeps the primitive contracts and ownership rules intact. Earlier findings on the check-out split button, --control-icon-color on toggles, the terminal tab cursors, and the conflicts action have all been addressed.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Three consistency findings on newly added controls. Everything flagged in earlier runs (terminal tab cursors, the hand-built check-out split button, the interactive conflict Badge, the agent-spawn card geometry, the inert --control-icon-color on the panel toggles) is resolved in this revision.
Posted via Macroscope — UI Consistency
maria-rcks
changed the title
4e73f84 to
03fa569
Compare
| useLayoutEffect(() => { | ||
| if (hoverIndex !== null) positionTooltip(); | ||
| }, [hoverIndex, positionTooltip]); |
There was a problem hiding this comment.
🟡 Medium usage/UsageProviderChart.tsx:286
The tooltip remains positioned using stale pixel coordinates when the plot or tooltip resizes while the pointer is stationary, so --usage-tooltip-left and --usage-tooltip-top can place it clipped or outside the resized chart. positionTooltip only runs on hover changes or mousemove; observe the plot and tooltip with ResizeObserver and reposition on resize.
- useLayoutEffect(() => {
- if (hoverIndex !== null) positionTooltip();
- }, [hoverIndex, positionTooltip]);
+ useLayoutEffect(() => {
+ if (hoverIndex === null) return;
+ positionTooltip();
+ const plot = plotRef.current;
+ const tooltip = tooltipRef.current;
+ if (plot === null || tooltip === null) return;
+ const observer = new ResizeObserver(positionTooltip);
+ observer.observe(plot);
+ observer.observe(tooltip);
+ return () => observer.disconnect();
+ }, [hoverIndex, positionTooltip]);🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/usage/UsageProviderChart.tsx around lines 286-288:
The tooltip remains positioned using stale pixel coordinates when the plot or tooltip resizes while the pointer is stationary, so `--usage-tooltip-left` and `--usage-tooltip-top` can place it clipped or outside the resized chart. `positionTooltip` only runs on hover changes or `mousemove`; observe the plot and tooltip with `ResizeObserver` and reposition on resize.
03fa569 to
de48ddd
Compare
| (entry, index) => | ||
| index > latestUserMessageIndex && timelineEntryTurnId(entry) === unsettledTurnId, |
There was a problem hiding this comment.
🟡 Medium chat/MessagesTimeline.logic.ts:643
An active-turn proposed-plan is rendered above the shared Working header when it precedes the first entry whose timelineEntryTurnId matches unsettledTurnId, so the timeline order is incorrect. firstOwnedAfterUser only recognizes turn IDs, but proposed-plan entries have no turn ID; include proposed-plan entries when locating the active-turn start.
| (entry, index) => | |
| index > latestUserMessageIndex && timelineEntryTurnId(entry) === unsettledTurnId, | |
| (entry, index) => | |
| index > latestUserMessageIndex && | |
| (entry.kind === "proposed-plan" || timelineEntryTurnId(entry) === unsettledTurnId), |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/chat/MessagesTimeline.logic.ts around lines 643-644:
An active-turn `proposed-plan` is rendered above the shared Working header when it precedes the first entry whose `timelineEntryTurnId` matches `unsettledTurnId`, so the timeline order is incorrect. `firstOwnedAfterUser` only recognizes turn IDs, but `proposed-plan` entries have no turn ID; include `proposed-plan` entries when locating the active-turn start.
| }; | ||
|
|
||
| function commandProgramName(command: string): string | null { | ||
| const tokens = command.trim().split(/\s+/); |
There was a problem hiding this comment.
🟡 Medium chat/MessagesTimeline.tsx:2180
commandProgramName labels "/Applications/My Tool/bin/tool" --flag as Running My instead of Running tool, so live activity displays the wrong executable whenever its path contains spaces. Tokenize quoted arguments before extracting the basename.
| const tokens = command.trim().split(/\s+/); | |
| const tokens = command.trim().match(/"[^"]*"|'[^']*'|\S+/g) ?? []; |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 2180:
`commandProgramName` labels `"/Applications/My Tool/bin/tool" --flag` as `Running My` instead of `Running tool`, so live activity displays the wrong executable whenever its path contains spaces. Tokenize quoted arguments before extracting the basename.
There was a problem hiding this comment.
Reviewed the changed web UI for shared-primitive and styling consistency. The earlier findings on this PR (terminal toolbar radius, terminal tab cursors, resolve-conflicts button primitive, repository link cursor, settle/snooze hover parity, agent-spawn card geometry, inert --control-icon-color on toggles) all look addressed at this head. Two remaining items below, both about affordances that changed lines removed.
Also worth a look (not commented inline): summarizeChangedFileScopes and the workingStepLabel prop chain (ChatView → MessagesTimeline context) no longer have a consumer after this refresh.
Posted via Macroscope — UI Consistency
| "flex flex-col rounded-md px-0.5 transition-colors", | ||
| isExpandedToolGroupEntry ? "py-0" : "py-0.5", | ||
| canExpand && | ||
| "cursor-pointer hover:bg-accent/20 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-inset focus-visible:ring-ring/70", | ||
| "cursor-pointer focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-inset focus-visible:ring-ring/70", |
There was a problem hiding this comment.
This row is still click-to-expand (role="button", aria-expanded, cursor-pointer), but this change drops its hover surface in the same commit that removes its chevron/status column, so an expandable tool row now has no affordance beyond the cursor — and transition-colors on the line above is left with nothing to transition. The sibling WorkGroupToggleTimelineRow keeps hover:bg-accent/20 for the same gesture in the same list, so the two expandable rows now respond differently to hover.
Suggest keeping the hover surface here:
| "flex flex-col rounded-md px-0.5 transition-colors", | |
| isExpandedToolGroupEntry ? "py-0" : "py-0.5", | |
| canExpand && | |
| "cursor-pointer hover:bg-accent/20 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-inset focus-visible:ring-ring/70", | |
| "cursor-pointer focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-inset focus-visible:ring-ring/70", | |
| "flex flex-col rounded-md px-0.5 transition-colors", | |
| isExpandedToolGroupEntry ? "py-0" : "py-0.5", | |
| canExpand && | |
| "cursor-pointer hover:bg-accent/20 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-inset focus-visible:ring-ring/70", |
Posted via Macroscope — UI Consistency
| <div key={provider} className="flex flex-col gap-1"> | ||
| <div className="flex items-baseline justify-between gap-4"> | ||
| <span className="flex min-w-0 items-center gap-2 text-sm text-foreground"> | ||
| <ProviderMark provider={provider} className="size-4" /> |
There was a problem hiding this comment.
UsageProviderChart still encodes each provider as a distinct series color (fill/stroke from PROVIDER_PRESENTATION[provider].color), but this refresh removes both keys to it: the per-provider share bar that painted that color here, and UsageChartLegend from the chart header. The hover tooltip lists providers with monochrome brand marks only, so nothing on the page maps a colored line back to a provider.
Smallest fix is to carry the series color on this row, which is now the only per-provider list beside the chart:
| <ProviderMark provider={provider} className="size-4" /> | |
| <span | |
| aria-hidden | |
| className="size-1.5 shrink-0 rounded-full" | |
| style={{ backgroundColor: PROVIDER_PRESENTATION[provider].color }} | |
| /> | |
| <ProviderMark provider={provider} className="size-4" /> |
Posted via Macroscope — UI Consistency
de48ddd to
b53b569
Compare
github-actions
Bot
added
size:XL
|
Superseded by #7153, which preserves this PR's workspace/navigation diff as the bottom layer of the review stack. The remaining categories are split into #7147, #7148, #7149, #7150, #7151, and #7152. Closing this fork-head copy because GitHub cannot add fork pull requests to native stacks; its discussion remains here for reference. |
| if (isMobile) setOpenMobile(false); | ||
| if (scopedProjectGroup) { | ||
| void newThreadContext.handleNewThread( | ||
| scopeProjectRef(scopedProjectGroup.environmentId, scopedProjectGroup.id), |
There was a problem hiding this comment.
🟡 Medium components/Sidebar.tsx:3312
When a scoped logical project has members in multiple environments, this branch always creates the thread with scopedProjectGroup.environmentId and scopedProjectGroup.id, so a thread or draft opened in another member environment is silently created in the representative environment instead of the current context. Resolve the contextual member project ref (as the command-palette path does) before falling back to the group's representative ref.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/Sidebar.tsx around line 3312:
When a scoped logical project has members in multiple environments, this branch always creates the thread with `scopedProjectGroup.environmentId` and `scopedProjectGroup.id`, so a thread or draft opened in another member environment is silently created in the representative environment instead of the current context. Resolve the contextual member project ref (as the command-palette path does) before falling back to the group's representative ref.
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 b53b569. Configure here.
| void newThreadContext.handleNewThread( | ||
| scopeProjectRef(scopedProjectGroup.environmentId, scopedProjectGroup.id), | ||
| ); | ||
| return; |
There was a problem hiding this comment.
Scoped new thread ignores context
Medium Severity
When the sidebar is scoped to a project group, New thread always creates against the group representative via scopedProjectGroup.environmentId / id. That skips the contextual member selection used elsewhere (buildSidebarProjectPickerEntries / startNewThreadFromContext), so in mixed local/remote groups a thread opened on a non-representative member can spawn the new draft in the wrong environment.
Reviewed by Cursor Bugbot for commit b53b569. Configure here.
There was a problem hiding this comment.
UI consistency review found five issues, all in the newly added shared contracts: two new abstractions land without any call site, the new Toggle segmented sizes lose their font size at sm and up, and two smaller ownership/redundancy items. Details inline.
Posted via Macroscope — UI Consistency
| { | ||
| id: "archive", | ||
| label: "Archive thread", | ||
| icon: "folder", |
There was a problem hiding this comment.
archive now resolves to the same folder glyph as the new copy-path child (line 115), so the web fallback menu shows one icon for both Archive thread and Copy > Path in the same popup. Lucide's archive isn't in contextMenuFallback.ts's ICON_PATHS yet, so the fix is an entry there plus icon: "archive" here — the app already uses ArchiveIcon for this action in the settings nav.
Posted via Macroscope — UI Consistency
| render={ | ||
| <Toggle | ||
| className="shrink-0 [-webkit-app-region:no-drag]" | ||
| className="shrink-0 text-foreground [-webkit-app-region:no-drag]" |
There was a problem hiding this comment.
text-foreground restates what the primitive already owns: toggleVariants' ghost variant is border-transparent text-foreground …, and when a theme is active index.css (html[data-theme-id] [data-panel-layout-controls] [data-slot="toggle"], … [data-slot="tooltip-trigger"]) sets color at higher specificity than a utility class. So the added class changes nothing on any of the three toggles in this file, in either theme mode.
Same for rightPanelUnavailableLabel (lines 15 and 30): no caller passes it and the default is the literal it replaced. Suggest dropping both and letting the variant plus the themed rule keep owning the tone.
Posted via Macroscope — UI Consistency
| ); | ||
| } | ||
|
|
||
| function SegmentedTab({ |
There was a problem hiding this comment.
Nothing at this head imports SegmentedTab/SegmentedTabList (nor the segmented / segmented-compact toggle sizes added for them), while the hand-rolled segmented groups this primitive mirrors are untouched — usage/UsagePage.tsx still builds them from raw buttons (flex rounded-md border border-border with per-button first:rounded-s-*, bg-muted text-foreground / text-muted-foreground hover:text-foreground) at both the window picker and the cost/tokens toggle.
A shared control with no consumer can't be validated against a real surface and leaves two competing segmented looks in the tree. Suggest migrating at least one of those groups here, or landing the primitive together with its first call site.
Posted via Macroscope — UI Consistency
| } | ||
|
|
||
| /** Keeps an icon glyph on the content edge while its larger hit target extends outward. */ | ||
| export function WorkspacePageHeaderEdgeControl({ |
There was a problem hiding this comment.
WorkspacePageHeaderEdgeControl is exported but never rendered at this head, so the -me-[7px] nudge it encodes has no call site that pins it to a real header edge. Suggest dropping it until a header needs the alignment (or adding that consumer in this PR) so the magic offset stays verifiable.
Posted via Macroscope — UI Consistency
| segmented: | ||
| "h-6 min-w-0 rounded-md px-2.5 text-xs before:rounded-[calc(var(--radius-md)-1px)]", | ||
| "segmented-compact": | ||
| "h-5 min-w-0 rounded-md px-2 text-[11px] before:rounded-[calc(var(--radius-md)-1px)]", |
There was a problem hiding this comment.
The base toggle class carries text-base sm:text-sm. tailwind-merge only drops the unprefixed text-base when a size adds text-xs, so sm:text-sm survives and wins from 40rem up — both new sizes render 14px text on desktop and segmented-compact loses its text-[11px] distinction entirely (only the 20px vs 24px height would differ). Button's xs size spells out both breakpoints (text-sm sm:text-xs) for exactly this reason.
| segmented: | |
| "h-6 min-w-0 rounded-md px-2.5 text-xs before:rounded-[calc(var(--radius-md)-1px)]", | |
| "segmented-compact": | |
| "h-5 min-w-0 rounded-md px-2 text-[11px] before:rounded-[calc(var(--radius-md)-1px)]", | |
| segmented: | |
| "h-6 min-w-0 rounded-md px-2.5 text-xs sm:text-xs before:rounded-[calc(var(--radius-md)-1px)]", | |
| "segmented-compact": | |
| "h-5 min-w-0 rounded-md px-2 text-[11px] sm:text-[11px] before:rounded-[calc(var(--radius-md)-1px)]", |
Posted via Macroscope — UI Consistency
1 participant
Changes
Workspace, navigation, and Settings
Usage
Pull Requests
Terminal
Timeline, tool calls, and changed files
Validation
Built with GPT-5.6 Sol via Codex in T3 Code.
Note
Low Risk
Mostly UI layout and menu structure; scoped new-thread behavior is a small product-logic change with tests updated. No auth, data, or security surface in this diff.
Overview
Introduces
WorkspacePageHeaderandWorkspacePageContainerso chat, empty-thread, onboarding, settings, and project settings share one top-bar geometry (safe area, collapsed-sidebar inset, Electron drag region, native control clearance) and optional readable/wide/expanded content widths. Settings pages adopt the container widths; panel layout toggles get themed foreground styling and a customizable disabled tooltip.Context menus gain a contract-level
separatorBeforeflag, wired through Electron native menus (with deduped separators) and the web fallback, which also adds submenu chevrons and a broader icon set. Thread actions are reorganized: icons on lifecycle items, path/branch/thread ID grouped under Copy, and section separators before rename, copy, and archive.Sidebar behavior updates: New thread creates directly in the active project scope (no picker repeat); row pin/status/snooze/settle controls use consistent micro buttons and icon sizing; settings footer navigation is centralized in
SidebarUtilityMenu(Back on utility pages, shared with the main sidebar chrome).Adds
SegmentedTab/ segmented toggle variants for shared tab styling, tightens composer inline chip vertical alignment, and adjusts theme library card min width for three-column layouts.Reviewed by Cursor Bugbot for commit b53b569. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix UI consistency across sidebar, context menus, settings, and workspace headers
WorkspacePageHeaderandWorkspacePageContainercomponents to standardize titlebar layout and content width across chat, settings, and onboarding viewsseparatorBeforesupport toContextMenuItemcontracts, Electron menus, and the web fallback context menuSegmentedTabandSegmentedTabListUI components and newsegmentedtoggle variants for segmented control stylingSidebarUtilityMenu; back navigation now uses browser history when availableMacroscope summarized b53b569.