feat(macapp): window chrome, sidebar type, and the duplicate action glyph - #1072
Merged
Conversation
…lyph Round 8 of the Codex look-and-feel gauntlet. Implements issue #1071. **The full-width titlebar band is gone.** A visible toolbarBackground paints one colour across the whole window, so the sidebar's grey ran over the content pane and the window wore a #222222 band from y=0 to y=52 — the content pane did not exist at that height. Hidden now, with each column painting its own colour through the top safe area, which is what the reference does. Verified by pixel probe on the running app rather than by reading the diff, because reading the diff is how round 7 called this closed while it was not. At y=5: x=100 → #222222, x=700 → #181818. Both previously returned #222222. **The nav pill no longer steals the selected state.** selectedRowSurface was painted on both the active nav row and the active conversation, putting two competing "this one is selected" affordances in the sidebar 31.5pt apart. The reference reserves that fill for the conversation alone; nav selection now reads through ink weight instead. **Sidebar type sits at body size**, as the reference's does — slightly larger than transcript body rather than 24% smaller. The section label goes the other way: larger but two rungs dimmer, since foregroundQuaternary is #747474, which is the reference's label ink exactly. It had been smaller *and* brighter, the opposite of a label that recedes. **Copy-message and copy-conversation had the same glyph.** Both were "doc.on.doc", so the action row showed one icon twice and read as a duplicated button. A critic found this two rounds ago and the fix never landed, so it is now pinned by a test that fails if the two ever match again. Not in this round, and deliberately: the sixteen properties already measured as matching. Two earlier rounds were partly spent re-fixing things that were already correct. Closes #1071. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPFeSNp49415WenKDF7gy9
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 877ce86913
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the way the rail does. Previously only the toolbar background | ||
| // covered this strip, in the *sidebar's* colour, so the pane began at | ||
| // y=52 and the window wore a full-width band above it. | ||
| .background(Theme.background.ignoresSafeArea(.container, edges: .top)) |
There was a problem hiding this comment.
This background extension exists only in ChatView, while AppShell now hides the toolbar background for the entire ProjectView. When the user switches to Activity, Sessions, Checkpoints, or Settings—or while startup/failure content is shown—those views do not ignore the top safe area, so the right-hand titlebar strip falls back to the window background instead of the pane surface. Apply the top-safe-area background around the shared content container rather than only Chat; the new module-wide token test currently passes without exercising any of these affected sections.
AGENTS.md reference: AGENTS.md:L31-L31
Useful? React with 👍 / 👎.
| // "doc.on.doc", so the two actions rendered identically and the | ||
| // row read as a duplicated button. This one copies the whole | ||
| // transcript, which is a page rather than a snippet. | ||
| Image(systemName: copied ? "checkmark" : "text.document") |
There was a problem hiding this comment.
text.document was introduced after macOS 14, but macapp/Package.swift:7 still declares macOS 14 as supported. On that supported OS, Image(systemName:) cannot resolve this name, leaving the whole-conversation copy toolbar action without its intended visible glyph. Use an older equivalent such as doc.text, or gate the new symbol with an availability fallback.
Useful? React with 👍 / 👎.
8 tasks
Round 9 of the Codex gauntlet. Implements issue #1073. **The ink ramp had four rungs where the reference has six**, and seven chrome elements had collapsed into a single 220–224 band as a result: with nothing between secondary and quaternary, everything rounded up to secondary. Added #8B8B8B (subtle) and #616161 (placeholder), and moved tertiary from 163 to 150 — a critic measuring both apps found 163 (#A3A3A3) is a value the reference never uses anywhere, and we were the only one with it. Three inversions fixed and each verified by probe on the running app: - folder icon 163 → **255** (the reference draws it white; it belongs to the title beside it) - overflow menu 221 → **139** (it had been brighter than the title's own icon) - nav label 255 → **222** (the reference reserves white for the active conversation, not for navigation) The overflow needed `.tint` rather than `.foregroundStyle`: `.borderlessButton` re-tints whatever label it is given, so styling the Image had no effect and styling the Menu with foregroundStyle had none either. Both looked correct in the source and neither changed a pixel, which is why each rung was measured rather than assumed. **Plan mode is a chip, not a checkbox.** The reference's composer uses icon+label chips and has no checkbox anywhere; a square AppKit control in a chat composer was the most out-of-family element on the screen. ComposerChip is shared rather than styled inline, so the next composer toggle inherits it. **Composer type and padding**: the field set no font at all, so its placeholder rendered a step under the reference's. It takes the body rung now, the control labels match it, and the vertical padding grows to suit — the padding had been compressed to fit type that was too small. Left alone deliberately: the 1px bright rim at the very top of the window. It is a single raw pixel, correct by y=1pt, and I could not attribute it to anything this app draws — reporting it as unexplained rather than guessing at a cause. Closes #1073. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPFeSNp49415WenKDF7gy9
Closed
9 tasks
…chips Round 10. Implements issue #1074, and corrects two claims round 9 got wrong. **The composer border round 9 reported did not exist.** Its stroke was 43 against a 45 fill — darker than what it bordered, and therefore indistinguishable from the antialiased edge of the rounded rect underneath. I measured a colour transition and called it a border without checking which direction it went. Theme.rule is 60, lighter than the fill, and the probe now reads 24 → 60 → 45 down the composer edge, matching the reference's 32 → 60 → 45. The test pins the *direction*, not just the value. **"The reference never uses #A3A3A3" was false.** It uses 1,198px of it, mostly in the transcript. Near-absent from reference chrome, so removing it there was defensible; asserting it as a fact was not. **Round 8's nav change was an over-correction.** Removing the fill to stop it competing with the active conversation left nothing indicating the current section — the row scanned uniformly #222222. Restored, and the principle round 9 stated alongside it was also wrong: the reference does not reserve white for conversations, spending it on the header title, account name and active controls too. The reference is a compartmented app and this had no rules at all. Three added, each measured lighter than the surface it borders: main-pane header (#3C3C3C at y=103), sidebar footer (#393939), composer (#3C3C3C). Sidebar 220 → 310pt. It had been 63% of the reference's width, failing on both the absolute and the proportional measure. The two composer chips had a 48% icon-width spread and an 85% label-gap spread, because Label sizes its symbol from font metrics and the two labels differ. Both now build their icon and gap from tokens, so adjacent chips cannot drift. Verified on the running app: composer rule 60-on-45, header rule at y=103, selected row filled, sidebar divider at x=310. Closes #1074. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPFeSNp49415WenKDF7gy9
8 tasks
…r header Round 11. Implements part of issue #1075; two items are honestly not closed. **Fixed two regressions round 10 introduced.** Round 10 restored the nav fill and produced a sidebar with *two* identical #333333 bands — the nav row and the conversation row, same fill, height and inset, nothing distinguishing current from adjacent. The reference has exactly one in its whole sidebar. The nav row is *current*, not *selected*; it carries that in ink weight now and leaves the fill to the conversation list. Probe on the running app: zero fills at the nav row's column, where there had been one competing with the conversation's. Round 10 also set the header title to primary and the overflow menu followed it back to #FFFFFF, re-inverting a hierarchy round 9 had fixed. The tint is reapplied on the Menu, where .borderlessButton actually reads it. **Every rule in the app was twice the reference's weight** — composer border, header rule, footer rule and column divider all measured 2 raw px against the reference's 1, four for four. Spacing.hairline is 0.5 now, which is the single pixel a 2x display draws. The column divider was also the one rule whose colour was wrong, because it was a system Divider rather than a token: 47 where the reference draws 67. Both verified on the running app — divider now 67 at 0.5pt. **Corner radii were ~40% short on the two smallest surfaces.** An edge-inset profile put the reference's user bubble at r≈18 against ours at 10, and its selected row at r≈10 against ours at 6, on identical 36pt rows. **The header is shorter but not right.** 103pt → 91.5pt against the reference's 55pt. Two causes were found and one fixed: the header row was bound to ConversationColumn, so the title truncated at the column's edge while 402pt of its own row — 38% of it — sat empty to the right. The header spans the pane now and only the transcript is column-bound. The remaining 36pt is the titlebar band above it. Moving the title into the toolbar was tried and reverted: `.navigation` placement puts it in the *sidebar* pane and pushes the app name into the content pane, which is worse than the problem. Closing this properly needs the window to adopt a hidden titlebar so content starts at y=0, which is a larger change than this round. Not attempted this round, and still open from #1075: transcript vertical rhythm, the tool-activity label's second type size, and the absent accent channel in the sidebar. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPFeSNp49415WenKDF7gy9
…lbar Tried .windowStyle(.hiddenTitleBar) to let the conversation header sit on the traffic-light row, which is where the reference puts it. It removed the app wordmark, which was the intended half. It also moved the toolbar items into the *sidebar's* top-left corner beside the traffic lights, where the reference has them at the top right of the content pane — and the header rule stayed at 91.5pt regardless, so the actual gap did not close. Reverted rather than left in: one intended improvement, one clear regression, and none of the measurement that motivated it. The header remains 91.5pt against the reference's 55pt, recorded as open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPFeSNp49415WenKDF7gy9
Round 12. The previous critic's summary was that the chrome is now materially correct and what separates the two apps is a greyscale ladder pointed at the wrong roles, type rungs running short, and rhythm. This addresses those. **Ink roles.** Six of thirteen sampled roles sat on the wrong rung. Fixed the three that are ours to fix here: sidebar nav label 255 → 222 (the reference reserves white for content, not navigation); transcript action icons 116 → 139 (quaternary is the section-label rung and sat a step below); tool-activity label 150 → 163, separated from body by colour rather than by scale. **Type rungs.** The caption rung was the cause of two short roles rather than their call sites: measured ascender-to-descender, the composer placeholder ran 12.0pt against the reference's 15.0 and the sidebar section header 10.0 against 14.5. Raised 14 → 16, which moves both. The tool-activity label moves to body size. The reference sets it 0.5pt below its own body ascender and separates it by colour; ours sat 2.5pt below, de-scaled *and* dimmed where the reference only dims. **Transcript rhythm was wrong in both directions.** The reference opens tight — 38.5pt from the header rule to the first bubble — then leaves 62pt before the tool row. Ours opened at 65.5 and then crammed the tool row 19.5pt below the bubble, 3.2x tight. Both now come from tokens rather than a shared `large`. **Composer padding was doubled.** The send button sat 18.5pt from the right edge and 20pt from the bottom against the reference's 9 and 9.5 — twice as far in, inside a composer 14pt shorter overall. **One correction to round 11.** The 0.5pt rule convention was over-applied. The reference draws four rules at 0.5pt and the tool-activity separator at 1.0pt; round 11 made all five half-weight. That fifth rule has its own token now. Not addressed, and the largest remaining: the app has zero chromatic pixels anywhere, where the reference spends 952 saturated samples on identity, status and diff counts. Also open: no right rail, no sidebar header row, and a sidebar carrying one nav item against the reference's five. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPFeSNp49415WenKDF7gy9
…leanup Round 13. Half of the change the last critic named as most important. **The app's identity moved into the sidebar**, which is where the reference puts it: a bright header row with a disclosure chevron and a search glyph. Ours had been a recessed system window title measuring 89 against the reference's 222 — 60% dimmer, in the wrong pane, and costing a whole chrome band purely to repeat the app's name. The window title is empty now. Probed on the running app: identity header 222, nav label 222, both exact. **The action row was not a set.** Four icons measured 19.5 / 14.5 / 12.0 / 12.5pt — a 62% spread — because different SF Symbols draw to different heights at one point size. Their ink was 139 / 116 / 116 / 116 because the two copy buttons overrode the row's colour and the other two inherited the section-label rung. One size and one ink are set on the row now, and the copy buttons override only for their copied state. **Chat rows carried two marks the reference's do not**: a bullet peaking at 255 — the brightest ink anywhere in the sidebar — and a leading speech-bubble glyph, together creating a third indent level. Both removed; the reference's chat rows are text-only and align with its project rows. Nav icon 18 → 15pt: measured ink width had been 21.0 against the reference's 15.0. Footer's fifth icon matched to its four neighbours, having rendered 60% brighter, which read as a state rather than as the same kind of control. **Not closed, and it is the other half of that one change.** The conversation title still sits on its own row, so the header rule stays at 91.5pt against the reference's 55. Removing the wordmark band did not collapse it — the title's row is separate from the window-control row, and the two structural approaches tried so far both made things worse: `.navigation` toolbar placement puts the title in the *sidebar* pane, and `.windowStyle(.hiddenTitleBar)` moves the toolbar items into the sidebar's corner. Recorded as open rather than attempted a third time blind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPFeSNp49415WenKDF7gy9
The change three previous attempts failed at, and the reason they failed. The header rule sat at 91.5pt against the reference's 55: a 52pt system titlebar stacked above a 40pt header band. Hiding the titlebar was tried twice and reverted twice, because a window toolbar *requires* that band — hide it and the toolbar's items are re-homed into the sidebar's corner, which is worse than the problem. The missing step was that the toolbar did not need to exist. Its two controls belong to the conversation, so the header now owns them, there is no toolbar left to displace, and the titlebar can go. One band, measured 67.5pt on the running app. The app's identity moved into the sidebar in the previous commit, which is what made this possible: with the wordmark gone from the titlebar, nothing was left up there worth a band. **Still wrong, and measured rather than glossed:** the header's controls sit beside the title instead of at the far right. The Spacer between them is not expanding, so the row sizes to its content — the Group wrapper appears to be constraining it. The height gain is real and independent of this; the alignment is not fixed. Also open: 67.5 is not 55. The remaining 12.5pt is the header band's own padding, which is worth measuring against the reference's internal spacing before changing, rather than trimmed to hit a number. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPFeSNp49415WenKDF7gy9
A `.borderlessButton` menu expands to fill whatever width it is offered, so the overflow menu was swallowing the Spacer and leaving the header's controls clustered beside the title. `.fixedSize()` on the menu — the same fix the other copy of this code already carried — lets the Spacer do its job. The Group wrapper went with it; it was doing nothing once the header stopped being bound to the transcript's column. Verified on the running app: title at the left, controls at the right edge, one header band. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPFeSNp49415WenKDF7gy9
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Round 8 of the Codex look-and-feel gauntlet. Closes #1071.
The full-width titlebar band is gone
A visible
toolbarBackgroundpaints one colour across the whole window, so the sidebar’s grey ran over the content pane and the window wore a#222222band from y=0 to y=52 — the content pane did not exist at that height. Hidden now, with each column painting its own colour through the top safe area.Verified by pixel probe on the running app, not by reading the diff — reading the diff is how round 7 called this closed while it was not:
#222222#222222#222222#222222#181818#181818The nav pill no longer steals the selected state
selectedRowSurfacewas painted on both the active nav row and the active conversation — two competing "this one is selected" affordances 31.5pt apart. The reference reserves that fill for the conversation alone; nav selection now reads through ink weight.Sidebar type sits at body size
The reference sets its sidebar very slightly larger than transcript body. Ours was 24% smaller, which read as a footnote rather than as the list the sidebar exists to be.
The section label goes the other way — larger but two rungs dimmer.
foregroundQuaternaryis already#747474, which is the reference’s label ink exactly. It had been smaller and brighter: the opposite of a label that recedes.A real bug: two actions, one glyph
ChatView.swift:214and:247both useddoc.on.doc, so copy-message and copy-conversation rendered identically and read as a duplicated button. A critic found this two rounds ago and the fix never landed. Now pinned by a test that fails if the two ever match again.Not in this round, deliberately
The sixteen properties already measured as matching — composer max width (883.0 vs 882.5pt), body cap height, all four surface colours, bubble geometry, and the rest. Two earlier rounds were partly spent re-fixing things that were already correct, which is why the issue carries a do-not-touch list.
183 Swift tests pass; build and
swift format lint --strictclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01UPFeSNp49415WenKDF7gy9