Skip to content

Handle provider switcher shortcuts in the open menu#1157

Merged
steipete merged 4 commits into
steipete:mainfrom
anirudhvee:fix/provider-switcher-keyboard
May 26, 2026
Merged

Handle provider switcher shortcuts in the open menu#1157
steipete merged 4 commits into
steipete:mainfrom
anirudhvee:fix/provider-switcher-keyboard

Conversation

@anirudhvee
Copy link
Copy Markdown
Contributor

@anirudhvee anirudhvee commented May 26, 2026

Fixes #1156
Fixes #1144

Summary

  • make provider switcher keypresses work in the real open menu
  • add Cmd-number shortcuts for the visible switcher order
  • keep the menu open after switching providers from the keyboard

Validation

  • make check
  • swift test --filter StatusMenuSwitcherClickTests
  • ./Scripts/compile_and_run.sh
  • verified the freshly built app with the menu open: Cmd+1..4 and left/right switch tabs, and the menu stays open

Proof

  • Video proof: shows Cmd+1..4 and left/right switching providers while the menu stays open.
codexbar-switcher-wider-proof.mp4

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 26, 2026

Codex review: needs maintainer review before merge. Reviewed May 26, 2026, 4:11 PM ET / 20:11 UTC.

Summary
The branch adds a CFRunLoop event-tracking key monitor for the open merged provider menu, Cmd-number provider selection, monitor cleanup, and focused provider-switcher shortcut tests.

Reproducibility: not applicable. as a PR review; I did not need a failing current-main reproduction to assess the diff. The attached recording provides after-fix proof for the open-menu shortcut behavior.

Review metrics: 2 noteworthy metrics.

  • Diff Surface: 6 files changed, +305/-8. The change is focused but spans runtime menu wiring, a new event monitor, shortcut parsing, and tests.
  • Runtime Hooks Added: 1 CFRunLoop observer added. This is the main merge risk because it runs while AppKit is tracking an open menu.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster ✨ media proof bonus
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:

  • Refresh the unstable check and have a maintainer explicitly approve the .eventTracking monitor approach before merge.

Risk before merge

  • The PR adds a main-run-loop CFRunLoop observer that polls keyDown events in .eventTracking; if the event queue handling is wrong, users could see open-menu responsiveness problems or swallowed keyboard input.
  • GitHub context reports the PR as mergeable but unstable, and the author notes a failed SubprocessRunnerTests job as likely unrelated; maintainers should refresh checks before merge.

Maintainer options:

  1. Approve The Event-Tracking Monitor After Check Refresh (recommended)
    If maintainers are comfortable with polling keyDown events in .eventTracking and checks pass on rerun, this PR can land as the active fix.
  2. Ask For A Narrower AppKit Hook
    If the run-loop observer is too risky, request a design that scopes keyboard handling to the menu or switcher view without polling the app event queue.

Next step before merge
No automated repair remains; maintainer review should decide whether the event-tracking monitor is acceptable and refresh the unstable check before merge.

Security
Cleared: No security or supply-chain concern found; the diff does not touch dependencies, workflows, secrets, release scripts, or credential handling.

Review details

Best possible solution:

Merge this focused fix if maintainers accept the event-tracking monitor after green checks; otherwise ask for an AppKit-specific alternative that preserves open-menu keyboard switching.

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

Not applicable as a PR review; I did not need a failing current-main reproduction to assess the diff. The attached recording provides after-fix proof for the open-menu shortcut behavior.

Is this the best way to solve the issue?

Unclear as a product/design call: the patch is focused and proof-positive, but CFRunLoop polling in .eventTracking is the design choice maintainers should explicitly accept.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 8fccb454acbb.

Label changes

Label justifications:

  • P2: This is a normal-priority provider-switcher usability fix with limited blast radius but real menu interaction impact.
  • merge-risk: 🚨 availability: The new event-tracking observer could affect open-menu responsiveness or keyboard event handling if it behaves incorrectly.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (recording): The attached recording visibly demonstrates Cmd-number and arrow switching in the open menu while the menu remains open.
  • proof: sufficient: Contributor real behavior proof is sufficient. The attached recording visibly demonstrates Cmd-number and arrow switching in the open menu while the menu remains open.
  • proof: 🎥 video: Contributor real behavior proof includes video or recording evidence. The attached recording visibly demonstrates Cmd-number and arrow switching in the open menu while the menu remains open.
Evidence reviewed

What I checked:

Likely related people:

  • Peter Steinberger: Introduced the merged icon switcher, introduced the current provider navigation/menu keyboard files in the v0.29.1 history, and authored the latest PR-head monitor revisions. (role: feature owner and recent area contributor; confidence: high; commits: 6534c6d9c26d, 333d584a4950, 6ec819726bc2; files: Sources/CodexBar/StatusItemController+Menu.swift, Sources/CodexBar/StatusItemMenu.swift, Sources/CodexBar/StatusItemController+ProviderNavigation.swift)
  • Brandon Charleson: Recently changed ProviderSwitcherView, provider-switcher button layout, and StatusMenuSwitcherClickTests for quota-bar behavior near this keyboard surface. (role: recent adjacent contributor; confidence: medium; commits: 53a7f22885dc; files: Sources/CodexBar/StatusItemController+SwitcherViews.swift, Sources/CodexBar/ProviderSwitcherButtons.swift, Tests/CodexBarTests/StatusMenuSwitcherClickTests.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.

@anirudhvee anirudhvee changed the title fix: handle provider switcher shortcuts in open menu Handle provider switcher shortcuts in the open menu May 26, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels May 26, 2026
@anirudhvee
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 26, 2026

🦞🧹
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. proof: 🎥 video Contributor real behavior proof includes video or recording evidence. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. 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: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels May 26, 2026
@anirudhvee
Copy link
Copy Markdown
Contributor Author

Looks like the failed job is from SubprocessRunnerTests.reads large stdout without deadlock, not the provider switcher changes here.

I ran swift test --filter SubprocessRunnerTests locally and it passed. Could someone rerun the failed job?

@steipete steipete force-pushed the fix/provider-switcher-keyboard branch from 3b7e037 to 995a880 Compare May 26, 2026 20:06
@steipete steipete merged commit 9045495 into steipete:main May 26, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. proof: 🎥 video Contributor real behavior proof includes video or recording evidence. 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.

Left/right provider navigation doesn't work Add Cmd-number shortcuts for provider switcher

2 participants