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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a9b9cae89
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| var resolvedClaudeSwapExecutablePath: String { | ||
| Self.resolveClaudeSwapExecutablePath(configured: self.claudeSwapExecutablePath) |
There was a problem hiding this comment.
Share the default executable path with CLI consumers
When the new settings UI is enabled with its path left blank, the config still stores no claudeSwapExecutablePath; only this app-scoped computed property supplies ~/.local/bin/cswap. Consequently, codexbar cards still resolves the path to "" in CLIClaudeSwapCards.executablePath, and dashboard collection does the same in CLIDashboardCommand, so both pass an unconfigured path to ClaudeSwapAccountReader even though the menu-bar app now works. Put the fallback in shared config/path resolution or apply it at both CLI call sites as well.
AGENTS.md reference: AGENTS.md:L4-L4
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Reviewed September 14, 2026, 10:25 AM ET / 14:25 UTC (Revision 11). ClawSweeper reviewWhat this changesUnifies Claude and Codex account menus, separates viewing from activation, adds switch feedback, and groups Claude adapter settings with automatic executable discovery. Merge readiness⛔ Blocked before merge - 5 items remain This PR remains distinct from the merged account-menu work. Earlier privacy and subtitle fixes are preserved, but the previously reported feedback-identity defect remains unresolved. Priority: P2 Review scores
Verification
How this fits togetherCodexBar turns provider account snapshots into menu cards and account controls. Explicit System Account actions call the existing provider switching transactions, then display progress and completion feedback. flowchart TD
A[Provider account snapshots] --> B[Shared account menu]
B --> C[Select account to view]
C --> D[Usage card]
B --> E[Explicit System Account choice]
E --> F[Existing provider switching transaction]
F --> G[Card feedback or notification]
Decision needed
Why: These deliberately change shipped interaction and configuration semantics; documentation and synthetic upgrade checks establish behavior but cannot establish maintainer acceptance. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep shared presentation and existing transaction owners, bind feedback to stable account identity, and ship the interaction/default transition only with explicit acceptance and native notification evidence. Do we have a high-confidence way to reproduce the issue? Yes, by source inspection: promote a managed Codex workspace among accounts sharing an email, then rebuild its stacked or compact card; its visible ID changes while feedback retains the old ID. No runtime reproduction was executed in this read-only review. Is this the best way to solve the issue? Unclear until the upgrade direction is accepted; sharing controls while retaining existing switching owners is maintainable, but feedback must use stable identity and the notification path needs real native validation. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against a5f2c581ce2e. 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 (10 earlier review cycles; latest 8 shown)
|
1aa6acc to
1adbaad
Compare
1adbaad to
581af6f
Compare
88cc007 to
df3df0c
Compare
Selecting a Claude account segment now only changes which account's details the menu shows; it never invokes cswap activation. Activation stays behind the existing explicit "Switch Account…" card action with its validation and serialization intact. - Highlight the viewed segment (filled) independently from claude-swap's source-owned active marker (a "●" glyph plus tooltip/accessibility text). - Keep the viewed account for the app session across menu closes, keyed by ProviderAccountIdentity so it survives refreshes and list reordering. - Scope the selection to the adapter configuration: disabling claude-swap or changing its executable drops it. A removed slot falls back to the source-reported active account, and no active account is invented. - Keep unavailable accounts inspectable without activation, and let a later view selection outrank pending/failed activation while the activation error stays attached to its own account. Adds menu-model regression coverage plus an opt-in synthetic render proof; no real cswap executable, provider probe, or Keychain read is involved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R
Enabling "Read accounts from claude-swap" no longer requires typing a path. The adapter now resolves ~/.local/bin/cswap — the location the settings field already advertised as its placeholder — whenever something executable is really there. An explicitly configured path always wins, including when it is missing, so a wrong path reports an error instead of silently running another binary; a missing default resolves to empty, so users without claude-swap keep seeing no adapter activity and no recurring error. The claude-swap toggle, executable field, adapter status and discovered accounts now render as one section through the same supplementarySettingsContent slot the Codex accounts section uses, replacing the split toggle/field layout whose status text said "below" while the settings form rendered fields above toggles. Also drops the "Details for <account>" heading from the segmented menu: it dated from when the switcher highlighted the active account while the card showed a different one, and now only restates the highlighted segment. VoiceOver still reads the state from the segment's own accessibility label. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017aP1xPvmUqQLRtXXqUjiLx
…wap proof The signed proof recorded activationsStarted as a literal 0. It now clicks every segment through the real menu's switcher view and handler, and measures activation on the store's switch owner after each click and on a logging stub standing in for cswap. It also records that the card's explicit Switch Account action is offered only where activation is possible. A second case runs executable discovery for fresh, missing-default, upgraded empty-path, explicit and missing-explicit configs through the production config loader, shared resolver and account reader against synthetic stubs, and checks that an explicit path never falls back to the default. Both cases stay skipped unless CODEXBAR_CLAUDE_SWAP_VIEW_PROOF_DIR is set with credential isolation, and run no real cswap, account or Keychain access. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R
One segmented switcher for account menus: layout, selected styling, click tracking and hit-testing ported from the Codex switcher, plus a leading ● marker for the System account and tooltips/accessibility labels that name the System and Selected states. Providers supply labels and width-aware titles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R
… mark the System account The Codex menu switcher is now the shared segmented switcher. Width-aware email/workspace/discriminator titles and private ordinals move into CodexAccountSwitcherLabeling as pure functions; CodexAccountSwitcherView remains as an override-free subclass so menu reuse still recognizes the Codex row. The live account (the one the Codex CLI uses) gains a ● System marker, and tooltips name the System and Selected states. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R
The segmented claude-swap switcher is now the shared account switcher, built through ClaudeSwapAccountMenuDisplay.switcherView from the same display model. The ● marker is the System account (the one claude-swap reports active) and tooltips read System and Selected, matching Codex. Selection stays view-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R
Adds UsageMenuCardView.Model.applyingSubtitle(text:style:), matching the applyingUsageItemVisibility pattern, so account switch progress can show in loading, info or error style on both account and live cards built through the shared card construction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R
A pure per-provider phase (switching, succeeded, failed) that derives the card subtitle and the completion notification. Successes clear once a menu closes; failures stay until the next switch; a discarded result leaves nothing behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R
Codex's System Account submenu becomes a shared builder behind two defaulted ProviderImplementation hooks, with a provider-neutral requestSystemAccountSwitch action replacing requestCodexSystemPromotion. Codex keeps its exact promotion eligibility and coordinator; its submenu titles now honor Hide Personal Info and the title is localized. The controller records switch progress per provider: the card subtitle shows switching, success and failure, successes clear after a top-level menu closes, and a notification reports the result when no menu is open, replacing the Codex failure alert. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R
…ubmenu claude-swap accounts join the shared System Account submenu while claude-swap owns Claude account presentation: the active slot is checked, inactive slots with usable credentials are enabled, and everything is disabled while a switch runs. The adapter awaits the existing serialized cswap --switch-to transaction, which now returns its task, and reads the per-slot error from the store. The card no longer offers Switch Account for inactive slots. It keeps upstream's Active badge and its explicit Re-authenticate repair action for an active slot claude-swap marks re-activatable. The target card shows switch progress and success; errors keep rendering on the account that produced them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R
Adds the System Account submenu title, the Selected switcher state, and the switch progress, success, notification and stale-account strings to all 23 app catalogs, reusing each locale's existing System account wording. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R
Documents the System Account submenu as the activation entry point for claude-swap and Codex, the ● System marker, switch feedback and notifications, and lists the claude-swap changes under 0.60.2 Unreleased. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R
…nly clicks The signed claude-swap proof now also switches through the System Account submenu against the logging stub (the Claude refresh is replaced): segment clicks start no activation, the submenu checks the System slot and enables only the switchable one, choosing it runs exactly one cswap --switch-to, and a success notice is posted. The receipt records the submenu, the switch invocation and the notice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R
Addresses review of the shared switch feedback: - A switch error claude-swap already published outranks switching progress on the card, instead of staying hidden until the Claude refresh finishes. - Stacked and compact Codex account cards show progress, success and failure for the account being promoted. - Switch feedback no longer gets replaced by the live refresh monitor: a card with an applied subtitle stops following live provider status, and the header resolves its subtitle through a helper covered by a monitor-backed test. - A failure notice that cannot be delivered (notifications not authorized) falls back to the alert Codex showed before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R
df3df0c to
0c775b8
Compare
Co-authored-by: Codex <codex@ai>
Co-authored-by: Codex <codex@ai>
Apply the existing private account-label policy to System Account submenu titles while preserving raw names when privacy is off and retaining promotion targets, checked state, and visibility. Native proof: #3702 (comment) Extracted from #3550; the broader account-switching redesign remains separate. Co-authored-by: Chandler Anderson <chanderson90@proton.me>
|
The existing System Account submenu now honors Hide Personal Info on main through #3702, merged as 141ecf6. It uses stable private account labels while keeping promotion targets and menu state intact; privacy off retains the original display names. Validation: Thanks @zenibako. This repairs the existing submenu's privacy omission; the broader account-switching redesign in #3550 remains open. |
|
Thanks @zenibako for the implementation and proof. Closing this broad unification proposal. Main already fixes Codex System Account label privacy in 141ecf6 and clarifies Claude’s explicit chip switching and reconciliation feedback in 9a2f5e4 (#3740). The remaining changes are a larger UX redesign: view-only chips, relocated activation, executable discovery, grouped settings, and shared notification state. Those are not all superseded, but we are retaining the current provider-specific behavior rather than adopting this combined redesign. |
Why
Claude and Codex should behave the same way, so expectations don't differ per provider. Before this PR they didn't:
cswap --switch-to, changing Claude Code's login from a click that looks like "show me this account".This PR gives both providers one model: segments select, the System Account submenu switches.
What
1. Claude segment clicks are view-only.
cswap.ProviderAccountIdentity. It survives refreshes, reordering and menu closes for the session, and is not persisted across restarts. It never changes which account drives the menu bar quota.2. One shared account switcher for Claude and Codex.
AccountSegmentedSwitcherViewreplacesCodexAccountSwitcherViewandClaudeSwapAccountSwitcherView. It owns the layout, selection styling, click tracking, hit-testing, tooltips and accessibility.CodexAccountSwitcherLabelingas pure functions.●marks the System account, the one the CLI uses (CodexisLive, claude-swap active slot).CodexAccountSwitcherViewremains as an override-free subclass, so menu reuse still recognizes the Codex row.3. One shared System Account submenu for switching.
ProviderImplementationhooks.requestSystemAccountSwitchaction replacesrequestCodexSystemPromotion.CodexAccountPromotionCoordinatorare unchanged. Submenu titles now honor Hide Personal Info; previously they showed raw emails. The submenu title is now localized.cswap --switch-to <slot> --jsontransaction.foreign_credential).4. Switch feedback.
MenuCardLiveSubtitle.resolve.5. The claude-swap executable has a real default, and its settings are grouped.
~/.local/bin/cswapwas only the settings field's placeholder, so enabling the adapter without typing a path silently did nothing.supplementarySettingsContent, the slot Codex already uses.Behavior changes for existing users
docs/claude.md.●in the switcher;~/.local/bin/cswapif it is installed there. Otherwise nothing runs, as before.Proof
The signed synthetic native proofs are opt-in via
CODEXBAR_CLAUDE_SWAP_VIEW_PROOF_DIR. They require credential isolation and use synthetic accounts and stub executables only.Screenshots: the production switcher in Light and Dark, viewing slots 2, 7 and 9 while slot 2 is System.
Segments vs submenu (
menu-owner.json):--switch-toinvocation.--switch-to 7 --jsonand posts one notice: "Claude Code now uses Account 7".Executable discovery (
discovery.json), run through the production config loader, resolver and reader:Testing
main62a7fa1c.make check: 0 violations, including the app locale checker.Scripts/test.sh): 106/106 groups pass on the first pass, with nothing skipped.MenuCardRefreshMonitor.cswap, provider probe or Keychain read in any test.Notes
docs/claude.md,docs/claude-multi-account-and-status-items.mdanddocs/codex.mdare updated.Not included
The merged-menu row-height bug from #3549 is fixed separately in #3616.
🤖 Generated with Claude Code
https://claude.ai/code/session_012T91QRonsX591fKr6UuN5R