fix dashboard manage actions to use visible row numbers#91
fix dashboard manage actions to use visible row numbers#91
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| activeIndexByFamily: { codex: 0 }, | ||
| accounts: Array.from({ length: 29 }, (_, index) => ({ | ||
| email: `account-${index + 1}@example.com`, | ||
| accountId: `acc_${index + 1}`, | ||
| refreshToken: `refresh-${index + 1}`, | ||
| accessToken: `access-${index + 1}`, | ||
| expiresAt: now + 3_600_000, | ||
| addedAt: now - 1_000 - index, | ||
| lastUsed: now - 1_000 - index, | ||
| enabled: true, | ||
| })), | ||
| }; | ||
| const staleAccounts = storage.accounts.slice(0, 28); |
There was a problem hiding this comment.
loadCall === 3 is brittle and can mask bugs
the stale-accounts test hard-codes loadCall === 3 to determine when to return stale storage, but the exact call count depends on implementation details inside runCodexMultiAuthCli. if the number of loadAccounts calls in the flow changes (e.g., an extra check is added), the stale data will be injected at the wrong point — the test could pass while testing a completely different scenario than intended.
use a flag that is set explicitly after the promptLoginMode mock fires the first time, so stale storage is returned on any call that follows the switch decision:
let returnStale = false;
loadAccountsMock.mockImplementation(async () => {
if (returnStale) {
return { version: 3, activeIndex: 0, activeIndexByFamily: { codex: 0 }, accounts: structuredClone(staleAccounts) };
}
return structuredClone(storage);
});
promptLoginModeMock
.mockImplementationOnce(async () => {
returnStale = true;
return { mode: "manage", switchAccountIndex: 28, selectedAccountNumber: 1 };
})
.mockResolvedValueOnce({ mode: "cancel" });
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/codex-manager-cli.test.ts
Line: 1345-1357
Comment:
**`loadCall === 3` is brittle and can mask bugs**
the stale-accounts test hard-codes `loadCall === 3` to determine when to return stale storage, but the exact call count depends on implementation details inside `runCodexMultiAuthCli`. if the number of `loadAccounts` calls in the flow changes (e.g., an extra check is added), the stale data will be injected at the wrong point — the test could pass while testing a completely different scenario than intended.
use a flag that is set explicitly after the promptLoginMode mock fires the first time, so stale storage is returned on any call that follows the switch decision:
```
let returnStale = false;
loadAccountsMock.mockImplementation(async () => {
if (returnStale) {
return { version: 3, activeIndex: 0, activeIndexByFamily: { codex: 0 }, accounts: structuredClone(staleAccounts) };
}
return structuredClone(storage);
});
promptLoginModeMock
.mockImplementationOnce(async () => {
returnStale = true;
return { mode: "manage", switchAccountIndex: 28, selectedAccountNumber: 1 };
})
.mockResolvedValueOnce({ mode: "cancel" });
```
How can I resolve this? If you propose a fix, please make it concise.
1 participant
What:
codex auth switch [index]behavior on saved-index semanticsWhy:
Risk:
Validation:
npm test -- test/cli-auth-menu.test.ts test/auth-menu-hotkeys.test.ts test/codex-manager-cli.test.tsnpm test -- test/codex-manager-cli.test.tsnpx eslint lib/cli.ts lib/codex-manager.ts test/cli-auth-menu.test.ts test/codex-manager-cli.test.tsnpx eslint lib/codex-manager.ts test/codex-manager-cli.test.tsgit diff --checknpm run typecheckcurrently fails on untouched repo-baseline issues inindex.ts,lib/storage.ts, and@codex-ai/*type resolutionnote: 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 fixes a user-facing bug where interactive manage-mode confirmations (switch, delete, toggle, refresh) were showing saved storage indexes instead of the visible dashboard row numbers after auto-reordering. the fix threads a
selectedAccountNumberfield throughLoginMenuResult, resolves it inbuildManageResult/resolveAccountDisplayNumber, and passes it asdisplayAccountNumberintorunSwitchand all other action handlers. directcodex auth switch <index>calls are unchanged.lib/cli.ts: addsresolveAccountDisplayNumber(prefersquickSwitchNumber, falls back toindex+1) andbuildManageResultwrapper; all eight manage-action return sites updatedlib/codex-manager.ts: addsresolveManageActionAccountNumberhelper andRunSwitchOptions; all confirmationconsole.log/console.error/console.warnmessages now usedisplayAccountNumber;formatAccountLabelis also passeddisplayAccountNumber - 1, making the label number consistent with the prefix but causing the account number to appear redundantly in the output stringtest/cli-auth-menu.test.ts: existing tests updated withselectedAccountNumberassertions; new regression forquickSwitchNumber: 1 / sourceIndex: 28(visible row 1 → saved index 29) addedtest/codex-manager-cli.test.ts: new switch/stale-switch/delete/toggle/refresh regressions added; the stale-switch test uses aloadCall === 3hard-coded counter which is fragile; toggle re-enable path from a reordered row is not coveredsplice/mutation followed bysaveAccountswithout a write lock, which is a pre-existing race on windows; no token data flows through the newselectedAccountNumberfield so there is no token leakage riskConfidence Score: 4/5
resolveAccountDisplayNumberpriority ordering is sound and all eight action return sites are updated consistently. the only actionable concern is the brittleloadCall === 3counter in the stale-switch test which could silently mis-target a future refactor, and missing toggle re-enable coverage for reordered rows. no token leakage, no new windows concurrency risk introduced.Important Files Changed
resolveAccountDisplayNumberandbuildManageResulthelpers; threadsselectedAccountNumberthrough all manage action returns. logic is clean, priority ofquickSwitchNumberoverindex+1is correct, and defensiveMath.max(1,…)guards are appropriate.resolveManageActionAccountNumberandRunSwitchOptions; propagatesdisplayAccountNumberthrough all confirmation messages.formatAccountLabel(account, displayAccountNumber - 1)makes the label number consistent with the prefix, though it produces a redundant number in output. no new windows filesystem concurrency concerns introduced, but delete/toggle still lack a write lock around splice+save.selectedAccountNumberassertions to all affected existing tests and a new regression for thequickSwitchNumber: 1 / sourceIndex: 28case. coverage is solid for thepromptLoginModesurface.loadCall === 3counter that will silently mis-target if call order changes. toggle re-enable path from a reordered row is not covered.Sequence Diagram
sequenceDiagram participant UI as auth-menu UI participant CLI as cli.ts (promptLoginMode) participant MGR as codex-manager.ts (handleManageAction) participant SW as runSwitch() participant FS as saveAccounts / loadAccounts UI->>CLI: action (account with index/sourceIndex/quickSwitchNumber) CLI->>CLI: resolveAccountSourceIndex(account) → savedIndex CLI->>CLI: resolveAccountDisplayNumber(account) → displayNumber CLI->>CLI: buildManageResult → { switchAccountIndex: savedIndex, selectedAccountNumber: displayNumber } CLI-->>MGR: LoginMenuResult MGR->>MGR: resolveManageActionAccountNumber(result, savedIndex) → displayAccountNumber alt switch action MGR->>SW: runSwitch([String(savedIndex+1)], { displayAccountNumber }) SW->>FS: loadAccounts() FS-->>SW: storage SW->>FS: saveAccounts(storage) SW-->>MGR: console.log("Switched to account {displayAccountNumber}: …") else delete action MGR->>FS: storage.accounts.splice(savedIndex, 1) MGR->>FS: saveAccounts(storage) MGR-->>MGR: console.log("Deleted account {displayAccountNumber}.") else toggle action MGR->>FS: account.enabled = !account.enabled MGR->>FS: saveAccounts(storage) MGR-->>MGR: console.log("Enabled/Disabled account {displayAccountNumber}.") else refresh action MGR->>MGR: runOAuthFlow() MGR->>FS: persistAccountPool / syncSelectionToCodex MGR-->>MGR: console.log("Refreshed account {displayAccountNumber}.") endPrompt To Fix All With AI
Last reviewed commit: d9556ab
Context used: