Add notch hover usage overlay - #3164
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs real behavior proof before merge. Reviewed August 30, 2026, 7:34 AM ET / 11:34 UTC. ClawSweeper reviewWhat this changesThe PR adds a disabled-by-default notch hover overlay for provider usage and optional agent sessions, with settings, shortcut behavior, localized text, tests, and docs. Merge readiness⛔ Blocked until stronger real behavior proof is added - 4 items remain Keep this PR open: the owner has explicitly reserved final review, and the available screenshots do not prove the repaired native click-through and interaction boundary on the current head. Priority: P2 Review scores
Verification
How this fits togetherCodexBar gathers provider usage and optional agent-session state, then renders it in menu-bar and settings surfaces. This PR adds a separate notch-triggered overlay that reads those existing stores and displays the summaries below a notched Mac display. flowchart LR
A[Provider usage store] --> C[Notch overlay controller]
B[Agent session store] --> C
D[Settings and shortcut] --> C
C --> E[Notch-sized hover trigger]
C --> F[Interactive overlay panel]
F --> G[Provider and session summaries]
Decision needed
Why: The owner has approved the direction but explicitly reserved final review of the native menu-bar interaction boundary. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the opt-in implementation, then approve it only after a signed current-head build demonstrates the full native interaction boundary on a notched Mac. Do we have a high-confidence way to reproduce the issue? Not applicable as a bug reproduction: this is a new feature PR. The remaining question is real native behavior on a notched Mac, which the supplied screenshots do not exercise on the current head. Is this the best way to solve the issue? Unclear: the split trigger/content-window design is a focused source-level solution and the feature direction is approved, but its essential native interaction claim still requires the owner-requested signed-build proof. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against efb952e0bf5f. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (29 earlier review cycles; latest 8 shown)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2f342ed97
ℹ️ 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".
d2f342e to
74e3b41
Compare
|
Both review findings are fixed in f3b6fec, and the branch is rebased onto current main (merge state is clean now):
|
|
Fixed in ab83688: all three bar factories (
|
steipete
left a comment
There was a problem hiding this comment.
Really nice work overall — the self-reported sizing approach and the no-global-monitor/no-AX design are exactly right, and the focused suites plus make check all pass on a merge with latest main. Four findings before this can land, two of them blocking:
P1 — Disabled feature still registers a global shortcut. NotchUsageOverlayController.swift:54 installs the KeyboardShortcuts handlers unconditionally, and the dependency registers any saved shortcut system-wide the moment a handler is attached. Disabling the overlay never unregisters it, and PreferencesNotchPane.swift:38 allows recording a shortcut while the feature is off. That breaks the off-by-default guarantee and can steal shortcuts from other apps. Handlers should only exist while the overlay is enabled.
P1 — Any settings change destroys an expanded overlay. The activation observation (NotchUsageOverlayController.swift:123) reads a computed property backed by the whole observable defaults struct, so unrelated preference changes re-fire it; at line 143 an existing panel is preserved only when collapsed — an expanded panel is closed and recreated, losing hover/shortcut state. Please narrow the observation to the fields that actually affect activation, and preserve an expanded panel across benign changes.
P2 — Controller restarts accumulate shortcut handlers. Handlers registered at line 71 are never removed on stop; each stop/start adds another callback, and in Toggle mode two callbacks make one press open-and-immediately-close the panel. The dependency ships KeyboardShortcuts.removeHandler(for:) for exactly this lifecycle.
P2 — Codex credits aren't actually a fallback. NotchUsageOverlayModel.swift:191 appends monthly credits whenever fewer than four bars exist, even when the extra window/spend bar was already chosen — a Codex account with two ordinary windows shows both, contradicting the documented mutually-exclusive fallback. Gate the append on the fallback slot being unfilled.
Happy to merge once these are addressed — the feature itself is a great addition.
4521c61 to
8fb9030
Compare
|
Addressed all four findings in 8fb9030 (rebased onto current main):
Verification on the rebased head:
@clawsweeper please re-review the current head. |
# Conflicts: # Sources/CodexBar/Resources/ar.lproj/Localizable.strings # Sources/CodexBar/Resources/ca.lproj/Localizable.strings # Sources/CodexBar/Resources/de.lproj/Localizable.strings # Sources/CodexBar/Resources/en.lproj/Localizable.strings # Sources/CodexBar/Resources/es.lproj/Localizable.strings # Sources/CodexBar/Resources/fa.lproj/Localizable.strings # Sources/CodexBar/Resources/fr.lproj/Localizable.strings # Sources/CodexBar/Resources/gl.lproj/Localizable.strings # Sources/CodexBar/Resources/id.lproj/Localizable.strings # Sources/CodexBar/Resources/it.lproj/Localizable.strings # Sources/CodexBar/Resources/ja.lproj/Localizable.strings # Sources/CodexBar/Resources/ko.lproj/Localizable.strings # Sources/CodexBar/Resources/nl.lproj/Localizable.strings # Sources/CodexBar/Resources/pl.lproj/Localizable.strings # Sources/CodexBar/Resources/pt-BR.lproj/Localizable.strings # Sources/CodexBar/Resources/ru.lproj/Localizable.strings # Sources/CodexBar/Resources/sv.lproj/Localizable.strings # Sources/CodexBar/Resources/th.lproj/Localizable.strings # Sources/CodexBar/Resources/tr.lproj/Localizable.strings # Sources/CodexBar/Resources/uk.lproj/Localizable.strings # Sources/CodexBar/Resources/vi.lproj/Localizable.strings # Sources/CodexBar/Resources/zh-Hans.lproj/Localizable.strings # Sources/CodexBar/Resources/zh-Hant.lproj/Localizable.strings
Separate the notch-sized hover trigger from the interactive content window. Keep shared hover ownership, disable mouse handling at collapse, and cancel both windows and all pending transitions on teardown. Add geometry and hover regressions, make SwiftUI helper isolation explicit, and sync current main. Co-authored-by: William Mitchell <wdmitchell.uk@gmail.com>
|
Thanks @wdmitchelluk — I checked the new merge commit The maintainer repair separates the notch-sized trigger from the interactive window below the menu-bar strip, handles hover across both windows, and disables mouse handling immediately on collapse. The changelog now records the feature only under Unreleased, with contributor credit preserved. Verification completed: 78 focused tests, the full 968-selection/81-group suite with zero failures/retries/timeouts, clean The merge-only push superseded the previous CI run, so I am following CI for the new head. I am holding further pushes while that run finishes. The remaining native hover/shortcut/scroll/neighboring-menu click-through proof is maintainer-side, and the PR remains open until it is complete. No additional contributor code change is requested here. |
# Conflicts: # CHANGELOG.md
Summary
Adds an opt-in notch usage overlay under Settings → Notch, off by default. It shows enabled provider tiles and an optional agent-session band, supports one to four columns, independently scrolling sections, reordering, and an optional Toggle/Hold shortcut. Agent sessions come from the existing store, not a second scanner.
The feature direction is approved in the owner review. The four original findings are addressed: shortcut registration follows enablement/start-stop, benign settings changes preserve expanded panels, restart does not accumulate handlers, and Codex credits occupy only an otherwise unused fallback slot.
Owner review hold: keep this feature PR open for Peter's review. Direction approval and passing CI do not authorize an unattended merge. Fresh sanitized before/after screenshots and native interaction proof are still pending.
Maintainer click-safety repair
Verification
swift test --jobs 4 --filter 'Notch|ProviderArchitectureGatekeeperTests': 78 tests in six suites passed, including new geometry and hover-state coverage and the existing owner-fix tests.make check: passed with zero violations across 2,063 Swift files.make test: passed — 968 selections across 81 groups, all first-pass successful; zero failures, retries, or timeouts (1,010.4 seconds). The final changelog-only follow-up leaves application and test sources identical to this tested tree.5317e595c31b6c6de9d1ee653d48098eae7bc7f4imports the already-landed fix: publish completed Codex cost history without rescanning #3279 changes without modifying notch source, settings, tests, or visuals. Its complete tree differs from the previously verified build, so the full-suite, whole-repository lint, signed-bundle, and packaging results above are previous-head evidence. CI for this updated head is separate. Before further local suite runs, incorporate the session-file isolation repair from fix: isolate provider session files during tests #3280. Fresh signed-build native interaction evidence and Peter's review remain required before any merge.Routine verification uses a clean environment with Keychain access suppressed, Codex-file isolation enabled, and live provider fetching disabled. No real provider/account request is required for the interaction repair. No release is part of this PR update.
Existing contributor visuals (before the click-safety repair)
These show the feature and settings, not proof of the repaired native click-through boundary.