Repository navigation
Add configurable menu hover highlight color and opacity - #4412
cynicalight wants to merge 1 commit into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
clawsweeper
Bot
added
P3
|
Codex review: needs real behavior proof before merge. What this changesThis PR adds an optional menu highlight color picker and opacity slider for custom menu cards and the Refresh row. Example: Enable custom highlights with color #E8B078 and opacity 42%.
Review scores
ProductKind: Preference · Worth it: Needs a maintainer decision Merge readiness⛔ Blocked before merge - 4 items remain Keep this PR open under repository policy; the optional customization needs an owner decision and has a concrete text-contrast defect. Priority: P3 Decision needed
Before merge
Findings
Tests
Agent review detailsHow this fits togetherMenu preferences persist appearance values in UserDefaults, which the shared styling helper supplies to SwiftUI cards, GPU selection layers, and the custom Refresh row. flowchart TD
A[Menu preferences] --> B[UserDefaults]
B --> C[MenuHighlightStyle]
C --> D[SwiftUI menu cards]
C --> E[GPU selection rows]
C --> F[Refresh row]
Technical reviewBest possible solution: If approved, preserve native styling by default and choose readable foreground colors from the rendered custom fill across all three paths. Do we have a high-confidence way to reproduce the issue? Source establishes the contrast failure for an enabled #989898 highlight at 100% opacity; no runtime reproduction was performed. Is this the best way to solve the issue? The shared helper is a sensible implementation boundary, but custom styling needs owner approval and foreground selection must account for actual contrast and transparency. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 6e118bdb5782. Provenance checked
TestingProof path: unit tests only. SecurityNone. EvidenceWhat I checked:
Likely related people:
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 10, 2026, 11:53 AM ET / 15:53 UTC. |
![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: b65966f1c0
ℹ️ 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".
| static var selectionTextColor: NSColor { | ||
| guard let appearance = self.customAppearance() else { return .selectedMenuItemTextColor } | ||
| // A translucent fill leaves the menu background visible. Keep the normal label contrast there. | ||
| guard appearance.opacity >= 0.65 else { return .labelColor } |
There was a problem hiding this comment.
For a dark-mode user who chooses white at 60% opacity (an allowed slider value), this low-opacity branch returns dynamic .labelColor, which is also white, while the fill composites to a light gray; the same mistake occurs in reverse for dark fills in light mode, so highlighted card text can lose most of its contrast. Choose the text color after compositing the selected RGB/alpha with the effective menu background instead of using the 0.65 cutoff alone.
Useful? React with 👍 / 👎.
| "menu_highlight_section" = "تمييز القائمة"; | ||
| "menu_highlight_custom" = "استخدام تمييز مخصص"; | ||
| "menu_highlight_color" = "اللون"; | ||
| "menu_highlight_opacity" = "الشفافية"; |
There was a problem hiding this comment.
In the Arabic locale, الشفافية means transparency, while the bound value is customHighlightOpacity and is passed directly to alpha, so the displayed 0% is fully transparent and 100% is fully opaque—the reverse of the label. Arabic users therefore get a misleading slider; use an opacity/opaqueness term such as التعتيم rather than transparency.
Useful? React with 👍 / 👎.
|
Closing this PR after testing the local build. The customization did not change the Codex card's hover color as intended. The existing selected-row treatment also turns colored text and the Weekly left progress bar white, which makes the menu harder to read. I made this change too broad. I will replace it with a smaller opt-out setting that removes the menu card hover highlight without recoloring its contents. |
1 participant
Summary
Validation
swift build— passed.PATH=/opt/homebrew/bin:$PATH make check— passed.swift test --skip-build --filter MenuHighlightStyleTests— passed (3 tests).swift test --skip-build --filter MenuCardViewRecyclingTests— passed (28 tests).PATH=/opt/homebrew/bin:$PATH make test— failed inAbacusPluginTestsbecause two existing timeout-sensitive cases reported.timedOut; the runner retried the group and one case still timed out.UI
No screenshot was captured. The menu bar app was not launched against local accounts during validation.