fix: follow system dimming for menu bar icon, standard size and stepper-tuned vertical offset - #2882
fix: follow system dimming for menu bar icon, standard size and stepper-tuned vertical offset#2882luantu wants to merge 4 commits into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![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: b878015e5d
ℹ️ 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".
| let leadingIcon: NSImage? = if layout.lines.first?.first == .icon, icon != nil { | ||
| icon |
There was a problem hiding this comment.
When “Improve visibility on inactive displays” is enabled and the layout starts with an icon, this branch still extracts the icon into button.image. AppKit consequently dims the icon on inactive displays—the behavior this preference is meant to avoid—while only the text receives the high-contrast labelColor. Keep the icon as an attributed attachment, or otherwise bypass native dimming, when options.highContrast is true.
Useful? React with 👍 / 👎.
| if leadingIcon != nil, lineIndex == 0, tokenIndex == 0, token == .icon { | ||
| continue |
There was a problem hiding this comment.
When the editor previews any layout beginning with an icon, including the icon presets, this skips the icon in attributedTitle and exposes it only through leadingIcon; however, MenuBarLayoutPreviewText renders only attributedTitle. The preview therefore omits the brand icon, and an icon-only layout appears blank. Update the preview to display leadingIcon as well, or retain the attachment for preview rendering.
Useful? React with 👍 / 👎.
| if leadingIcon != nil, lineIndex == 0, tokenIndex == 0, token == .icon { | ||
| continue |
There was a problem hiding this comment.
When a valid leading icon is used, this continue runs before renderItem can append its provider-icon accessibility text. For an icon-only layout, accessibilityLabel consequently becomes empty and StatusItemController installs that empty value as the button's accessibility title, leaving VoiceOver users without a label. Preserve the icon's accessibility text even though its visual content is moved to button.image.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs real behavior proof before merge. Reviewed August 12, 2026, 4:54 AM ET / 08:54 UTC. ClawSweeper reviewWhat this changesThe PR moves a leading menu-bar provider icon into AppKit’s template-image slot, adds a vertical adjustment setting, and updates rendering and layout tests. Regression provenancePossible regression — suspected (reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked until real behavior proof is added - 9 items remain This PR is still needed, but its surfaced-icon path regresses the existing high-contrast option, editor preview, and icon-only accessibility. It also needs real after-fix menu-bar evidence before merge. Priority: P2 Review scores
Verification
How this fits togetherCodexBar renders provider usage into an AppKit status-bar button through a configurable layout. Layout tokens become attributed text and/or a provider image, then the status item exposes that result to macOS and accessibility services. flowchart LR
A[Provider usage data] --> B[Configurable menu layout]
B --> C[Layout renderer]
C --> D[Attributed title and icon]
D --> E[AppKit status-bar button]
E --> F[macOS display tinting]
E --> G[Accessibility label]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Preserve the attributed-icon path when high contrast is enabled, render the surfaced image in preview, retain icon accessibility text, and provide redacted real macOS menu-bar proof from the freshly built bundle. Do we have a high-confidence way to reproduce the issue? Yes from source: a leading icon enters the proposed surfaced-image path, while the current preview and high-contrast behavior rely on the attributed title; icon-only output also loses the current icon accessibility text. Is this the best way to solve the issue? No: moving the icon to the native image slot is appropriate only if the high-contrast, preview, and accessibility branches preserve their current contracts. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against a90dfed5c264. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
1 participant
Summary
Two related menu bar refinements for the custom layout title, plus a follow-up that makes the vertical nudge editable with a stepper.
1. Icon follows the system's active/inactive display tinting (no more wrong gray-on-secondary-display)
Before, a leading
.icontoken was baked into the attributed title as anNSTextAttachment(a pre-rendered bitmap). AppKit dims menu bar items on inactive displays automatically, but it only does that for template images assigned toNSStatusBarButton.image— pre-rendered attachments stay bright/gray regardless. On dual-screen setups this made CodexBar's icon look inconsistent with every other menu bar extra.Now the leading icon token is surfaced as
button.image(a template image) so AppKit applies the same active/inactive display tinting as native menu bar icons, while text tokens keep rendering through the attributed title.2. Standard size + consistent optical centering
3. Stepper-tuned vertical offset
The existing "vertical adjustment" preference (already added as a numeric input in the layout editor) is now a
TextField+Stepperpair (range -20…20, default 0, ±1 per click), matching the pattern already used elsewhere in Preferences (e.g. cost history days). The label was shortened from "Vertical Adjustment" to "Vertical" and the whole row aligns with the adjacent Size/Gap pickers.Why this matters on dual-screen and custom-resolution displays
button.imageit now dims exactly like native icons when focus moves to the other display.Tests
MenuBarLayoutRendererTestsextended: leading-icon-as-image behavior, single-line baseline offset, vertical-adjustment baseline shift, stale (refresh-failed) title dimming, andleadingIconsize/template checks.swift test --filter MenuBarLayoutRendererTests(18 tests, all pass) plus the MenuBarLayout/StatusItem/SettingsStore suites (369 tests pass). Lint-clean (swiftlint/swiftformat); the 42 repo-wide lint violations are pre-existing on upstream.Commits
feat: make menu bar vertical adjustment a numeric inputfix: follow system dimming for menu bar icon and use standard sizerefactor: vertical adjustment uses stepper with visible value