Skip to content

Fix provider switcher shortcuts - #1260

Closed
ProspectOre wants to merge 1 commit into
steipete:mainfrom
ProspectOre:codex/provider-switcher-key-equivalents
Closed

Fix provider switcher shortcuts#1260
ProspectOre wants to merge 1 commit into
steipete:mainfrom
ProspectOre:codex/provider-switcher-key-equivalents

Conversation

@ProspectOre

@ProspectOre ProspectOre commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Route provider switcher command-number shortcuts through StatusItemMenu instead of a custom event-tracking monitor.

Context

Part of the UI hang/lag cleanup set: this removes a custom event-tracking shortcut monitor and uses the normal menu key-equivalent path instead.

Validation

  • swift test --filter 'StatusMenuSwitcherClickTests|StatusMenuPersistentRefreshTests|StatusItemControllerShutdownTests'
  • make check
  • git diff --check

Runtime Proof

Built and launched this PR branch from 7d3e97f3990a3a60e8cafacaf6a4b77f2dd0b800 with the PR-built app isolated as the only running CodexBar status-item app.

Redacted live state monitor while the visible menu was operated manually:

23:25:55 PDT selected=codex overview=0
23:26:31 PDT selected=claude overview=0   # after Cmd-3
23:26:35 PDT selected=claude overview=1   # after Cmd-1
23:26:42 PDT selected=cursor overview=0   # extra command-number check

No screenshots or raw AX menu dumps were posted because the menu can contain private account/cost data.

@clawsweeper

clawsweeper Bot commented Jun 1, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 1, 2026, 4:15 AM ET / 08:15 UTC.

Summary
The PR routes Command-number provider switcher shortcuts through StatusItemMenu.performKeyEquivalent, adds a provider-selection delegate hook, removes the custom CFRunLoop event-tracking monitor, and updates shortcut/menu tests.

Reproducibility: no. high-confidence before-fix reproduction was run in this review. The source path and merged related reports show the current shortcut implementation, and the PR body provides after-fix live output from the built app.

Review metrics: 3 noteworthy metrics.

  • Diff size: 9 files, +36/-204. Most of the patch is deletion, so review should focus on whether the replacement shortcut delivery path preserves behavior.
  • Shortcut monitor removal: 1 CFRunLoop monitor removed. The removed monitor is the current fallback mechanism for provider switcher shortcut delivery in event-tracking mode.
  • Reported validation: 3 commands plus live output. The PR body reports focused shortcut tests, make check, git diff --check, and redacted runtime proof for the changed behavior.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Clear draft/CI state and have a maintainer accept the AppKit shortcut-path swap before merge.

Risk before merge

  • [P1] Merging removes the existing CFRunLoop event-tracking monitor, so existing provider switcher shortcuts depend on AppKit delivering Command-number key equivalents through NSMenu in event-tracking mode.
  • [P1] The PR is still draft and GitHub reports mergeable_state unstable, so maintainer/CI readiness is not fully settled even though no narrow code repair is apparent.

Maintainer options:

  1. Accept the new menu path (recommended)
    Maintainers can accept the redacted live-output proof and merge after normal draft and CI gates clear.
  2. Ask for visual desktop proof
    If stronger AppKit confidence is needed, ask for a short redacted recording showing the visible provider switcher responding to Command-number shortcuts without exposing account or cost data.
  3. Keep a fallback monitor
    If the NSMenu delivery path is considered too environment-sensitive, preserve the old event-tracking monitor as a compatibility fallback instead of removing it outright.

Next step before merge

  • No automated repair is needed; the remaining action is maintainer review of the compatibility-sensitive AppKit shortcut path and draft/CI readiness.

Security
Cleared: The diff only changes Swift AppKit menu routing and tests; it does not touch dependencies, CI, release scripts, secrets, or supply-chain surfaces.

Review details

Best possible solution:

Land the simpler menu key-equivalent path once maintainers accept the AppKit compatibility tradeoff and the draft/CI gates are ready.

Do we have a high-confidence way to reproduce the issue?

No high-confidence before-fix reproduction was run in this review. The source path and merged related reports show the current shortcut implementation, and the PR body provides after-fix live output from the built app.

Is this the best way to solve the issue?

Yes, subject to maintainer acceptance of the AppKit compatibility risk. Reusing StatusItemMenu.performKeyEquivalent is narrower than maintaining a separate event-tracking monitor, but the fallback removal is the key merge decision.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 4756ba06bf42.

Label changes

Label justifications:

  • P2: This is a limited-surface provider switcher keyboard shortcut fix/cleanup with normal maintainer review urgency.
  • merge-risk: 🚨 compatibility: The PR replaces the existing event-tracking shortcut monitor with a different AppKit key-equivalent delivery path for existing shortcuts.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body includes redacted live output from a freshly built PR app showing provider selection state changes after Command-number shortcuts.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes redacted live output from a freshly built PR app showing provider selection state changes after Command-number shortcuts.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; its guidance to prefer focused menu/model tests and avoid private live-provider validation informed the proof and validation assessment. (AGENTS.md:1, 4756ba06bf42)
  • Current main menu key-equivalent path: Current main routes persistent actions and provider arrow navigation through StatusItemMenu.performKeyEquivalent, but Command-number selection is not handled there. (Sources/CodexBar/StatusItemMenu.swift:18, 4756ba06bf42)
  • Current main shortcut monitor path: Current main installs ProviderSwitcherShortcutEventMonitor during menu open and handles Command-number shortcuts through handleProviderSwitcherShortcut. (Sources/CodexBar/StatusItemController+ProviderSwitcher.swift:4, 4756ba06bf42)
  • PR replacement path: The PR adds Command-number handling to StatusItemMenu and delegates selected indices to the persistent action delegate, while removing the event-tracking monitor. (Sources/CodexBar/StatusItemMenu.swift:34, 7d3e97f3990a)
  • PR live proof: The PR body includes redacted live state-monitor output from a built PR app showing selected-provider and overview-state changes after Cmd-3, Cmd-1, and another Command-number check. (7d3e97f3990a)
  • Related feature history: GitHub search and commit history show the current monitor-based shortcut feature was introduced by merged work for provider switcher shortcuts, including the closed Cmd-number request and merged shortcut PR. (904549557ec1)

Likely related people:

  • anirudhvee: Commit 9045495 introduced the current provider switcher shortcut handling in the open menu and closed the related Cmd-number and arrow-navigation reports. (role: introduced current shortcut behavior; confidence: high; commits: 904549557ec1; files: Sources/CodexBar/StatusItemMenu.swift, Sources/CodexBar/StatusItemController+ProviderSwitcher.swift, Tests/CodexBarTests/StatusMenuSwitcherClickTests.swift)
  • steipete: Local blame on current main and GitHub path history show Peter Steinberger on the current menu/shortcut code and adjacent provider switcher navigation/menu readiness work. (role: recent area contributor; confidence: high; commits: 4756ba06bf42, eadf5c175e53, d7db9922b5fb; files: Sources/CodexBar/StatusItemMenu.swift, Sources/CodexBar/StatusItemController+ProviderSwitcher.swift, Sources/CodexBar/StatusItemController+ProviderNavigation.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jun 1, 2026
@ProspectOre

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 1, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 1, 2026
@ProspectOre

Copy link
Copy Markdown
Contributor Author

Closing this PR: live validation on macOS 26.5.1 (25F80) invalidated its premise.

Retest protocol

Packaged release build of this change rebased onto current main (cde92cfb), merged icons, codex + claude + Overview, Apple Silicon. A polling state monitor recorded selectedMenuProvider / mergedMenuLastSelectedWasOverview at 0.5s while the menu was operated manually: open → ⌘3 → ⌘1 → ⌘2 → ← / → → Esc, with the dropdown visibly tracking and the system otherwise idle (sample confirmed no background scan activity and an idle main thread before the test).

Result: zero selection changes recorded. None of the shortcuts worked.

12:49:50 selected=codex overview=0   # armed; no further transitions during the test

What this means

  1. StatusItemMenu.performKeyEquivalent does not receive key events while AppKit is tracking the menu on macOS 26.5. The persistent ⌘R/⌘,/⌘Q shortcuts keep working because they are real NSMenuItem.keyEquivalents matched natively by the tracking session — not because of the override.
  2. The ProviderSwitcherShortcutEventMonitor run-loop observer is therefore load-bearing for Command-number and arrow shortcut delivery during tracking, and since 463ec91 / 9778128 it is also the switcher's pointer-interaction fast path. Removing it (this PR) or narrowing it to pointer events only (retested separately on the same hardware) silently breaks the keyboard shortcuts.
  3. Hidden NSMenuItems do not participate in key-equivalent matching, so attaching ⌘1–⌘9 to invisible items is not a viable replacement either.

I could not reproduce this PR's original June 1 live proof on 26.5.1; on today's hardware retest the key-equivalent route never fired during tracking. Treat the monitor as the supported mechanism until AppKit offers a real key-equivalent hook for tracking sessions.

@ProspectOre
ProspectOre deleted the codex/provider-switcher-key-equivalents branch June 15, 2026 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant