Skip to content

refactor: route switch through command module#157

Closed
ndycode wants to merge 1 commit intorefactor/pr1-remove-inline-features-wrapperfrom
refactor/pr1-remove-inline-switch-wrapper
Closed

refactor: route switch through command module#157
ndycode wants to merge 1 commit intorefactor/pr1-remove-inline-features-wrapperfrom
refactor/pr1-remove-inline-switch-wrapper

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 20, 2026

Summary

  • remove the remaining inline switch wrapper from lib/codex-manager.ts
  • route both CLI and login-menu switch paths directly through the extracted switch command module

What Changed

  • deleted the now-redundant inline runSwitch() helper
  • updated codex auth switch dispatch to call runSwitchCommand(...) directly
  • updated the login-menu manage flow to use the same extracted switch command path

Validation

  • npm run test -- test/codex-manager-switch-command.test.ts test/codex-manager-cli.test.ts
  • npm run lint
  • npm run typecheck
  • npm run build

Risk and Rollback

  • Risk level: low
  • Rollback plan: revert 9bfbcc6 to restore the inline switch wrapper

Additional Notes

  • this removes another dispatcher-only wrapper without changing behavior or storage semantics

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this pr removes the now-redundant runSwitch inline helper from lib/codex-manager.ts and routes both the codex auth switch cli path and the login-menu manage flow directly through runSwitchCommand from the extracted switch command module. the deps object (setStoragePath, loadAccounts, persistAndSyncSelectedAccount) is identical at both call sites, so behavior is unchanged — this is a clean dead-code removal.

  • cli path (command === "switch" in runCodexMultiAuthCli): now calls runSwitchCommand(rest, deps) directly instead of through runSwitch
  • menu path (handleManageAction when switchAccountIndex is set): same direct call, same index-offset arithmetic (index + 1 → parsed back to targetIndex = parsed - 1)
  • no token safety or windows filesystem risks introduced — persistAndSyncSelectedAccount uses the existing unified-settings retry queue, unchanged
  • no new concurrency risks — storage access serialisation is unchanged
  • one pre-existing gap worth pinning: handleManageAction discards the number return from runSwitchCommand (it's Promise<void>), so switch failures from the menu are only surfaced via console.error; a negative-path vitest case would make this contract explicit (see inline comment)

Confidence Score: 5/5

  • safe to merge — clean dead-code removal with no behavioral changes
  • identical deps at both call sites, behavior unchanged, existing tests cover both dispatch paths, no token or windows filesystem risks introduced
  • no files require special attention

Important Files Changed

Filename Overview
lib/codex-manager.ts removes the redundant runSwitch wrapper; both CLI and menu paths now call runSwitchCommand directly with identical deps — behavior is unchanged, deps are correct, no token/storage safety regressions

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[runCodexMultiAuthCli] -->|command === 'switch'| B["runSwitchCommand(rest, deps)"]
    C[handleManageAction] -->|switchAccountIndex is number| D["index = switchAccountIndex\nrunSwitchCommand([String(index+1)], deps)"]
    B --> E[runSwitchCommand]
    D --> E
    E --> F{validate args}
    F -->|missing arg| G["logError / return 1"]
    F -->|invalid index| H["logError / return 1"]
    F -->|valid| I[loadAccounts]
    I --> J{index in range?}
    J -->|no| K["logError / return 1"]
    J -->|yes| L[persistAndSyncSelectedAccount]
    L --> M{synced?}
    M -->|no| N[logWarn]
    M -->|yes| O[logInfo]
    N --> O
    O --> P[return 0]

    style D fill:#d4edda,stroke:#28a745
    style B fill:#d4edda,stroke:#28a745
    style G fill:#f8d7da,stroke:#dc3545
    style H fill:#f8d7da,stroke:#dc3545
    style K fill:#f8d7da,stroke:#dc3545
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 4385-4390

Comment:
**missing vitest coverage for menu-switch error paths**

`handleManageAction` is `Promise<void>` so the `number` returned by `runSwitchCommand` is silently dropped here. the existing test in `codex-manager-cli.test.ts` (line ~3022) only exercises the happy path (`switchAccountIndex: 1`, two-account storage, success). there's no coverage for when `runSwitchCommand` returns `1` from this call site — e.g. storage load failure, index out of range from a stale menu render, or `persistAndSyncSelectedAccount` throwing. those failures surface only as a `console.error` and the menu loop continues silently.

this behavior is pre-existing (the old `runSwitch` wrapper also discarded the return), but since the wrapper is now gone it's worth adding at least one negative-path case in `codex-manager-cli.test.ts` to pin the error-suppression contract explicitly:

```ts
it("silently continues when switch command errors from menu", async () => {
  loadAccountsMock.mockResolvedValue(null); // forces runSwitchCommand → return 1
  promptLoginModeMock
    .mockResolvedValueOnce({ mode: "manage", switchAccountIndex: 0 })
    .mockResolvedValueOnce({ mode: "cancel" });
  const errSpy = vi.spyOn(console, "error").mockImplementation(() => {});
  const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js");

  const exitCode = await runCodexMultiAuthCli(["auth", "login"]);
  expect(exitCode).toBe(0);           // menu loop finishes, not propagated
  expect(errSpy).toHaveBeenCalledWith("No accounts configured.");
});
```

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "refactor: route swit..."

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 20, 2026

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)
  • skip-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6e8dc479-cbe7-4ea7-bdea-2d39199e614d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/pr1-remove-inline-switch-wrapper
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch refactor/pr1-remove-inline-switch-wrapper

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@ndycode ndycode added the passed label Mar 20, 2026
@ndycode
Copy link
Owner Author

ndycode commented Mar 23, 2026

Closing because this work is now included in main via #318 and #319.

@ndycode ndycode closed this Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant