Repository navigation
fix(web): bring design mode back in line with the fork's design system - #241
Conversation
- The screen-size menu's divider runs full width (-mx-1), per the full-width divider rule. - The panel's segment strips take Glass's selected-tab chip finish, like every other selected tab; the hairline gives way to the glass edge. - The layers rail header buttons are the ghost icon-xs Button: the 24px box and hover fill every icon-only control in the chrome carries. - The on-scale spacing badge lights in the design green rather than --primary, which is white in dark themes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
NoahHendrickson
left a comment
There was a problem hiding this comment.
Code review of 9758b20d87f295af11202ac34f9733aa9966c52e
One finding, low severity, posted inline. Everything else checks out:
- Screen-size divider:
MenuPopup's scroll wrapper isp-1, and the separator sits right inside it, so-mx-1runs it edge to edge. - Glass segment rule: it sits in the Full-only block next to the other selected-tab rules. It's unlayered, so
border-width: 0beats Tailwind'sborderutility.SegmentFieldhas no ring utilities, so leaving out the--tw-ring-*shadows drops nothing. The guard and the manifest entry match. - Spacing badge: it uses the same
--fork-design-accent/-bgpair asScreenSizeMenu,CanvasControlsand the other badges inDesignPanelFields. - Hide button:
data-fork-design-layers-hidesurvived the swap toButton, sofocusLayersControl(LAYERS_HIDE_BUTTON)still finds it.
Claude Opus 5.5 via Claude Code in T3 Code.
| variant="ghost" | ||
| size="icon-xs" | ||
| type="button" | ||
| title="Clear filter" |
There was a problem hiding this comment.
These three buttons still show native title tooltips. Button merges its props onto the <button> it renders, so title="Clear filter" / "Collapse all" / "Hide layers" reach the DOM unchanged. Removing the file's oxlint-disable only works because t3code/no-native-title-tooltip checks intrinsic elements (/^[a-z]/) and can't see through a component. So the PR description's line "They no longer use native title tooltips" isn't accurate.
It also breaks the pairing the PR is going for. The show-layers toggle these swap with (ForkPreviewLayersToggle.tsx) uses Tooltip > TooltipTrigger render={<Button variant="ghost" size="icon-xs" …/>} > TooltipPopup. So the rail's buttons now look like it, but their tooltip is the OS one. Either wrap each in the same Tooltip pattern and drop title (aria-label already names them), or put the disable comment back and fix the description.
There was a problem hiding this comment.
Fixed in 8f7fcbf. You're right: Button passed title through, so the OS tooltip was still there and the lint rule just couldn't see it. The three controls now go through a small RailIconButton helper: Tooltip > TooltipTrigger render={<Button variant="ghost" size="icon-xs" aria-label={label} …/>} > TooltipPopup, the same build as ForkPreviewLayersToggle. There's no title anywhere, and the description is corrected.
NoahHendrickson
left a comment
There was a problem hiding this comment.
Code-quality review of 9758b20d87f295af11202ac34f9733aa9966c52e.
The divider and spacing-badge fixes are fine. Two of the four changes need restructuring before this meets the bar:
- Design-panel segments under Glass. This adds a third copy of the selected-tab glass rule, plus a new marker, a manifest entry and guard assertions, all to dress a third hand-rolled segmented control.
SegmentFieldshould join the segmented-tab model that already exists, so this rule doesn't need to be added at all. The component's look is now also split across two files: the TSX declares the hairline and the theme sheet removes it. - Layers rail header buttons. The lint exemption was removed by getting around the rule, not by fixing what it flags.
Buttonpassestitlethrough to the native<button>, so all three controls still show native tooltips, andno-native-title-tooltipjust can't see them through the component. The PR description says they no longer use native title tooltips, which is wrong. They also don't match the show-layers toggle they're meant to pair with.
The file sizes are fine (the glass sheet is at 758 lines). Details are in the inline comments. This is not an approval.
| /* The design panel's segment strips are tabs on a field track too. Their | ||
| hairline goes: the glass edge replaces it, and a light drawn inside a | ||
| 1px border would sit a pixel in from the edge. */ | ||
| &.dark[data-fork-theme="cool-darker"] [data-fork-design-segment][aria-pressed="true"] { |
There was a problem hiding this comment.
This adds a third version of "selected tab is a glass chip". The box-shadow here is the same halo and glint pair as [data-fork-segmented-tab][data-pressed] just above it, and the comment calls these "tabs on a field track too". Each segmented control we write by hand now costs a marker, a rule, a manifest line and two guard assertions, and that keeps growing.
A simpler fix: build SegmentField on SegmentedTabs/SegmentedTab (the base-ui ToggleGroup/Toggle). Its mixed state becomes value={[]}, and selection comes from data-pressed instead of aria-pressed. Then the existing rule covers it, and this block, data-fork-design-segment, the manifest line and the new guard assertions can all go. SegmentedTab currently drops className, so it would need to accept the design panel's track and fill classes. That's a small change in a fork-owned file.
If that's too much for this PR, at least add this selector to the existing [data-fork-segmented-tab][data-pressed] rule so the halo and glint are declared once. As written, the next tweak to the tab glass has to be made twice.
There was a problem hiding this comment.
Addressed in 8f7fcbf, with no new rule. I didn't rebuild SegmentField on ToggleGroup/Toggle in this PR: that also changes keyboard behaviour (roving focus, arrow keys), which is outside a styling pass. Instead it joins the model at the marker level. Each segment stamps data-fork-segmented-tab, and the selected one adds data-pressed, so the existing [data-fork-segmented-tab][data-pressed] rule is the only place the tab glass is declared. The added block, the data-fork-design-segment marker and its guard assertions are gone. The guard now pins the stamp in DesignPanelLayoutControls.
| type="button" | ||
| title={option.title} | ||
| aria-pressed={selected ? "true" : "false"} | ||
| data-fork-design-segment |
There was a problem hiding this comment.
With this PR, this line is no longer the full story. The selected segment declares border border-border here, and theme.custom.glass-surfaces.css removes it with border-width: 0 under Glass. Anyone reading SegmentField (and its JSDoc, "with a hairline border") can't tell that the border goes away on one theme, or that the content box grows 2px when it does.
If SegmentField moves onto SegmentedTab (see the CSS comment), this conflict goes away. If it doesn't, keep the border decision in one place, for example a design token for the selected-segment border that the Glass palette sets to transparent, so the theme sheet isn't undoing a utility class from another file.
There was a problem hiding this comment.
Fixed in 8f7fcbf. The selected segment's hairline is now shadow-[inset_0_0_0_1px_var(--border)] instead of border border-border. Under Glass, the shared selected-tab rule replaces that shadow with the chip edge, so no theme sheet undoes a class from this file and the segment's box is the same in every theme. The JSDoc says so.
| variant="ghost" | ||
| size="icon-xs" | ||
| type="button" | ||
| title="Clear filter" |
There was a problem hiding this comment.
This doesn't actually remove the native tooltip. Button merges title onto the rendered <button>, so hovering still shows the OS tooltip. no-native-title-tooltip only checks intrinsic elements, so moving to <Button title> hides the problem from the lint rule without fixing it, and deleting the file's exemption looks like a fix when it isn't. The PR description ("They no longer use native title tooltips") is wrong as a result.
It's also inconsistent with the control this is meant to match. ForkPreviewLayersToggle wraps the same ghost icon-xs Button in Tooltip / TooltipTrigger render={...} / TooltipPopup, so the show and hide halves of the same toggle now behave differently on hover.
The three buttons also each repeat variant="ghost" size="icon-xs" type="button" title=X aria-label=X. type="button" is already Button's default, and the label is written twice per control. A small local helper removes all of that, and the explanatory JSX comment can go too:
function RailIconButton({ label, ...props }: { label: string } & ComponentProps<typeof Button>) {
return (
<Tooltip>
<TooltipTrigger render={<Button variant="ghost" size="icon-xs" aria-label={label} {...props} />} />
<TooltipPopup>{label}</TooltipPopup>
</Tooltip>
);
}Then the header is just <RailIconButton label="Clear filter" onClick={...}><XIcon /></RailIconButton> and so on. That passes the lint rule for real, and these controls match the show-layers toggle.
There was a problem hiding this comment.
Fixed in 8f7fcbf, with the helper you sketched: RailIconButton sets aria-label from label, wraps the button in the same Tooltip as the show-layers toggle, and drops the redundant type/title props and the JSX comment. The design-mode guard now pins label=\"Hide layers\" plus aria-label={label} instead of the old literal.
NoahHendrickson
left a comment
There was a problem hiding this comment.
Reviewed commit 9758b20d87f295af11202ac34f9733aa9966c52e against base e63be3e7f92f64f7e095131a6ec6b39442ea3dda, after fetching PR #241 and verifying the isolated worktree's checked-out HEAD.
No actionable findings introduced by this diff. Checked the layers controls against the shared Button implementation, the menu separator against popup padding, and the selected-segment and spacing-badge styles against the fork's theme tokens and Glass settings.
Validation: 169 focused design-mode/theme tests passed across 17 files; targeted lint completed with no errors (warnings on unchanged code); web typecheck passed. No live browser or desktop visual pass was performed.
Review only; no approval or code changes.
…rule Review follow-up on the design-mode alignment pass. - The rail's header buttons rendered native title tooltips through Button's prop passthrough, which the lint rule cannot see. They now use the Tooltip pattern of the show-layers toggle they pair with, through a small RailIconButton helper. - The panel's segment strip stamps SegmentedTab's marker and data-pressed, so the existing selected-tab Glass rule covers it. Its hairline is an inset ring that rule replaces, so no theme sheet undoes a border and the duplicate rule, marker and guard assertions are gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
…s room
- The engine re-sends the layers tree after `state {active: true}`, like
tokens and canvas already do. Its first emit lands before `state`, and
the host's enable reset wiped it, so the rail could stay missing until
the page next mutated. A browser test boots the real engine and checks
a layers message follows the state flip.
- While a visible preview tab has design mode on, it asks the right panel
for the room its chrome takes (panel + preview chrome row + layers rail
when shown). ChatView passes that to the panel width hook as a floor,
capped by the hook's own maximum. The user's dragged width is never
written, so turning design mode off restores it. At the default width
the 325px panel used to overflow the window and clip Send to chat.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
knip flags it as an unused export; only the panel and rail widths are read outside designPanelRoom.ts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in #240 (landing draft cleanup) and #241 (design mode fixes). Upstream moved the right panel's width hook from ChatView into ChatRightPanel, so design mode's panel-room floor now reaches it through a fenced minWidth prop; manifest watch entry and guard follow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The landing-draft header and a guard header were over four lines, the panel-room effect had no stated reason, and the boot browser test cleaned up in afterEach instead of a disposable. All four tripped the lint-owned ratchet once custom merged in. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Design mode's panel and layers rail had drifted from design-system changes made since the panel was built: a divider that didn't run full width, selected segments without Glass's chip finish, undersized icon buttons, and a spacing badge lit in the wrong color.
Changes
-mx-1), per the full-width divider rule in.fork/AGENTS.md.SegmentFieldstampsSegmentedTab'sdata-fork-segmented-tabmarker anddata-pressed, so the existing selected-tab rule covers it with no new CSS. Its hairline is now an inset ring, which that rule replaces, so the segment's box doesn't change.icon-xsButtonwrapped inTooltip, the same build as the show-layers toggle they swap with. A smallRailIconButtonhelper does this. They get the 24px box and hover fill that icon-only controls in the chrome carry, and real tooltips instead of nativetitleones.--primary, which is white in dark themes.Two bugs found while filming the feature
layersbeforestate {active: true}, and the host's enable reset wiped it. The engine now re-sends the tree afterstate, as it already did for tokens and canvas. A browser test boots the real engine and asserts a layers message follows the state flip; it fails without the fix.usePreviewPanelInlineSizeas a minimum (fenced), capped by the hook's existing maximum so the chat column keeps its floor. The width you dragged to is never written, so turning design mode off restores it. Each preview tab publishes under its own key, so a hidden tab can't clear the visible one's request.Verification
forkGlassSurfaces, the cool-darker theme and vibrancy guards: 189 passing.Reviewed with sol-loop: 1 round with GPT-6.1-Sol on high (scope: the layers-rail and panel-room fixes in d530d7d). Sol verdict: approved
Claude Opus 5.5 via Claude Code in T3 Code.
🤖 Generated with Claude Code