Fix provider sidebar trailing control spacing#1183
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed May 27, 2026, 8:40 PM ET / 00:40 UTC. Summary Reproducibility: not applicable. this is a visual PR review rather than a bug report with a failing reproduction path. Source inspection confirms current main and v0.30.0 do not already contain the trailing inset. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance: Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge Security Review detailsBest possible solution: Keep the narrow padding change if rebuilt Settings > Providers visual proof confirms the checkbox spacing improves without making the row look cramped or misaligned. Do we have a high-confidence way to reproduce the issue? Not applicable; this is a visual PR review rather than a bug report with a failing reproduction path. Source inspection confirms current main and v0.30.0 do not already contain the trailing inset. Is this the best way to solve the issue? Likely yes; a small trailing inset on the existing row container is a narrow maintainable fix for checkbox breathing room. The missing piece is visual proof from the rebuilt UI, not a code repair. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 63d9373b733f. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 028b004609
ℹ️ 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".
| if !self.isMergedMenuOpen { | ||
| self.applyIcon(phase: nil) | ||
| } |
There was a problem hiding this comment.
Suppress the deferred icon render too
When a number-key provider shortcut is used with the merged menu open, switcherView.handleKeyboardSelection(at:) immediately runs the switcher's onSelect, which calls deferSwitcherMenuRebuildIfStillVisible; that task yields once, confirms the same menu is still in openMenus, and then rebuildOpenMenuIfStillVisible calls applyIcon(phase: nil) while the menu is still tracked. This new guard therefore only skips the extra direct render on line 117, but the status item can still change width one run-loop later in the exact keyboard-selection scenario this change is trying to prevent; the added test also checks before yielding so it misses this path.
Useful? React with 👍 / 👎.
028b004 to
58d2a3c
Compare
Summary
Adds a small trailing inset to provider sidebar rows so the enabled checkbox has enough breathing room from the scroll/list boundary and adjacent detail pane.
This is a layout-only fix for Settings > Providers and does not change provider logic, quota parsing, billing, or data behavior.
Validation
swift build— passed./Scripts/lint.sh lint— blocked by an unrelated SwiftFormat warning inTests/CodexBarTests/StatusMenuSwitcherClickTests.swift(redundantAsync)