feat(tui): migrate auth dashboard and settings flows to ink#50
feat(tui): migrate auth dashboard and settings flows to ink#50ndycode wants to merge 2 commits intofeat/storage-resilience-recoveryfrom
Conversation
📝 WalkthroughWalkthroughmajor refactor introducing interactive auth dashboard ui with two renderers: ink (react-based terminal) and opentui (native tui). refactors login flow from direct action objects to resolution-based interaction model. adds comprehensive settings ui with persistence. extends storage deduplication logic. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes significant architectural change introducing dual ui renderers and restructuring auth flow. major concerns:
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
12 issues found across 29 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/storage.test.ts">
<violation number="1" location="test/storage.test.ts:641">
P2: Test description "returns null when file does not exist" is now misleading — the assertions verify a non-null restore-eligible object (with `accounts: []`, `restoreEligible: true`, `restoreReason: 'missing-storage'`). Update the `it()` string to reflect the actual expected behavior, e.g. `"returns restore-eligible sentinel when file does not exist"`.</violation>
<violation number="2" location="test/storage.test.ts:1869">
P2: Test description says "removes primary, backup, and wal artifacts" but the assertions now verify that backups (`.bak`, `.bak.1`, `.bak.2`) and `.wal` are **preserved** — only the primary file is removed. This will confuse anyone reading the test to understand `clearAccounts` behavior. Update the `it()` string, e.g. `"removes only primary storage, preserving backup and wal artifacts"`.</violation>
</file>
<file name="lib/ui-ink/detail-flows.ts">
<violation number="1" location="lib/ui-ink/detail-flows.ts:169">
P2: The `render()` return value is discarded, so `waitUntilExit()` is never awaited. The promise resolves via the callback before Ink finishes restoring stdout, which can corrupt subsequent terminal output. Store the instance and await `waitUntilExit()` before returning.</violation>
<violation number="2" location="lib/ui-ink/detail-flows.ts:231">
P1: Destructive action auto-confirms on last keystroke without requiring Enter, contradicting the footer instruction "Type DELETE then Enter". The user has no chance to review their input before the action fires. Remove the auto-confirm block so only the explicit Enter path triggers confirmation.</violation>
</file>
<file name="lib/ui-ink/dashboard.ts">
<violation number="1" location="lib/ui-ink/dashboard.ts:81">
P2: `!timestamp` is falsy for both `undefined` and `0`. A timestamp value of `0` would return `"never"` instead of a formatted date. Use a nullish check instead.</violation>
</file>
<file name="lib/runtime-paths.ts">
<violation number="1" location="lib/runtime-paths.ts:81">
P2: On non-Windows, `pathsEqualNormalized` only trims but doesn't actually normalize paths. A trailing slash in `CODEX_HOME` (common in shell configs) will cause a false mismatch against the `join()`-produced default, making `isExplicitNonDefaultHome` incorrectly `true` and skipping all fallback candidate scanning.</violation>
</file>
<file name="lib/codex-manager.ts">
<violation number="1" location="lib/codex-manager.ts:3593">
P2: Toggle value `enabled` is computed from stale `storage` outside the transaction. Since `withAccountStorageTransaction` provides the latest `current` state, the toggle should be derived from `nextAccount.enabled` inside the callback to avoid overwriting a concurrent state change.</violation>
</file>
<file name="lib/ui-ink/settings.ts">
<violation number="1" location="lib/ui-ink/settings.ts:975">
P1: Ink `render()` return value is discarded — the instance is never awaited via `waitUntilExit()`. The promise resolves on `onResolve` before `exit()` finishes unmounting, so in the looping callers (`promptInkBackendSettings`, `configureInkUnifiedSettings`) a new render starts while the previous instance's stdin listeners are still attached, causing duplicate key handling.</violation>
</file>
<file name="test/ui-ink-dashboard.test.ts">
<violation number="1" location="test/ui-ink-dashboard.test.ts:138">
P2: Test omits `env` option, making it sensitive to the host environment. If `CODEX_TUI`, `CODEX_DESKTOP`, `TERM_PROGRAM=codex`, or `ELECTRON_RUN_AS_NODE=1` is set in the runner, `resolveInkAuthShellBootstrap` returns unsupported and `promptInkAuthDashboard` returns `null`. Pass a clean `env: {}` for deterministic behavior.</violation>
</file>
<file name="test/codex-manager-cli.test.ts">
<violation number="1" location="test/codex-manager-cli.test.ts:273">
P1: `mockReset()` strips the default implementation from `createEmptyAccountStorageMock`, making it return `undefined`. Production code in `codex-manager.ts` calls `createEmptyAccountStorage()` in multiple paths (e.g. `await saveAccounts(createEmptyAccountStorage())`). Use `mockClear()` instead to preserve the implementation while resetting call tracking.</violation>
</file>
<file name="lib/storage.ts">
<violation number="1" location="lib/storage.ts:748">
P1: `loadAccountsForRestoreAssessment` removes the `.reset-intent` marker as a side effect of a read/query operation. If `getRestoreAssessment()` is called before `loadAccounts()`, the marker is gone and the subsequent load won't know the reset was intentional—defeating the reset suppression feature.
The assessment function should read the marker without deleting it; only `loadAccountsInternal` (the authoritative load path) should consume the marker.</violation>
<violation number="2" location="lib/storage.ts:1411">
P1: Fallback migration undoes an intentional account reset. When `clearAccounts()` deletes the primary file and writes a `.reset-intent` marker, `migrateFallbackAccountStorageIfNeeded` runs first, finds a fallback file, and persists it via `saveAccountsUnlocked`—which removes the reset marker and re-creates the primary file. The subsequent early-return check (`hasIntentionalResetMarker && !existsSync(path)`) then fails because the file now exists, so the user's reset is silently overridden by the migrated fallback data.
Move the intentional reset early-return check before the fallback migration call.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const nextValue = `${valueRef.current}${nextLetters}`.slice(0, confirmText.length); | ||
| valueRef.current = nextValue; | ||
| setValue(nextValue); | ||
| if (nextValue.toUpperCase() === confirmText) { |
There was a problem hiding this comment.
P1: Destructive action auto-confirms on last keystroke without requiring Enter, contradicting the footer instruction "Type DELETE then Enter". The user has no chance to review their input before the action fires. Remove the auto-confirm block so only the explicit Enter path triggers confirmation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/ui-ink/detail-flows.ts, line 231:
<comment>Destructive action auto-confirms on last keystroke without requiring Enter, contradicting the footer instruction "Type DELETE then Enter". The user has no chance to review their input before the action fires. Remove the auto-confirm block so only the explicit Enter path triggers confirmation.</comment>
<file context>
@@ -0,0 +1,464 @@
+ const nextValue = `${valueRef.current}${nextLetters}`.slice(0, confirmText.length);
+ valueRef.current = nextValue;
+ setValue(nextValue);
+ if (nextValue.toUpperCase() === confirmText) {
+ resolve(true);
+ }
</file context>
| patchConsole: options.patchConsole ?? false, | ||
| exitOnCtrlC: options.exitOnCtrlC ?? false, | ||
| }; | ||
| render(createElement(App, { ...options, onResolve: (value: TResult) => resolve(value) }), renderOptions); |
There was a problem hiding this comment.
P1: Ink render() return value is discarded — the instance is never awaited via waitUntilExit(). The promise resolves on onResolve before exit() finishes unmounting, so in the looping callers (promptInkBackendSettings, configureInkUnifiedSettings) a new render starts while the previous instance's stdin listeners are still attached, causing duplicate key handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/ui-ink/settings.ts, line 975:
<comment>Ink `render()` return value is discarded — the instance is never awaited via `waitUntilExit()`. The promise resolves on `onResolve` before `exit()` finishes unmounting, so in the looping callers (`promptInkBackendSettings`, `configureInkUnifiedSettings`) a new render starts while the previous instance's stdin listeners are still attached, causing duplicate key handling.</comment>
<file context>
@@ -0,0 +1,1773 @@
+ patchConsole: options.patchConsole ?? false,
+ exitOnCtrlC: options.exitOnCtrlC ?? false,
+ };
+ render(createElement(App, { ...options, onResolve: (value: TResult) => resolve(value) }), renderOptions);
+ });
+}
</file context>
| saveAccountsMock.mockReset(); | ||
| saveFlaggedAccountsMock.mockReset(); | ||
| clearAccountsMock.mockReset(); | ||
| createEmptyAccountStorageMock.mockReset(); |
There was a problem hiding this comment.
P1: mockReset() strips the default implementation from createEmptyAccountStorageMock, making it return undefined. Production code in codex-manager.ts calls createEmptyAccountStorage() in multiple paths (e.g. await saveAccounts(createEmptyAccountStorage())). Use mockClear() instead to preserve the implementation while resetting call tracking.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/codex-manager-cli.test.ts, line 273:
<comment>`mockReset()` strips the default implementation from `createEmptyAccountStorageMock`, making it return `undefined`. Production code in `codex-manager.ts` calls `createEmptyAccountStorage()` in multiple paths (e.g. `await saveAccounts(createEmptyAccountStorage())`). Use `mockClear()` instead to preserve the implementation while resetting call tracking.</comment>
<file context>
@@ -178,18 +205,81 @@ function makeErrnoError(message: string, code: string): NodeJS.ErrnoException {
saveAccountsMock.mockReset();
saveFlaggedAccountsMock.mockReset();
+ clearAccountsMock.mockReset();
+ createEmptyAccountStorageMock.mockReset();
+ withAccountStorageTransactionMock.mockReset();
queuedRefreshMock.mockReset();
</file context>
| createEmptyAccountStorageMock.mockReset(); | |
| createEmptyAccountStorageMock.mockClear(); |
| return null; | ||
| } | ||
|
|
||
| if (hasIntentionalResetMarker) { |
There was a problem hiding this comment.
P1: loadAccountsForRestoreAssessment removes the .reset-intent marker as a side effect of a read/query operation. If getRestoreAssessment() is called before loadAccounts(), the marker is gone and the subsequent load won't know the reset was intentional—defeating the reset suppression feature.
The assessment function should read the marker without deleting it; only loadAccountsInternal (the authoritative load path) should consume the marker.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/storage.ts, line 748:
<comment>`loadAccountsForRestoreAssessment` removes the `.reset-intent` marker as a side effect of a read/query operation. If `getRestoreAssessment()` is called before `loadAccounts()`, the marker is gone and the subsequent load won't know the reset was intentional—defeating the reset suppression feature.
The assessment function should read the marker without deleting it; only `loadAccountsInternal` (the authoritative load path) should consume the marker.</comment>
<file context>
@@ -391,6 +448,364 @@ function computeSha256(value: string): string {
+ return null;
+ }
+
+ if (hasIntentionalResetMarker) {
+ await removeIntentionalResetMarker(path);
+ }
</file context>
| const migratedLegacyStorage = persistMigration | ||
| ? await migrateLegacyProjectStorageIfNeeded(persistMigration) | ||
| : null; | ||
| const migratedFallbackStorage = persistMigration |
There was a problem hiding this comment.
P1: Fallback migration undoes an intentional account reset. When clearAccounts() deletes the primary file and writes a .reset-intent marker, migrateFallbackAccountStorageIfNeeded runs first, finds a fallback file, and persists it via saveAccountsUnlocked—which removes the reset marker and re-creates the primary file. The subsequent early-return check (hasIntentionalResetMarker && !existsSync(path)) then fails because the file now exists, so the user's reset is silently overridden by the migrated fallback data.
Move the intentional reset early-return check before the fallback migration call.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/storage.ts, line 1411:
<comment>Fallback migration undoes an intentional account reset. When `clearAccounts()` deletes the primary file and writes a `.reset-intent` marker, `migrateFallbackAccountStorageIfNeeded` runs first, finds a fallback file, and persists it via `saveAccountsUnlocked`—which removes the reset marker and re-creates the primary file. The subsequent early-return check (`hasIntentionalResetMarker && !existsSync(path)`) then fails because the file now exists, so the user's reset is silently overridden by the migrated fallback data.
Move the intentional reset early-return check before the fallback migration call.</comment>
<file context>
@@ -899,22 +1399,40 @@ async function loadAccountsFromJournal(path: string): Promise<AccountStorageV3 |
const migratedLegacyStorage = persistMigration
? await migrateLegacyProjectStorageIfNeeded(persistMigration)
: null;
+ const migratedFallbackStorage = persistMigration
+ ? await migrateFallbackAccountStorageIfNeeded(path, persistMigration)
+ : null;
</file context>
| const migratedFallbackStorage = persistMigration | |
| if (hasIntentionalResetMarker && !existsSync(path)) { | |
| await removeIntentionalResetMarker(path); | |
| const emptyStorageWithMetadata: AccountStorageWithMetadata = { | |
| ...createEmptyAccountStorage(), | |
| restoreEligible: false, | |
| restoreReason: "intentional-reset", | |
| }; | |
| return emptyStorageWithMetadata; | |
| } | |
| const migratedFallbackStorage = persistMigration |
| exitOnCtrlC: options.exitOnCtrlC ?? false, | ||
| }; | ||
|
|
||
| render(createElement(InkChoiceApp, { ...options, onResolve: finish }), renderOptions); |
There was a problem hiding this comment.
P2: The render() return value is discarded, so waitUntilExit() is never awaited. The promise resolves via the callback before Ink finishes restoring stdout, which can corrupt subsequent terminal output. Store the instance and await waitUntilExit() before returning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/ui-ink/detail-flows.ts, line 169:
<comment>The `render()` return value is discarded, so `waitUntilExit()` is never awaited. The promise resolves via the callback before Ink finishes restoring stdout, which can corrupt subsequent terminal output. Store the instance and await `waitUntilExit()` before returning.</comment>
<file context>
@@ -0,0 +1,464 @@
+ exitOnCtrlC: options.exitOnCtrlC ?? false,
+ };
+
+ render(createElement(InkChoiceApp, { ...options, onResolve: finish }), renderOptions);
+ });
+}
</file context>
| } | ||
|
|
||
| function formatRelativeTime(timestamp: number | undefined): string { | ||
| if (!timestamp) return "never"; |
There was a problem hiding this comment.
P2: !timestamp is falsy for both undefined and 0. A timestamp value of 0 would return "never" instead of a formatted date. Use a nullish check instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/ui-ink/dashboard.ts, line 81:
<comment>`!timestamp` is falsy for both `undefined` and `0`. A timestamp value of `0` would return `"never"` instead of a formatted date. Use a nullish check instead.</comment>
<file context>
@@ -0,0 +1,681 @@
+}
+
+function formatRelativeTime(timestamp: number | undefined): string {
+ if (!timestamp) return "never";
+ const days = Math.floor((Date.now() - timestamp) / 86_400_000);
+ if (days <= 0) return "today";
</file context>
| if (process.platform === "win32") { | ||
| return win32.normalize(trimmed).toLowerCase(); | ||
| } | ||
| return trimmed; |
There was a problem hiding this comment.
P2: On non-Windows, pathsEqualNormalized only trims but doesn't actually normalize paths. A trailing slash in CODEX_HOME (common in shell configs) will cause a false mismatch against the join()-produced default, making isExplicitNonDefaultHome incorrectly true and skipping all fallback candidate scanning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/runtime-paths.ts, line 81:
<comment>On non-Windows, `pathsEqualNormalized` only trims but doesn't actually normalize paths. A trailing slash in `CODEX_HOME` (common in shell configs) will cause a false mismatch against the `join()`-produced default, making `isExplicitNonDefaultHome` incorrectly `true` and skipping all fallback candidate scanning.</comment>
<file context>
@@ -72,6 +72,17 @@ function deduplicatePaths(paths: string[]): string[] {
+ if (process.platform === "win32") {
+ return win32.normalize(trimmed).toLowerCase();
+ }
+ return trimmed;
+ };
+ return normalize(a) === normalize(b);
</file context>
| return trimmed; | |
| return trimmed.replace(/\/+$/, "") || "/"; |
| if (account) { | ||
| account.enabled = account.enabled === false; | ||
| await saveAccounts(storage); | ||
| const enabled = account.enabled === false; |
There was a problem hiding this comment.
P2: Toggle value enabled is computed from stale storage outside the transaction. Since withAccountStorageTransaction provides the latest current state, the toggle should be derived from nextAccount.enabled inside the callback to avoid overwriting a concurrent state change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/codex-manager.ts, line 3593:
<comment>Toggle value `enabled` is computed from stale `storage` outside the transaction. Since `withAccountStorageTransaction` provides the latest `current` state, the toggle should be derived from `nextAccount.enabled` inside the callback to avoid overwriting a concurrent state change.</comment>
<file context>
@@ -3687,10 +3590,18 @@ async function handleManageAction(
if (account) {
- account.enabled = account.enabled === false;
- await saveAccounts(storage);
+ const enabled = account.enabled === false;
+ await withAccountStorageTransaction(async (current, persist) => {
+ const nextStorage = cloneAccountStorage(current) ?? createEmptyAccountStorage();
</file context>
| const stderr = createMockOutput(); | ||
| const dashboard = createDashboardViewModel(); | ||
|
|
||
| const resultPromise = promptInkAuthDashboard({ |
There was a problem hiding this comment.
P2: Test omits env option, making it sensitive to the host environment. If CODEX_TUI, CODEX_DESKTOP, TERM_PROGRAM=codex, or ELECTRON_RUN_AS_NODE=1 is set in the runner, resolveInkAuthShellBootstrap returns unsupported and promptInkAuthDashboard returns null. Pass a clean env: {} for deterministic behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/ui-ink-dashboard.test.ts, line 138:
<comment>Test omits `env` option, making it sensitive to the host environment. If `CODEX_TUI`, `CODEX_DESKTOP`, `TERM_PROGRAM=codex`, or `ELECTRON_RUN_AS_NODE=1` is set in the runner, `resolveInkAuthShellBootstrap` returns unsupported and `promptInkAuthDashboard` returns `null`. Pass a clean `env: {}` for deterministic behavior.</comment>
<file context>
@@ -0,0 +1,155 @@
+ const stderr = createMockOutput();
+ const dashboard = createDashboardViewModel();
+
+ const resultPromise = promptInkAuthDashboard({
+ dashboard,
+ stdin: input,
</file context>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (assessment.restoreReason === "empty-storage") { | ||
| try { | ||
| await fs.unlink(assessment.storagePath); | ||
| } catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code; | ||
| if (code !== "ENOENT") { | ||
| console.warn( | ||
| `Restore preparation failed for ${assessment.storagePath}: ${error instanceof Error ? error.message : String(error)}`, | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
windows filesystem locking prevents WAL/backup recovery
when restoreReason === "empty-storage", the code deletes the primary storage file (line 822) before calling loadAccounts() so WAL/backup recovery can proceed. on windows, antivirus scanners and shell integrations routinely hold files open, causing fs.unlink to throw EBUSY or EPERM.
the catch block logs a warning (lines 826-829) but then continues to loadAccounts() on line 833. the empty primary file remains on disk. loadAccounts() loads it, finds zero accounts, and recovery is skipped. user loses their backed-up accounts with only a warning log they won't see.
fix: wrap the fs.unlink with a short retry loop for EBUSY and EPERM (reuse the pattern from settings-persistence.ts line 12: RETRYABLE_SETTINGS_WRITE_CODES). if unlink still fails after retries, log clearly and fall back gracefully.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 820-831
Comment:
**windows filesystem locking prevents WAL/backup recovery**
when `restoreReason === "empty-storage"`, the code deletes the primary storage file (line 822) before calling `loadAccounts()` so WAL/backup recovery can proceed. on windows, antivirus scanners and shell integrations routinely hold files open, causing `fs.unlink` to throw `EBUSY` or `EPERM`.
the catch block logs a warning (lines 826-829) but then continues to `loadAccounts()` on line 833. the empty primary file remains on disk. `loadAccounts()` loads it, finds zero accounts, and recovery is skipped. user loses their backed-up accounts with only a warning log they won't see.
**fix**: wrap the `fs.unlink` with a short retry loop for `EBUSY` and `EPERM` (reuse the pattern from `settings-persistence.ts` line 12: `RETRYABLE_SETTINGS_WRITE_CODES`). if unlink still fails after retries, log clearly and fall back gracefully.
How can I resolve this? If you propose a fix, please make it concise.
| function formatAccountDetail(account: AuthAccountViewModel): string | undefined { | ||
| const parts: string[] = []; | ||
| if (account.isCurrentAccount) parts.push("current"); | ||
| if (account.status) parts.push(`status ${account.status}`); | ||
| if (account.quotaSummary) parts.push(account.quotaSummary); | ||
| return parts.length > 0 ? parts.join(" | ") : undefined; | ||
| } | ||
|
|
||
| function toneForAccount(account: AuthAccountViewModel): InkShellTone { | ||
| switch (account.status) { | ||
| case "active": | ||
| case "ok": | ||
| return "success"; | ||
| case "rate-limited": | ||
| case "cooldown": | ||
| return "warning"; | ||
| case "disabled": | ||
| case "error": | ||
| case "flagged": | ||
| return "danger"; | ||
| default: | ||
| return "muted"; | ||
| } | ||
| } | ||
|
|
||
| function entriesForSection( | ||
| dashboard: AuthDashboardViewModel, | ||
| sectionId: AuthDashboardSectionId, | ||
| ): AuthInkShellEntry[] { | ||
| if (sectionId === "saved-accounts") { | ||
| return dashboard.accounts.map((account, index) => ({ | ||
| id: `account:${account.sourceIndex ?? index}`, | ||
| label: account.email ?? account.accountLabel ?? account.accountId ?? `Account ${index + 1}`, | ||
| detail: formatAccountDetail(account), | ||
| tone: toneForAccount(account), | ||
| kind: "account", | ||
| })); | ||
| } | ||
|
|
||
| const section = dashboard.sections.find((candidate) => candidate.id === sectionId); | ||
| return (section?.actions ?? []).map((action) => ({ | ||
| id: `action:${action.id}`, | ||
| label: action.label, | ||
| tone: mapActionTone(action), | ||
| kind: "action", | ||
| })); | ||
| } |
There was a problem hiding this comment.
duplicate logic between auth-shell and dashboard
entriesForSection, normalizeFocus, formatAccountDetail, and tone functions (accountTone/toneForAccount, actionTone/mapActionTone) are defined in both auth-shell.ts and dashboard.ts with subtle differences. for example, formatAccountDetail in dashboard.ts (line 124–133) includes "last used" info via formatRelativeTime(account.lastUsed), but the version here (line 48–54) omits it entirely.
these implementations will diverge further over time (they already have). consider extracting them into a shared lib/ui-ink/shell-utils.ts module. at minimum, add a vitest test that calls both formatAccountDetail implementations with the same input and asserts they produce identical output — to catch future drift early.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/ui-ink/auth-shell.ts
Line: 48-94
Comment:
**duplicate logic between auth-shell and dashboard**
`entriesForSection`, `normalizeFocus`, `formatAccountDetail`, and tone functions (`accountTone`/`toneForAccount`, `actionTone`/`mapActionTone`) are defined in both `auth-shell.ts` and `dashboard.ts` with subtle differences. for example, `formatAccountDetail` in `dashboard.ts` (line 124–133) includes "last used" info via `formatRelativeTime(account.lastUsed)`, but the version here (line 48–54) omits it entirely.
these implementations will diverge further over time (they already have). consider extracting them into a shared `lib/ui-ink/shell-utils.ts` module. at minimum, add a vitest test that calls both `formatAccountDetail` implementations with the same input and asserts they produce identical output — to catch future drift early.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
ndycode
changed the title
Move the auth login flow onto the new OpenTUI runtime, wire settings persistence through the shared controller layer, and split the heavy CLI regression suite so the verified test gate stays green on Windows.
| async function promptInkChoice(options: PromptInkChoiceOptions): Promise<InkChoiceValue | null> { | ||
| const support = resolveInkAuthShellBootstrap(options); | ||
| if (!support.supported) return null; | ||
|
|
||
| return await new Promise<InkChoiceValue | null>((resolve) => { | ||
| let settled = false; | ||
| const finish = (value: InkChoiceValue | null) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| resolve(value); | ||
| }; | ||
|
|
||
| const renderOptions: RenderOptions = { | ||
| stdin: options.stdin ?? process.stdin, | ||
| stdout: options.stdout ?? process.stdout, | ||
| stderr: options.stderr ?? process.stderr, | ||
| debug: options.debug ?? false, | ||
| patchConsole: options.patchConsole ?? false, | ||
| exitOnCtrlC: options.exitOnCtrlC ?? false, | ||
| }; | ||
|
|
||
| render(createElement(InkChoiceApp, { ...options, onResolve: finish }), renderOptions); | ||
| }); | ||
| } | ||
|
|
||
| interface PromptInkTextConfirmOptions extends InkAuthShellEnvironment { | ||
| title: string; | ||
| panelTitle?: string; | ||
| subtitle?: string; | ||
| status?: string; | ||
| statusTone?: InkShellTone; | ||
| prompt: string; | ||
| confirmText: string; | ||
| stdin?: NodeJS.ReadStream; | ||
| stdout?: NodeJS.WriteStream; | ||
| stderr?: NodeJS.WriteStream; | ||
| debug?: boolean; | ||
| patchConsole?: boolean; | ||
| exitOnCtrlC?: boolean; | ||
| } | ||
|
|
||
| interface InkTextConfirmAppProps extends PromptInkTextConfirmOptions { | ||
| onResolve: (value: boolean | null) => void; | ||
| } | ||
|
|
||
| function InkTextConfirmApp(props: InkTextConfirmAppProps) { | ||
| const { exit } = useApp(); | ||
| const theme = createInkShellTheme(); | ||
| const [value, setValue] = useState(""); | ||
| const valueRef = useRef(""); | ||
| const confirmText = props.confirmText.toUpperCase(); | ||
| const current = value.toUpperCase(); | ||
| const ready = current === confirmText; | ||
|
|
||
| const resolve = (result: boolean | null) => { | ||
| props.onResolve(result); | ||
| exit(); | ||
| }; | ||
|
|
||
| useInput((input, key) => { | ||
| const lower = input.toLowerCase(); | ||
| if (key.escape || lower === "q") { | ||
| resolve(false); | ||
| return; | ||
| } | ||
| if (key.backspace || key.delete) { | ||
| const nextValue = valueRef.current.slice(0, -1); | ||
| valueRef.current = nextValue; | ||
| setValue(nextValue); | ||
| return; | ||
| } | ||
| if (key.return) { | ||
| if (valueRef.current.toUpperCase() === confirmText) { | ||
| resolve(true); | ||
| } | ||
| return; | ||
| } | ||
| if (!input || key.ctrl || key.meta) return; | ||
| const nextLetters = input.toUpperCase().replace(/[^A-Z]/g, ""); | ||
| if (nextLetters.length === 0) return; | ||
| const nextValue = `${valueRef.current}${nextLetters}`.slice(0, confirmText.length); | ||
| valueRef.current = nextValue; | ||
| setValue(nextValue); | ||
| if (nextValue.toUpperCase() === confirmText) { | ||
| resolve(true); | ||
| } | ||
| }); | ||
|
|
||
| return createElement( | ||
| InkShellFrame, | ||
| { | ||
| title: props.title, | ||
| subtitle: props.subtitle, | ||
| status: props.status ?? (ready ? "Confirmation ready" : `Type ${confirmText}`), | ||
| statusTone: props.statusTone ?? (ready ? "success" : "warning"), | ||
| footer: `Type ${confirmText} then Enter | Backspace delete | Q Back`, | ||
| theme, | ||
| }, | ||
| createElement( | ||
| InkShellPanel, | ||
| { title: props.panelTitle ?? props.title, theme }, | ||
| createElement(Text, { color: theme.textColor }, props.prompt), | ||
| createElement(Box, { marginTop: 1, flexDirection: "column" }, | ||
| createElement(Text, { color: theme.mutedColor }, `Required: ${confirmText}`), | ||
| createElement(Text, { color: ready ? theme.successColor : theme.headingColor, bold: true }, `Typed: ${current || "_"}`), | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| async function promptInkTextConfirm(options: PromptInkTextConfirmOptions): Promise<boolean | null> { | ||
| const support = resolveInkAuthShellBootstrap(options); | ||
| if (!support.supported) return null; | ||
|
|
||
| return await new Promise<boolean | null>((resolve) => { | ||
| let settled = false; | ||
| const finish = (value: boolean | null) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| resolve(value); | ||
| }; | ||
|
|
||
| const renderOptions: RenderOptions = { | ||
| stdin: options.stdin ?? process.stdin, | ||
| stdout: options.stdout ?? process.stdout, | ||
| stderr: options.stderr ?? process.stderr, | ||
| debug: options.debug ?? false, | ||
| patchConsole: options.patchConsole ?? false, | ||
| exitOnCtrlC: options.exitOnCtrlC ?? false, | ||
| }; | ||
|
|
||
| render(createElement(InkTextConfirmApp, { ...options, onResolve: finish }), renderOptions); | ||
| }); |
There was a problem hiding this comment.
render instance discarded — no cleanup path on abort
both promptInkChoice (line 169) and promptInkTextConfirm (line 279) call render() but throw away the returned Instance. if the ink app panics, the process is killed externally, or a future code path bypasses the useApp().exit() call, the ink renderer stays alive and stdin is never released.
compare renderInkDashboardOnce in dashboard.ts which explicitly keeps the instance and calls instance?.unmount(); instance?.cleanup(); inside the finish closure. the same pattern should be applied here — capture the Instance and add cleanup to the finish handler.
no vitest test covers the abort path for either helper, so a leaked renderer would go undetected in ci.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/ui-ink/detail-flows.ts
Line: 148-280
Comment:
**render instance discarded — no cleanup path on abort**
both `promptInkChoice` (line 169) and `promptInkTextConfirm` (line 279) call `render()` but throw away the returned `Instance`. if the ink app panics, the process is killed externally, or a future code path bypasses the `useApp().exit()` call, the ink renderer stays alive and stdin is never released.
compare `renderInkDashboardOnce` in `dashboard.ts` which explicitly keeps the instance and calls `instance?.unmount(); instance?.cleanup();` inside the `finish` closure. the same pattern should be applied here — capture the Instance and add cleanup to the finish handler.
no vitest test covers the abort path for either helper, so a leaked renderer would go undetected in ci.
How can I resolve this? If you propose a fix, please make it concise.| if (!input || key.ctrl || key.meta) return; | ||
| const nextLetters = input.toUpperCase().replace(/[^A-Z]/g, ""); | ||
| if (nextLetters.length === 0) return; | ||
| const nextValue = `${valueRef.current}${nextLetters}`.slice(0, confirmText.length); | ||
| valueRef.current = nextValue; | ||
| setValue(nextValue); | ||
| if (nextValue.toUpperCase() === confirmText) { | ||
| resolve(true); | ||
| } | ||
| }); |
There was a problem hiding this comment.
InkTextConfirmApp auto-submits the moment the final char is typed — no Enter gate
lines 231–232 call resolve(true) immediately when nextValue === confirmText, before the user presses Enter. the footer explicitly says Type DELETE then Enter, but the component bypasses that gate entirely.
on windows, clipboard paste events (and some terminal emulators) may deliver multiple keystrokes in a single ink input event. a partial paste that happens to fill confirmText will auto-delete without any explicit user confirmation. the key.return branch at line 219 already handles the intended flow correctly — the early-exit at lines 231–232 should be removed, requiring an explicit Enter press to confirm.
no vitest coverage for the paste-batch edge case on windows.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/ui-ink/detail-flows.ts
Line: 225-234
Comment:
**`InkTextConfirmApp` auto-submits the moment the final char is typed — no Enter gate**
lines 231–232 call `resolve(true)` immediately when `nextValue === confirmText`, before the user presses Enter. the footer explicitly says `Type DELETE then Enter`, but the component bypasses that gate entirely.
on windows, clipboard paste events (and some terminal emulators) may deliver multiple keystrokes in a single ink input event. a partial paste that happens to fill `confirmText` will auto-delete without any explicit user confirmation. the `key.return` branch at line 219 already handles the intended flow correctly — the early-exit at lines 231–232 should be removed, requiring an explicit Enter press to confirm.
no vitest coverage for the paste-batch edge case on windows.
How can I resolve this? If you propose a fix, please make it concise.| type DashboardDisplaySettingKey = | ||
| | "menuShowStatusBadge" | ||
| | "menuShowCurrentBadge" | ||
| | "menuShowLastUsed" | ||
| | "menuShowQuotaSummary" | ||
| | "menuShowQuotaCooldown" | ||
| | "menuShowDetailsForUnselectedRows" | ||
| | "menuShowFetchStatus" | ||
| | "menuHighlightCurrentRow" | ||
| | "menuSortEnabled" | ||
| | "menuSortPinCurrent" | ||
| | "menuSortQuickSwitchVisibleRow"; | ||
|
|
||
| interface DashboardDisplaySettingOption { | ||
| key: DashboardDisplaySettingKey; | ||
| label: string; | ||
| description: string; | ||
| } | ||
|
|
||
| const DASHBOARD_DISPLAY_OPTIONS: DashboardDisplaySettingOption[] = [ | ||
| { | ||
| key: "menuShowStatusBadge", | ||
| label: "Show Status Badges", | ||
| description: "Show [ok], [active], and similar badges.", | ||
| }, | ||
| { | ||
| key: "menuShowCurrentBadge", | ||
| label: "Show [current]", | ||
| description: "Mark the account active in Codex.", | ||
| }, | ||
| { | ||
| key: "menuShowLastUsed", | ||
| label: "Show Last Used", | ||
| description: "Show relative usage like 'today'.", | ||
| }, | ||
| { | ||
| key: "menuShowQuotaSummary", | ||
| label: "Show Limits (5h / 7d)", | ||
| description: "Show limit bars in each row.", | ||
| }, | ||
| { | ||
| key: "menuShowQuotaCooldown", | ||
| label: "Show Limit Cooldowns", | ||
| description: "Show reset timers next to 5h/7d bars.", | ||
| }, | ||
| { |
There was a problem hiding this comment.
settings constants re-declared — should import from settings-ui-controller.ts
lib/ui-ink/settings.ts re-declares DashboardDisplaySettingKey, DASHBOARD_DISPLAY_OPTIONS, and related constants as local copies instead of importing from lib/codex-manager/settings-ui-controller.ts where they are already exported and maintained.
these definitions have already partially drifted: DashboardDisplaySettingKey in settings.ts includes all the keys, but settings-ui-controller.ts is the authoritative source. maintaining duplicates invites future divergence as either file changes.
import the shared constants and types from the controller instead of re-declaring them locally. missing vitest assertion to keep both in sync.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/ui-ink/settings.ts
Line: 36-81
Comment:
**settings constants re-declared — should import from `settings-ui-controller.ts`**
`lib/ui-ink/settings.ts` re-declares `DashboardDisplaySettingKey`, `DASHBOARD_DISPLAY_OPTIONS`, and related constants as local copies instead of importing from `lib/codex-manager/settings-ui-controller.ts` where they are already exported and maintained.
these definitions have already partially drifted: `DashboardDisplaySettingKey` in `settings.ts` includes all the keys, but `settings-ui-controller.ts` is the authoritative source. maintaining duplicates invites future divergence as either file changes.
import the shared constants and types from the controller instead of re-declaring them locally. missing vitest assertion to keep both in sync.
How can I resolve this? If you propose a fix, please make it concise.| promptInkRestoreForLogin, | ||
| type InkShellTone, | ||
| } from "./ui-ink/index.js"; | ||
| import { promptOpenTuiAuthDashboard } from "../runtime/opentui/prompt.js"; |
There was a problem hiding this comment.
lib/ importing from ../runtime/opentui/ — unconventional dependency direction
import { promptOpenTuiAuthDashboard } from "../runtime/opentui/prompt.js" (line 98) crosses the lib/ → runtime/ boundary in the wrong direction. runtime/ is supposed to depend on lib/, not vice versa. this creates a circular potential and breaks build isolation — tests that mock lib/codex-manager.ts now transitively pull in the entire @opentui/solid and solid-js dependency chain from runtime/.
consider exposing a promptOpenTuiAuthDashboard adapter inside lib/ui-ink/index.ts (which is already the ui-layer boundary that codex-manager.ts imports from) and keeping the opentui import inside that layer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 98
Comment:
**`lib/` importing from `../runtime/opentui/`** — unconventional dependency direction
`import { promptOpenTuiAuthDashboard } from "../runtime/opentui/prompt.js"` (line 98) crosses the `lib/` → `runtime/` boundary in the wrong direction. `runtime/` is supposed to depend on `lib/`, not vice versa. this creates a circular potential and breaks build isolation — tests that mock `lib/codex-manager.ts` now transitively pull in the entire `@opentui/solid` and solid-js dependency chain from `runtime/`.
consider exposing a `promptOpenTuiAuthDashboard` adapter inside `lib/ui-ink/index.ts` (which is already the ui-layer boundary that `codex-manager.ts` imports from) and keeping the opentui import inside that layer.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 43
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/request/response-handler.ts (1)
85-87:⚠️ Potential issue | 🟠 Majorpotential token leak in debug logging
lib/request/response-handler.ts:86logsfullContent: fullTextwhich may contain auth tokens, api keys, or pii embedded in sse payloads. even behindLOGGING_ENABLED, this could leak sensitive data to logs in debug/staging environments.consider redacting or truncating before logging. as per coding guidelines, "check for logging that leaks tokens or emails."
🛡️ proposed mitigation
if (LOGGING_ENABLED) { - logRequest("stream-full", { fullContent: fullText }); + logRequest("stream-full", { + contentLength: fullText.length, + contentPreview: fullText.slice(0, 200) + (fullText.length > 200 ? '...[truncated]' : ''), + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/request/response-handler.ts` around lines 85 - 87, The debug log in response-handler.ts currently calls logRequest("stream-full", { fullContent: fullText }) and may leak tokens/PII; update the code around LOGGING_ENABLED/ logRequest to sanitize fullText before logging by implementing a redaction/truncation utility (e.g., redactSensitive(text) or truncateAndRedact(text)) and pass the sanitized value to logRequest instead of raw fullText, ensuring patterns for tokens/keys/emails are removed or masked and very long payloads are truncated; keep the original variable names (fullText, LOGGING_ENABLED, logRequest, "stream-full") so changes are minimal and easy to locate.lib/request/stream-failover.ts (2)
1-238:⚠️ Potential issue | 🟠 Majoradd explicit unit tests for new helper functions
test/stream-failover.test.tscoverswithStreamingFailoverintegration but lacks unit tests for new helpers introduced in this pr. add tests forlib/request/stream-failover.ts:113-121(normalizeRequestInstanceIdedge cases: empty, whitespace-only, >64 char truncation),lib/request/stream-failover.ts:35-50(timeout trigger and cleanup), andlib/request/stream-failover.ts:65-78(error detection). the integration test attest/stream-failover.test.ts:69-85usesrequestInstanceId: "req-123"but doesn't verify truncation, empty handling, or timeout mechanics perlib/**guideline requirement to cite affected tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/request/stream-failover.ts` around lines 1 - 238, The PR added helper functions but lacks unit tests for normalizeRequestInstanceId, readChunkWithTimeout, and isStallTimeoutError; add focused tests that exercise edge-cases: (1) normalizeRequestInstanceId should return null for undefined, empty, and whitespace-only inputs and should truncate inputs longer than MAX_REQUEST_INSTANCE_ID_LENGTH (validate length and preserved prefix) using the function name normalizeRequestInstanceId; (2) readChunkWithTimeout should reject with a StallTimeoutError when the read promise doesn't resolve within timeout and must clear the timer (use a mock read Promise that never resolves and assert the rejected error is a StallTimeoutError and that timers are cleaned up) referencing readChunkWithTimeout and StallTimeoutError; and (3) isStallTimeoutError should return true for a StallTimeoutError instance and objects with isStallTimeout === true and false otherwise (use isStallTimeoutError). Add these unit tests in test/stream-failover.test.ts (or a new unit test file) to complement the existing integration tests.
175-177:⚠️ Potential issue | 🟡 Minordocument that callers must sanitize requestInstanceId before passing sensitive data; add regression test
the
normalizeRequestInstanceIdfunction atlib/request/stream-failover.ts:82-88explicitly truncates to 64 chars but does not mask content. its docstring at line 77 states callers must "apply any token-redaction or masking required for logs or telemetry." in production usage (index.ts:2279),requestInstanceIdreceivesrequestCorrelationId, which is derived fromCODEX_THREAD_IDenv var orpromptCacheKey(a host-provided session identifier). these are safe identifiers, but there is no test verifying that tokens or emails cannot leak if a caller passes sensitive data by mistake. add a regression test that confirms the failover marker output rejects or redacts token-like strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/request/stream-failover.ts` around lines 175 - 177, normalizeRequestInstanceId currently only truncates to 64 chars and relies on callers to mask secrets; update its docstring (normalizeRequestInstanceId in lib/request/stream-failover.ts) to clearly state callers must sanitize/redact tokens/emails before passing requestInstanceId, and then add a regression test that exercises the failover marker generation (the code that builds markerLabel near the failoverAttempt usage) to assert token-like strings are rejected or redacted in output; implement the test by passing a requestInstanceId containing a token-like pattern and verifying the produced failover marker does not contain the raw token (e.g., contains a redaction placeholder or stripped secret) so future changes won’t leak secrets.test/codex-manager-cli.test.ts (1)
1415-1518:⚠️ Potential issue | 🟠 Majoradd a vitest that drives the real opentui quick-switch path.
test/codex-manager-cli.test.ts:1415-1594only inspects derivedquickSwitchNumbermetadata. it never presses a digit through the opentui shell, so the teardown race inruntime/opentui/app.ts:2043-2056would pass unnoticed. please add a deterministic vitest around the real shell entrypoint that asserts quick-switch exits cleanly without extra renders.as per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.Also applies to: 1520-1594
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 1415 - 1518, The current test only inspects quickSwitchNumber metadata and doesn't exercise the real OpenTUI shell, so add a deterministic vitest that calls the actual CLI entry (runCodexMultiAuthCli) and drives the OpenTUI shell to perform a quick-switch by sending the digit key press into the real prompt flow (the same code path used by promptOpenTuiAuthDashboard/OpenTUI runtime), then await the shell to fully exit and assert it completed without extra renders or lingering teardown work referenced in runtime/opentui/app.ts (teardown around the render loop). Use vitest utilities to fake timers and stable inputs (mock network/token refresh deterministically and use fixed timestamps like Date.now), drive the shell input programmatically to press the quick-switch digit, and assert the process exits cleanly (no extra renders/callbacks) and exit code is as expected to reproduce and prevent the teardown race.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eslint.config.js`:
- Around line 42-69: The override currently targets only "runtime/**/*.ts" and
should include TSX so the TypeScript parser and rules (parser: tsparser,
parserOptions.project: "./tsconfig.runtime-opentui.json") apply to .tsx
entrypoints too; update the files array in this override (the object that
defines plugins "@typescript-eslint" and rules like
"@typescript-eslint/no-floating-promises", "@typescript-eslint/await-thenable",
etc.) to include "runtime/**/*.tsx" alongside "runtime/**/*.ts" so future .tsx
files get the same async/promise safety and linting.
In `@lib/codex-manager.ts`:
- Around line 3640-3642: The non-interactive guard (isNonInteractiveMode())
currently sits inside the conditional that checks existingStorage &&
existingStorage.accounts.length > 0, so existingStorage is only nulled when
accounts exist; move the isNonInteractiveMode() check to the start of the flow
(before evaluating existingStorage and before the while loop that prompts for
selection) so non-interactive mode always skips interactive prompts, or
alternatively add a clarifying comment if the current behavior is intentional;
update references around existingStorage, isNonInteractiveMode(), and the while
prompt loop to reflect the new guard placement.
- Around line 3700-3704: The current call to persistOpenTuiSettingsSave(event)
swallows failures by only console.warn-ing in the .catch; instead, update the
error handler to surface the failure to the user by setting the relevant
statusMessage (e.g., this.statusMessage or the manager/state object that holds
statusMessage) with a clear, user-facing message including the error details,
and still log the warning for diagnostics; locate the call to
persistOpenTuiSettingsSave(event) and replace the silent .catch handler so it
sets statusMessage to something like "Failed to save OpenTUI settings: <error
message>" (include error instanceof Error ? error.message : String(error)) while
preserving existing logging.
In `@lib/codex-manager/auth-ui-controller.ts`:
- Around line 193-203: resolveManagedAccountIndex currently accepts and coerces
account.index as a fallback which lets negative/fractional or UI-derived row
indices be used for storage operations; change resolveManagedAccountIndex to
require a non-negative integer sourceIndex only (i.e., return sourceIndex only
when typeof account.sourceIndex === "number",
Number.isFinite(account.sourceIndex), and Number.isInteger(account.sourceIndex)
&& account.sourceIndex >= 0) and remove using/coercing account.index as a
fallback so delete/toggle/refresh cannot operate on unstable UI row indices;
also add a vitest regression that passes missing, negative, and fractional
sourceIndex values (and a UI-derived index value) and asserts the controller
takes the warning/abort path rather than operating on a saved account.
- Around line 205-210: The unresolvable-account message uses raw
account.email/accountId and can leak terminal control sequences; update
buildUnresolvableAccountMessage to pass the chosen label through
sanitizeTerminalText() (reuse the same sanitization used in the title path)
before interpolating into the returned string, and add a Vitest that simulates
an account with escape/control characters in email or accountId to assert the
returned message has those sequences stripped/escaped.
- Around line 590-593: The title construction appends a " [disabled]" suffix
even when account.status is already "disabled", causing duplicate labels; update
the logic that builds title (variable title near formatAccountTitle and
statusLabel) to only append " [disabled]" when account.enabled === false AND
(account.status ?? "unknown") !== "disabled" (or equivalently statusLabel !==
"disabled"), so the extra suffix is not added when status already indicates
disabled.
- Around line 168-181: The subtitle formatting is non-deterministic because
formatRelativeTime and formatDate call Date.now() and toLocaleDateString()
directly; change formatRelativeTime(timestamp) and formatDate(timestamp) to
accept a now parameter (e.g., formatRelativeTime(timestamp, now)) and use
toLocaleDateString with explicit locale/timeZone options (e.g., 'en-US' and
'UTC' or a chosen fixed zone) so formatting does not vary by host; update the
caller buildAuthAccountDetailViewModel to pass a deterministic now value
(injected from the view-model builder) and adjust tests to supply a fixed clock
(use vitest fake timers or a fixed timestamp) and add an assertion that the
subtitle output is stable across TZs.
In `@lib/request/stream-failover.ts`:
- Around line 16-17: The duplicated type aliases StreamReader and
StreamReadResult here should be consolidated with the identical
MinimalReadableStreamReader and StreamReadResult in response-handler.ts; remove
the local type defs and import the shared types instead (either export
MinimalReadableStreamReader/StreamReadResult from response-handler.ts and import
them into stream-failover.ts, or move both to a new shared module like
lib/request/types.ts and import from there), updating references to StreamReader
and StreamReadResult to use the exported/shared symbols to prevent drift.
In `@lib/ui-ink/auth-shell.ts`:
- Around line 186-197: The useInput callback in AuthInkShell currently closes
over props.dashboard causing stale reads; change to store dashboard in a ref
(e.g., const dashboardRef = useRef(props.dashboard)) and update it in a
useEffect whenever props.dashboard changes, then read dashboardRef.current
inside the useInput handler when calling reduceAuthInkShellFocus and
entriesForSection; update references to use focusActionFromInput, setFocus, and
reduceAuthInkShellFocus accordingly so the handler always operates on the latest
dashboard. Also add a regression test in test/ui-ink-auth-shell.test.ts that
mounts AuthInkShell, updates the dashboard prop while rapidly sending input
events to ensure the input handler uses the updated dashboard (simulate
concurrent rapid input and prop changes).
In `@lib/ui-ink/dashboard.ts`:
- Around line 361-364: The function warnUnresolvableAccountSelection currently
logs raw PII (email/accountId); remove the identifier-based console.log and
instead emit a generic non-identifying message (e.g., "Unable to resolve saved
account for action") inside warnUnresolvableAccountSelection, or remove the log
entirely if redundant with user-facing error handling; update any vitest cases
that asserted the previous detailed log to expect the new generic message (cite
those tests in your commit message), and when touching related auth/queue code
ensure any new queue logic accounts for EBUSY/429 retry semantics per project
guidelines and that no other logging in the changed code prints tokens/emails.
In `@lib/ui-ink/layout.ts`:
- Around line 49-52: createInkShellTheme currently ignores the saved accent and
returns BLUE_THEME or GREEN_THEME based only on ui.palette; update
createInkShellTheme to read the runtime accent from getUiRuntimeOptions() (e.g.,
ui.accent or similar) and derive the returned InkShellTheme's accentColor/token
from that value instead of hard-coding per-palette, preserving existing
palette-specific tokens for non-accent fields; then add a small vitest that
calls createInkShellTheme with mocked getUiRuntimeOptions values to assert the
returned theme contains the expected accentColor for multiple accent values (and
for both palettes) to ensure the accent selection flows through.
In `@lib/ui-ink/settings.ts`:
- Around line 959-976: The promptInkSettingsScreen function resolves its promise
immediately because it doesn't capture the Instance returned by render; update
promptInkSettingsScreen to store the render() return value (the Ink Instance),
pass onResolve to InkSettingsApp as before, but when onResolve is called (from
InkSettingsApp), first call instance.unmount() and then instance.cleanup() (or
await any async unmount if supported) before resolving the promise so the
renderer fully releases stdin/stdout; mirror the pattern used in dashboard.ts
around render/instance/unmount/cleanup and ensure this prevents re-entry races
when configureInkUnifiedSettings calls subsequent prompts like
promptInkAccountListSettings.
In `@lib/ui/auth-menu.ts`:
- Around line 608-619: The current chain of .replace() calls on detail.title is
fragile; instead have buildAuthAccountDetailViewModel return a discrete status
field (e.g., detail.status) or normalize and extract the status with a single
tolerant regex, then map that status to badges via statusBadge/formatUiBadge;
update the code that builds header in auth-menu (where detail.title is used) to
remove the multiple .replace() calls, read detail.status (or the regex result),
normalize casing/whitespace, and switch on known statuses
("active","ok","rate-limited","cooldown","disabled","error","flagged","unknown")
to produce the appropriate badge (using ui.v2Enabled to choose format for
"disabled"), with a safe default for unknown/new statuses so future title format
changes won't silently fail.
In `@package.json`:
- Around line 130-132: The package currently lists "ink", "react", and
"solid-js" as runtime dependencies ("ink", "react", "solid-js"), which increases
install footprint for consumers; if these UI libraries are only used for local
CLI/TUI or development, move them from "dependencies" to "devDependencies" in
package.json, otherwise leave them in "dependencies" if the package exposes
ink/React/Solid components as its public API—update package.json accordingly and
adjust any build/publish scripts that rely on those packages being present at
runtime.
- Line 53: The "test:vitest" npm script currently hardcodes
--max-old-space-size=12288 which can exhaust CI runners; change the script to
read the heap size from an environment variable (e.g.,
VITEST_MAX_OLD_SPACE_SIZE) with a sensible default (like 4096) instead of the
fixed 12288, and add a short note in project docs/README documenting the env var
and recommended CI memory requirement; update the package.json "test:vitest"
entry and documentation references accordingly.
In `@runtime/opentui/account-workspace.ts`:
- Around line 299-316: The detail.title is emitted raw in
buildOpenTuiAccountDetailPanel which can allow ANSI/control bytes; sanitize it
before returning by passing detail.title through sanitizeTerminalText (or
equivalent sanitizer) and use the sanitized value as the title (with a sensible
fallback if the sanitizer returns empty) so the returned object’s title is
always safe.
In `@runtime/opentui/app.ts`:
- Around line 753-760: The dashboard and backend draft signals
(savedDashboardSettings, savedBackendConfig) and the initial theme are being
seeded from defaults; instead initialize them from persisted snapshots passed
through OpenTuiBootstrapAppProps (e.g., use props.persistedDashboardSnapshot and
props.persistedPluginSnapshot) when available, so replace
cloneDashboardSettings(DEFAULT_DASHBOARD_DISPLAY_SETTINGS) and
cloneBackendPluginConfig(getDefaultPluginConfig()) with clones of the provided
persisted snapshots, and call applyUiThemeFromDashboardSettings(...) with the
persisted dashboard settings (fallback to defaults only if snapshots are
absent); also add a vitest regression that mounts the embedded drawer path and
verifies that opening the shell does not overwrite persisted theme or
plugin/dash drafts before user saves.
In `@runtime/opentui/index.ts`:
- Around line 3-8: Wrap the top-level await call to startOpenTuiBootstrap in a
try/catch so bootstrap errors are handled: call startOpenTuiBootstrap({
renderer: { exitOnCtrlC: true, targetFps: 30 } }) inside a try block, catch any
thrown error, log the error with useful details (error.message and/or
error.stack) via your logging mechanism or console.error, and exit the process
with a non-zero code (e.g., process.exit(1)) to signal failure; this change
should be applied around the startOpenTuiBootstrap invocation to ensure
tty/bootstrap failures are reported cleanly.
In `@scripts/run-bun-tests.js`:
- Around line 28-38: The script uses spawnSync to run bun but only checks
result.status, so launch failures (missing binary, permission denied, shell
errors) set result.error and are silently lost; update scripts/run-bun-tests.js
to explicitly check result.error after spawnSync (inspect the result variable
produced from spawnSync(bunCommand, bunArgs,...)) and, if present, write a
diagnostic (console.error or process.stderr.write) including bunCommand, bunArgs
and result.error.message/stack, then exit with a non-zero status; also add a
regression test that simulates a launch-failure (missing bun binary or
permission denied) to assert the script prints the error and exits non-zero.
In `@test/codex-manager-cli-manage.test.ts`:
- Around line 280-298: The file defines unused helper functions createDeferred
and makeErrnoError that were copy-pasted from another test; either remove these
two functions from test/codex-manager-cli-manage.test.ts if they are not used,
or extract them into a shared test utility module (e.g., test/utils) and replace
the local definitions with imports; update any tests that need them to import {
createDeferred, makeErrnoError } from the shared module and ensure exported
names match the originals.
In `@test/codex-manager-cli-noninteractive.test.ts`:
- Around line 280-298: The two helper functions createDeferred and
makeErrnoError in the test file are unused; either remove their dead definitions
or add tests that exercise them: if removing, delete the createDeferred and
makeErrnoError functions and any related unused imports/comments; if keeping,
add unit tests that call createDeferred to create a deferred promise,
resolve/reject it and assert behavior, and add a test that uses makeErrnoError
to construct an error with a code and assert the error.code and message—locate
the definitions by the function names createDeferred and makeErrnoError to
update or remove them.
In `@test/tui/account-details.test.tsx`:
- Around line 88-122: Add a concurrency regression case to the existing OpenTUI
account detail test by simulating rapid navigation inputs and asserting focus
consistency: after creating the harness with mountOpenTuiShellHarness (same
renderer settings) use harness.mockInput to fire multiple quick arrow events
(e.g., repeated mockInput.pressArrow("down") and "up") in rapid succession, call
await harness.renderOnce() between bursts, capture frames with
harness.captureCharFrame() and verify the focused detail remains correct using
findLine for expected focused account strings and compact row assertions; also
include one Windows-specific rendering assertion (e.g., one additional
captureCharFrame verification after a simulated resize or different renderer
height) and ensure cleanup with await harness.destroy() in the existing
try/finally.
- Line 1: The test imports the testing API from bun:test but our project
requires vitest; replace the import of describe, expect, test from "bun:test"
with the same named imports from "vitest" and ensure any bun-specific test
utilities in this file (e.g., describe, test, expect usages) follow vitest
semantics; update any setup/teardown that relied on bun-specific hooks to the
vitest equivalents if present (look for describe/test/expect in this file to
locate all usages).
In `@test/tui/account-workspace.test.tsx`:
- Around line 97-106: Tests use brittle exact substring assertions against the
full captured frame from harness.captureCharFrame(); update them to assert only
the key data points using partial matchers or regexes (e.g., check presence of
emails like "beta@ex.com", statuses like "[act]" or "[ok]", and quota numbers
such as "5h100" or "7d100") instead of full-line exact matches, and keep the
harness.destroy cleanup as-is; replace the four expect(...).toContain(...)
checks with a few expect(frame).toMatch(/.../) or
expect(frame).toContain(substring) calls that target the specific tokens so
formatting/spacing changes won't break the test.
- Around line 1-3: The test imports are using Bun's test runner; replace the
bun:test imports with vitest equivalents so the file uses vitest for
deterministic tests — change the import statement that currently reads import {
describe, expect, test } from "bun:test" to import { describe, expect, test }
from "vitest" (and update any other test files that import from "bun:test");
ensure the rest of the file continues to call describe/test/expect as before
(e.g., in test/tui/account-workspace.test.tsx referencing
buildAuthDashboardViewModel and DEFAULT_DASHBOARD_DISPLAY_SETTINGS remains
unchanged).
In `@test/tui/app-shell.test.tsx`:
- Around line 40-46: The test currently uses a single Promise.resolve() after
calling harness.mockInput.pressKey("q") which is brittle; instead, wait
deterministically for the shell/renderer teardown before asserting. Replace the
microtask wait with an explicit await that blocks until the expected condition
is true (for example, await a helper or vitest's waitFor(() =>
harness.exitReasons.includes("quit")) or waitFor(() =>
harness.renderer.isDestroyed === true)), then assert harness.keyNames,
harness.exitReasons, destroyCalls, and harness.renderer.isDestroyed; target the
call site around harness.mockInput.pressKey, harness.exitReasons, destroyCalls,
and harness.renderer.isDestroyed so the test reliably waits for exit/destroy
rather than a single microtask.
- Around line 1-2: Replace bun:test imports with vitest (import { describe,
expect, test, beforeEach, afterEach, vi } from "vitest") and convert any
bun-specific APIs to vitest equivalents; ensure mountOpenTuiShellHarness is used
with deterministic setup/teardown in beforeEach/afterEach and call its explicit
teardown/destroy method instead of relying on await Promise.resolve() (or use a
utility like waitFor/flushPromises to await actual async work) so the renderer
is destroyed before asserting the quit path; update tests to use vi mocks for
concurrency/token-refresh regressions and add explicit cases covering concurrent
operations and Windows filesystem behavior per guidelines.
In `@test/tui/bootstrap-boundaries.test.ts`:
- Around line 4-6: The tests duplicate the createMockStream helper; extract that
function into a shared test utility module (e.g., export createMockStream from a
new test utility file) and update both test files to import and use the shared
createMockStream instead of their local copies; ensure the exported helper
signature matches NodeJS.ReadStream & NodeJS.WriteStream and adjust imports in
the tests to reference the new utility (locate usages of createMockStream in the
tests to replace the local function with the imported one).
- Line 1: The test imports the Bun test runner; replace it with Vitest by
changing the import to bring describe, test, and expect from "vitest" (i.e.,
update the import in the test module that currently imports from "bun:test"),
and then ensure any Bun-specific APIs in this file (e.g., timeout handling or
globals) are adjusted to Vitest equivalents so the file uses Vitest's
describe/test/expect semantics and remains deterministic per guidelines.
In `@test/tui/cleanup.test.tsx`:
- Around line 52-60: The test currently assumes a single microtask (await
Promise.resolve()) is enough for escape-driven shutdown; this is racy. Replace
that microtask wait with a deterministic wait for shutdown by awaiting
harness.renderer.isDestroyed (polling) or, better, add/use a harness helper like
harness.waitForExit() that resolves when the exit flow completes, then perform
assertions on getShellListenerCounts, harness.clock.getActiveTimerCount(), and
harness.exitReasons; update the test to call harness.mockInput.pressEscape()
followed by await harness.waitForExit() (or loop until
harness.renderer.isDestroyed) before asserting listener and timer counts so the
test is deterministic across environments.
- Around line 1-2: The test file test/tui/cleanup.test.tsx is using bun:test and
a microtask-only yield (await Promise.resolve()) which breaks project guidance
and causes nondeterministic concurrency behavior; change imports to vitest
(import { describe, expect, test, vi } from "vitest") and replace the microtask
yield at the location that follows
mountOpenTuiShellHarness()/getShellListenerCounts() (the Promise.resolve() on
line ~53) with a deterministic async wait such as vitest's waitFor (or the
harness-provided async flush/settle helper) to poll until
getShellListenerCounts() stabilizes or a timeout occurs; ensure any test
setup/teardown uses vitest lifecycle hooks if needed so the regression suite
runs deterministically under the CI test runner.
In `@test/tui/harness.tsx`:
- Around line 22-40: createTrackedShellClock currently records timer ids
(activeTimers, nextTimerId) but never stores or invokes callbacks so scheduled
work never runs; fix by changing createTrackedShellClock to store callback+delay
entries keyed by the returned id (e.g., a Map from id to {cb, ms, repeat}), make
setInterval accept and save the callback and interval ms, have clearInterval
remove the stored entry, and expose a deterministic advance/tick helper (e.g.,
advance(ms) or tickOnce()) that iterates the stored callbacks and invokes them
in order while respecting repeats by re-scheduling or keeping them active;
update any tests to call the new tick/advance helper to drive timer-driven shell
behavior for deterministic vitest tests.
In `@test/tui/navigation.test.tsx`:
- Around line 4-69: Add regression tests to navigation.test.tsx that reproduce
concurrency and race conditions by (1) a test using mountOpenTuiShellHarness
where you fire many rapid inputs in tight sequence via
harness.mockInput.pressArrow/pressTab without awaiting renderOnce between some
presses, then call await harness.renderOnce() and assert
harness.selectionChanges, harness.renderer.currentFocusedRenderable, and
captureCharFrame remain consistent (use readyContexts[0]?.accountListRef and
navRef to check focus); (2) a test that simulates a token refresh race by
stubbing or triggering the harness/token manager refresh callback during
navigation (interleave a simulated token refresh event with mockInput presses
and await renderOnce) and assert focus and selectionChanges are stable; and (3)
a Windows-specific behavior test by temporarily setting process.platform =
"win32" (and restoring it) before mountOpenTuiShellHarness, exercising input
handling and asserting the same focus/selection invariants. Ensure each test
cleans up with await harness.destroy().
- Line 1: The test imports the Bun test runner ("bun:test") instead of Vitest;
update the import to use Vitest by replacing the module specifier "bun:test"
with "vitest" and keep the same named imports (describe, expect, test), then run
the test to ensure no Bun-specific APIs are used elsewhere in
navigation.test.tsx and adjust any Bun-only assertions or globals to Vitest
equivalents (e.g., replace bun-specific timers or APIs) so the file conforms to
the project's Vitest-based testing guidelines.
In `@test/tui/opentui-auth-login-smoke.test.ts`:
- Around line 1-5: The test currently imports test utilities from "bun:test"
which violates the guideline to use vitest; update the import to pull afterEach,
describe, expect, and test from "vitest" instead of "bun:test" (e.g., replace
the import line that references bun:test), and ensure any bun-specific APIs are
replaced with the standard vitest functions used in this file (references:
afterEach, describe, expect, test, and the test file
opentui-auth-login-smoke.test.ts).
- Around line 46-53: The fixture path resolution in loadFixtureDashboard
currently uses process.cwd(), which can fail when tests run from a non-repo-root
working directory; update loadFixtureDashboard to compute the fixture directory
relative to the test file instead (use import.meta.url or a __dirname
equivalent), e.g. derive the directory via fileURLToPath(new URL('.',
import.meta.url)) or a local __dirname and pass that into path.resolve when
reading "test/fixtures/v3-storage.json" so the readFileSync call always targets
the fixture next to the repo rather than the current working directory.
In `@test/ui-ink-auth-shell.test.ts`:
- Around line 137-141: The test uses brittle fixed delays and redundant optional
chaining on app; replace the timing-based waits with a deterministic wait for a
known render signal (e.g., use waitFor or waitForElementToBeRemoved / findByText
from your test utilities to wait for specific DOM content or a completed state)
instead of setTimeout, and remove the optional chaining on app.unmount() and
app.cleanup() since expect(app).not.toBeNull() guarantees app is present; update
the teardown to call app.unmount() and app.cleanup() after the deterministic
wait.
In `@test/ui-ink-dashboard.test.ts`:
- Around line 147-148: The test's timing-based wait (await new Promise(resolve
=> setTimeout(resolve, 30))) before calling input.push("2") can lose the
keypress if Ink hasn't mounted or subscribed to stdin; replace the fragile
timeout with a polling/wait helper that checks the CLI render state (e.g., wait
until the test's rendered output contains a specific string) before calling
input.push("2") or, if polling is not feasible, increase the delay and add a
comment; locate the call to input.push("2") in test/ui-ink-dashboard.test.ts and
modify the surrounding code (the short setTimeout block) to await a helper like
waitForOutput/awaitRender that inspects the component's output buffer or stdout
until the expected prompt appears, then send input.
- Around line 10-24: The createMockInput and createMockOutput helpers are
duplicated across tests; extract both functions into a single shared helper
module (e.g., export them from a mockStreams module), preserve their signatures
and the PassThrough/NodeJS.ReadStream typing and isTTY/setRawMode/ref/unref
behavior, update the test files to import these helpers instead of declaring
them inline, and ensure TypeScript types and exports are correct so existing
tests compile unchanged.
In `@test/ui-ink-detail-flows.test.ts`:
- Around line 120-126: The test is pushing DELETE characters into the input
stream in a tight loop which can race with the Ink input handler; update the
loop that pushes keys (the code that iterates over
["D","E","L","E","T","E","\r"] and calls input.push) to either await a small
delay between each push (e.g., await new Promise(r => setTimeout(r, 1-10))) or
call a provided helper that flushes/waits for the input buffer to drain after
each push; ensure this change is applied where the test constructs the rapid key
sequence so resultPromise is not raced.
In `@test/ui-ink-settings.test.ts`:
- Around line 213-234: Add a regression test that simulates failures from
saveDashboardDisplaySettingsMock and savePluginConfigMock and verifies
configureInkUnifiedSettings handles them without crashing: use
mockRejectedValueOnce on saveDashboardDisplaySettingsMock and
savePluginConfigMock, drive the save flow via the same input/typing sequence
used in the existing test for configureInkUnifiedSettings, await the
resultPromise, and assert the function returns a handled outcome (e.g., resolves
with false or true per app semantics), that the mocks were called as expected,
and that an error message was emitted to stderr (or appropriate logger) so
concurrent/save-failure scenarios are covered.
In `@vitest.config.ts`:
- Around line 19-22: The vitest configuration currently excludes the test/tui/**
tree which removes those regression tests from the deterministic Vitest gate;
restore that pattern to the gate by removing 'test/tui/**' from the exclude
array (or explicitly adding it to the include array) so TUI regression tests run
under Vitest alongside other auth/dashboard/settings tests, ensuring
concurrency/token-refresh/windows regressions remain part of the deterministic
test run.
---
Outside diff comments:
In `@lib/request/response-handler.ts`:
- Around line 85-87: The debug log in response-handler.ts currently calls
logRequest("stream-full", { fullContent: fullText }) and may leak tokens/PII;
update the code around LOGGING_ENABLED/ logRequest to sanitize fullText before
logging by implementing a redaction/truncation utility (e.g.,
redactSensitive(text) or truncateAndRedact(text)) and pass the sanitized value
to logRequest instead of raw fullText, ensuring patterns for tokens/keys/emails
are removed or masked and very long payloads are truncated; keep the original
variable names (fullText, LOGGING_ENABLED, logRequest, "stream-full") so changes
are minimal and easy to locate.
In `@lib/request/stream-failover.ts`:
- Around line 1-238: The PR added helper functions but lacks unit tests for
normalizeRequestInstanceId, readChunkWithTimeout, and isStallTimeoutError; add
focused tests that exercise edge-cases: (1) normalizeRequestInstanceId should
return null for undefined, empty, and whitespace-only inputs and should truncate
inputs longer than MAX_REQUEST_INSTANCE_ID_LENGTH (validate length and preserved
prefix) using the function name normalizeRequestInstanceId; (2)
readChunkWithTimeout should reject with a StallTimeoutError when the read
promise doesn't resolve within timeout and must clear the timer (use a mock read
Promise that never resolves and assert the rejected error is a StallTimeoutError
and that timers are cleaned up) referencing readChunkWithTimeout and
StallTimeoutError; and (3) isStallTimeoutError should return true for a
StallTimeoutError instance and objects with isStallTimeout === true and false
otherwise (use isStallTimeoutError). Add these unit tests in
test/stream-failover.test.ts (or a new unit test file) to complement the
existing integration tests.
- Around line 175-177: normalizeRequestInstanceId currently only truncates to 64
chars and relies on callers to mask secrets; update its docstring
(normalizeRequestInstanceId in lib/request/stream-failover.ts) to clearly state
callers must sanitize/redact tokens/emails before passing requestInstanceId, and
then add a regression test that exercises the failover marker generation (the
code that builds markerLabel near the failoverAttempt usage) to assert
token-like strings are rejected or redacted in output; implement the test by
passing a requestInstanceId containing a token-like pattern and verifying the
produced failover marker does not contain the raw token (e.g., contains a
redaction placeholder or stripped secret) so future changes won’t leak secrets.
In `@test/codex-manager-cli.test.ts`:
- Around line 1415-1518: The current test only inspects quickSwitchNumber
metadata and doesn't exercise the real OpenTUI shell, so add a deterministic
vitest that calls the actual CLI entry (runCodexMultiAuthCli) and drives the
OpenTUI shell to perform a quick-switch by sending the digit key press into the
real prompt flow (the same code path used by promptOpenTuiAuthDashboard/OpenTUI
runtime), then await the shell to fully exit and assert it completed without
extra renders or lingering teardown work referenced in runtime/opentui/app.ts
(teardown around the render loop). Use vitest utilities to fake timers and
stable inputs (mock network/token refresh deterministically and use fixed
timestamps like Date.now), drive the shell input programmatically to press the
quick-switch digit, and assert the process exits cleanly (no extra
renders/callbacks) and exit code is as expected to reproduce and prevent the
teardown race.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6b3a016-ade2-45a3-a902-7d28eeb34f61
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
.husky/pre-commitbunfig.tomleslint.config.jslib/cli.tslib/codex-manager.tslib/codex-manager/auth-ui-controller.tslib/codex-manager/settings-hub.tslib/codex-manager/settings-ui-controller.tslib/request/response-handler.tslib/request/stream-failover.tslib/storage.tslib/ui-ink/auth-shell.tslib/ui-ink/bootstrap.tslib/ui-ink/dashboard.tslib/ui-ink/detail-flows.tslib/ui-ink/index.tslib/ui-ink/layout.tslib/ui-ink/settings.tslib/ui/auth-menu.tspackage.jsonruntime/opentui/account-workspace.tsruntime/opentui/app.tsruntime/opentui/bootstrap.tsruntime/opentui/build.tsruntime/opentui/index.tsruntime/opentui/prompt.tsscripts/run-bun-tests.jstest/auth-menu-hotkeys.test.tstest/auth-ui-controller.test.tstest/cli-auth-menu.test.tstest/codex-manager-cli-manage.test.tstest/codex-manager-cli-noninteractive.test.tstest/codex-manager-cli.test.tstest/storage.test.tstest/tui/account-details.test.tsxtest/tui/account-workspace.test.tsxtest/tui/app-shell.test.tsxtest/tui/bootstrap-boundaries.test.tstest/tui/cleanup.test.tsxtest/tui/harness.tsxtest/tui/navigation.test.tsxtest/tui/opentui-auth-login-smoke.test.tstest/tui/settings-drawer.test.tsxtest/ui-ink-auth-shell.test.tstest/ui-ink-bootstrap.test.tstest/ui-ink-dashboard.test.tstest/ui-ink-detail-flows.test.tstest/ui-ink-settings.test.tstsconfig.jsontsconfig.runtime-opentui.jsonvitest.config.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.tstest/tui/account-details.test.tsxtest/ui-ink-settings.test.tstest/ui-ink-detail-flows.test.tstest/tui/account-workspace.test.tsxtest/tui/cleanup.test.tsxtest/ui-ink-bootstrap.test.tstest/tui/app-shell.test.tsxtest/codex-manager-cli-manage.test.tstest/ui-ink-dashboard.test.tstest/tui/bootstrap-boundaries.test.tstest/ui-ink-auth-shell.test.tstest/codex-manager-cli-noninteractive.test.tstest/auth-menu-hotkeys.test.tstest/tui/opentui-auth-login-smoke.test.tstest/cli-auth-menu.test.tstest/auth-ui-controller.test.tstest/tui/harness.tsxtest/tui/settings-drawer.test.tsxtest/tui/navigation.test.tsxtest/codex-manager-cli.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/ui-ink/index.tslib/ui-ink/bootstrap.tslib/request/stream-failover.tslib/request/response-handler.tslib/codex-manager/settings-ui-controller.tslib/ui-ink/detail-flows.tslib/ui-ink/layout.tslib/codex-manager/settings-hub.tslib/cli.tslib/ui-ink/dashboard.tslib/storage.tslib/ui/auth-menu.tslib/ui-ink/settings.tslib/ui-ink/auth-shell.tslib/codex-manager.tslib/codex-manager/auth-ui-controller.ts
🔇 Additional comments (38)
lib/request/stream-failover.ts (1)
90-105: lgtm on the soft/hard timeout retry logicthe pattern of reusing
readPromiseat line 103 with the remaining budget (hardTimeoutMs - softTimeoutMs) is correct - the original read is still pending and gets a second chance before we give up.concurrency assumption is documented. no filesystem io here, so windows edge cases don't apply.
lib/request/response-handler.ts (3)
11-16: type definitions look correctthe
MinimalReadableStreamReaderandStreamReadResulttypes properly subset the reader interface for timeout operations. same shapes exist inlib/request/stream-failover.ts:16-17- consolidation recommended (noted in other file).
144-167: readWithTimeout signature update is consistentthe updated signature at
lib/request/response-handler.ts:145-147correctly uses the new type aliases. behavior unchanged - timeout race pattern is standard.unlike
lib/request/stream-failover.ts:31, this function lacks explicit concurrency/platform documentation in the jsdoc. consider adding for consistency.
1-195: test coverage exists and is comprehensivetest/response-handler.test.ts provides full vitest coverage for convertSseToJson, ensureContentType, and isEmptyResponse. readWithTimeout is tested indirectly through stream stall timeout scenarios, and parseSseStream is validated through SSE parsing test cases. all affected functions are imported, tested, and cited in the test suite.
> Likely an incorrect or invalid review comment.test/auth-menu-hotkeys.test.ts (1)
201-219: good deterministic hotkey regression.this keeps the new account-detail mapping under vitest in
test/auth-menu-hotkeys.test.ts:201-219and asserts the concrete outputs without tty-dependent rendering.lib/storage.ts (1)
2039-2050: logic simplification looks correct.the chained deduplication
deduplicateAccountsByEmail(deduplicateAccounts(merged))correctly removes duplicates by key first, then by email. the max-accounts check against the final deduplicated list is the right boundary to enforce.test coverage at
test/storage.test.ts:219now usesACCOUNT_LIMITS.MAX_ACCOUNTS + 1which aligns with this check. as per coding guidelines,lib/**changes should cite affected tests - this is covered.test/storage.test.ts (1)
5-5: using the constant is better than a magic number.replacing hardcoded
21withACCOUNT_LIMITS.MAX_ACCOUNTS + 1ensures this test stays aligned if the limit ever changes. deterministic and uses vitest as required.Also applies to: 219-219
test/cli-auth-menu.test.ts (1)
278-301: good coverage for non-tty fallback paths.tests at
test/cli-auth-menu.test.ts:278-301exercise the readline fallback whenisTTYreturns false. the re-prompt test at line 290 validates the retry loop for invalid input. deterministic with vitest.test/codex-manager-cli-noninteractive.test.ts (1)
444-501: test exercises non-interactive guard correctly.the test at
test/codex-manager-cli-noninteractive.test.ts:444validates that interactive dashboard prompts are skipped whenisTTYis false. the assertions at lines 495-500 confirm no ui prompts were called. good coverage for the non-interactive path.test/codex-manager-cli-manage.test.ts (1)
444-604: good coverage for manage mode operations.tests at
test/codex-manager-cli-manage.test.ts:444-604cover:
- account deletion with storage persistence check at line 475-478
- toggle enabled state with assertion at line 507
- refresh flow with oauth mocks at lines 541-565
- reset all with clearAccountsMock assertion at line 601
the assertions verify both the operation and storage mutation. deterministic with vitest.
lib/ui-ink/bootstrap.ts (2)
22-29: host-managed ui detection is well-structured.
isHostManagedUiatlib/ui-ink/bootstrap.ts:22checks multiple env vars for embedded contexts (codex desktop, electron, etc). theFORCE_INTERACTIVE_MODEescape hatch at line 23 allows overriding for testing. good defensive design.
61-85: ink render wiring is correct
startInkAuthShellatlib/ui-ink/bootstrap.ts:61follows the standard ink pattern:render(createElement(Component, props), renderOptions). the null return when bootstrap unsupported allows callers to fall back gracefully. allRenderOptionsfields (stdin, stdout, stderr, debug, patchConsole, exitOnCtrlC) map correctly to ink 5.x types.runtime/opentui/build.ts (2)
1-20: the import atruntime/opentui/build.ts:3is valid.@opentui/solid/bun-pluginis correctly exported via the package's exports map. the build script setup is sound, including windows edge case handling inscripts/run-bun-tests.js:13withprocess.platform === "win32"and shell spawning. no changes needed.
13-18: log.message is the correct property per bun api.the
BuildMessageandResolveMessagetypes both have amessage: stringfield. the code at runtime/opentui/build.ts:13-18 will correctly access the message without any silent undefined issues.> Likely an incorrect or invalid review comment.test/tui/settings-drawer.test.tsx (1)
1-3: this file is correctly using bun:test per repo config.vitest.config.ts explicitly excludes
test/tui/**, which means all test/tui/** files—including settings-drawer.test.tsx—are intentionally run under bun:test, not vitest. all other test/** files use vitest. the drawer regressions are aligned with their configured test runner. no change needed.lib/ui-ink/settings.ts (1)
1724-1767: surface transient save failures to the user in the settings drawer.lib/ui-ink/settings.ts:1727-1766 awaits
persistDashboardSettingsSelection()andpersistBackendConfigSelection()without surfacing save failures. the persistence layer silently catches EBUSY, 429, and other retryable errors, logs a warning to console, and returns a fallback—but the ui never tells the user the save failed. add explicit error handling and ui feedback so users know to retry. test/tui/settings-drawer.test.tsx:72-192 has no coverage for failure modes; add tests for both EBUSY and 429 scenarios with expected ui prompts.test/tui/account-details.test.tsx (1)
14-43: mock token values look fine, but verify no real secrets leak.
test/tui/account-details.test.tsx:14-43uses placeholder tokens like"refresh-alpha"and"access-alpha". these appear to be intentionally fake, which is correct. ensure no real token patterns are used in tests.as per coding guidelines: "reject changes that mock real secrets or skip assertions."
lib/codex-manager/settings-hub.ts (3)
2004-2044: command resolution refactor looks clean.
lib/codex-manager/settings-hub.ts:2004-2044refactors the settings hub to useresolveSettingsHubCommandfor routing. the command-based dispatch at lines 2008-2043 is cleaner than direct action type checks. no auth tokens or emails are logged.
2047-2065: exports look complete.
lib/codex-manager/settings-hub.ts:2047-2065exports the newpersistOpenTuiSettingsSavealong with existing helpers. the public api surface is well-organized.
949-977: ebusy/429 handling is already implemented and tested downstream—no changes needed.
lib/codex-manager/settings-persistence.tsalready handles the scenarios you flagged:
- EBUSY, EPERM, EAGAIN, ENOTEMPTY, EACCES are in RETRYABLE_SETTINGS_WRITE_CODES (line 12)
- 429 rate-limiting is checked at line 46
- withQueuedRetry implements exponential backoff with up to 4 attempts (lines 90-106)
- persistDashboardSettingsSelection and persistBackendConfigSelection both use withQueuedRetry (lines 123, 145)
test/settings-hub-utils.test.ts confirms coverage: EBUSY/EPERM/EAGAIN retry tests pass (lines 93–107+), 429 rate-limiting with retryAfterMs is tested, and both persistence functions have regression tests (lines 249, 267). logging via formatPersistError only outputs error.message—no token or email leaks.
lib/ui-ink/auth-shell.ts (2)
73-94: entries mapping handles account data safely.
lib/ui-ink/auth-shell.ts:73-94builds shell entries from dashboard data. line 80 usesaccount.emailfor display, which is appropriate for ui rendering. no sensitive data (tokens, refresh tokens) is exposed or logged.
117-142: focus reducer is deterministic and handles edge cases.
lib/ui-ink/auth-shell.ts:117-142implementsreduceAuthInkShellFocuswith proper wrap-around behavior for section and entry navigation. the normalization at line 122 ensures valid state even with malformed input. this is solid.test/ui-ink-bootstrap.test.ts (2)
1-6: correct test framework and helper.
test/ui-ink-bootstrap.test.ts:1-6uses vitest as required and provides a clean mock stream helper. the type cast at line 5 is acceptable for testing purposes.
8-35: good bootstrap boundary coverage but consider windows edge cases.
test/ui-ink-bootstrap.test.ts:8-35covers key scenarios: non-tty rejection and host-managed ui boundaries. assertions are clear and meaningful.consider adding a test case for windows-specific tty detection behavior, as windows terminals may report
isTTYdifferently in some edge cases (e.g., conpty, mintty).as per coding guidelines: "demand regression cases that reproduce... windows filesystem behavior."
test/tui/bootstrap-boundaries.test.ts (1)
8-35: test cases are comprehensive for opentui bootstrap.
test/tui/bootstrap-boundaries.test.ts:8-35mirrors the ink bootstrap tests but targetsresolveOpenTuiAuthShellBootstrap. having both is reasonable since they test different bootstrap paths. the tests are deterministic.lib/ui-ink/index.ts (1)
1-6: barrel file looks correct.
lib/ui-ink/index.ts:1-6consolidates exports from six submodules. this is a clean pattern for public api organization. no runtime logic or auth concerns here.note:
export *can cause collisions if submodules export identically named symbols. verify no naming conflicts exist across layout, auth-shell, bootstrap, dashboard, detail-flows, and settings modules.test/ui-ink-dashboard.test.ts (1)
109-155: tests look deterministic and well-structured.the filter and quick-switch resolution tests at lines 110-130 are synchronous and deterministic. the live input test at lines 132-154 exercises the prompt flow end-to-end. good coverage for the new Ink dashboard surface.
test/tui/account-workspace.test.tsx (1)
119-142: input simulation flow looks solid.the filter-and-quick-switch test properly sequences keypresses, renders, and verifies both the filtered frame and the resulting action/selection state. the
try/finallycleanup pattern at lines 103-105 and 140-142 ensures the harness is destroyed even on failure.test/ui-ink-auth-shell.test.ts (1)
153-178: focus navigation tests are deterministic and thorough.the focus state tests at
test/ui-ink-auth-shell.test.ts:153-178verify section movement, entry navigation, and reset behavior synchronously without timing dependencies. solid coverage for the new focus reducer.test/tui/opentui-auth-login-smoke.test.ts (1)
85-105: smoke test structure is reasonable for integration validation.the test at
test/tui/opentui-auth-login-smoke.test.ts:86-105verifies the shell renders and signals readiness in interactive conditions. the afterEach cleanup ensures TTY descriptors are restored even on failure.test/ui-ink-detail-flows.test.ts (1)
59-84: parameterized key mapping tests are well-structured.the
it.eachpattern attest/ui-ink-detail-flows.test.ts:60-84cleanly covers all key-to-action mappings with minimal duplication. good use of vitest's table-driven test feature.lib/ui/auth-menu.ts (2)
85-93: sanitizeTerminalText cleanup is correct.the regex patterns at
lib/ui/auth-menu.ts:87-88properly match ANSI escape sequences and control characters. the split into named patterns improves readability over the previous inline replacements.
623-628: action mapping from detail model is clean.
lib/ui/auth-menu.ts:624-628mapsdetail.actionsdirectly to menu items, eliminating the previous hardcoded action array. this makes the menu dynamically driven by the view model.test/ui-ink-settings.test.ts (2)
236-280: good coverage for numeric editing and category toggles.
test/ui-ink-settings.test.ts:236-280verifies the +, -, and ] hotkeys modify config values correctly. the assertion at lines 274-279 validates both the toggle and the arithmetic adjustment, ensuring the hotkeys work as expected.
75-92: test isolation looks solid.
test/ui-ink-settings.test.ts:76-92properly resets modules and mocks in beforeEach, and restores UI runtime state in afterEach. this prevents test pollution across the suite.lib/codex-manager.ts (3)
3559-3627: handleManageAction type change looks correct.
lib/codex-manager.ts:3559-3627now takesLoginMenuResultinstead of the previous type. the function body correctly handles the various result shapes (switchAccountIndex, deleteAccountIndex, toggleAccountIndex, refreshAccountIndex).
84-98: new imports wire up opentui and screen state correctly.
lib/codex-manager.ts:84-98adds imports forbuildAuthDashboardScreenState,persistOpenTuiSettingsSave, andpromptOpenTuiAuthDashboard. these align with the PR's goal of migrating to Ink/OpenTUI.
3697-3712: fallback chain correctly handles unsupported vs user cancel.the null coalescing at
lib/codex-manager.ts:3697-3712works correctly.promptOpenTuiAuthDashboardreturnsnullonly when opentui is unsupported (runtime/opentui/prompt.ts:94-95), and returns{ mode: "cancel" }on user exit (runtime/opentui/prompt.ts:128). same pattern inpromptInkAuthDashboard—returnsnullonly when ink is unsupported (lib/ui-ink/dashboard.ts:634-635), returns result objects for all user actions including cancel. the fallback chain properly uses null to signal "renderer unavailable, try next" and result objects to signal "user made a choice, stop the chain." test coverage confirms this: test/codex-manager-cli.test.ts:1615-1623 verifies that when opentui returns null, ink is called and handles the user's cancel result.
| { | ||
| files: ["runtime/**/*.ts"], | ||
| languageOptions: { | ||
| parser: tsparser, | ||
| parserOptions: { | ||
| ecmaVersion: "latest", | ||
| sourceType: "module", | ||
| project: "./tsconfig.runtime-opentui.json", | ||
| }, | ||
| }, | ||
| plugins: { | ||
| "@typescript-eslint": tseslint, | ||
| }, | ||
| rules: { | ||
| "@typescript-eslint/no-explicit-any": "error", | ||
| "@typescript-eslint/no-unused-vars": ["error", { argsIgnorePattern: "^_" }], | ||
| "@typescript-eslint/explicit-function-return-type": "off", | ||
| "@typescript-eslint/no-non-null-assertion": "warn", | ||
| "@typescript-eslint/no-floating-promises": "error", | ||
| "@typescript-eslint/no-misused-promises": "error", | ||
| "@typescript-eslint/await-thenable": "error", | ||
| "@typescript-eslint/require-await": "warn", | ||
| "no-console": "off", | ||
| "prefer-const": "error", | ||
| "no-var": "error", | ||
| "eqeqeq": ["error", "always"], | ||
| "no-duplicate-imports": "error", | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd . runtime -e ts -e tsx
python - <<'PY'
from pathlib import Path
eslint = Path("eslint.config.js").read_text()
runtime_tsconfig = Path("tsconfig.runtime-opentui.json").read_text()
print("eslint has runtime ts glob:", '"runtime/**/*.ts"' in eslint)
print("eslint has runtime tsx glob:", '"runtime/**/*.tsx"' in eslint)
print("runtime tsconfig includes tsx:", '"runtime/**/*.tsx"' in runtime_tsconfig)
PYRepository: ndycode/codex-multi-auth
Length of output: 336
add runtime/**/*.tsx to the eslint override.
the eslint config at lines 42–69 matches only runtime/**/*.ts, but tsconfig.runtime-opentui.json includes runtime/**/*.tsx. no .tsx files exist in runtime/ today, but this config mismatch means any future .tsx entrypoints will skip the async/promise safety rules (no-floating-promises, await-thenable, etc.). keep the config aligned.
suggested fix
{
- files: ["runtime/**/*.ts"],
+ files: ["runtime/**/*.ts", "runtime/**/*.tsx"],
languageOptions: {
parser: tsparser,
parserOptions: {
ecmaVersion: "latest",
sourceType: "module",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| files: ["runtime/**/*.ts"], | |
| languageOptions: { | |
| parser: tsparser, | |
| parserOptions: { | |
| ecmaVersion: "latest", | |
| sourceType: "module", | |
| project: "./tsconfig.runtime-opentui.json", | |
| }, | |
| }, | |
| plugins: { | |
| "@typescript-eslint": tseslint, | |
| }, | |
| rules: { | |
| "@typescript-eslint/no-explicit-any": "error", | |
| "@typescript-eslint/no-unused-vars": ["error", { argsIgnorePattern: "^_" }], | |
| "@typescript-eslint/explicit-function-return-type": "off", | |
| "@typescript-eslint/no-non-null-assertion": "warn", | |
| "@typescript-eslint/no-floating-promises": "error", | |
| "@typescript-eslint/no-misused-promises": "error", | |
| "@typescript-eslint/await-thenable": "error", | |
| "@typescript-eslint/require-await": "warn", | |
| "no-console": "off", | |
| "prefer-const": "error", | |
| "no-var": "error", | |
| "eqeqeq": ["error", "always"], | |
| "no-duplicate-imports": "error", | |
| }, | |
| { | |
| files: ["runtime/**/*.ts", "runtime/**/*.tsx"], | |
| languageOptions: { | |
| parser: tsparser, | |
| parserOptions: { | |
| ecmaVersion: "latest", | |
| sourceType: "module", | |
| project: "./tsconfig.runtime-opentui.json", | |
| }, | |
| }, | |
| plugins: { | |
| "@typescript-eslint": tseslint, | |
| }, | |
| rules: { | |
| "@typescript-eslint/no-explicit-any": "error", | |
| "@typescript-eslint/no-unused-vars": ["error", { argsIgnorePattern: "^_" }], | |
| "@typescript-eslint/explicit-function-return-type": "off", | |
| "@typescript-eslint/no-non-null-assertion": "warn", | |
| "@typescript-eslint/no-floating-promises": "error", | |
| "@typescript-eslint/no-misused-promises": "error", | |
| "@typescript-eslint/await-thenable": "error", | |
| "@typescript-eslint/require-await": "warn", | |
| "no-console": "off", | |
| "prefer-const": "error", | |
| "no-var": "error", | |
| "eqeqeq": ["error", "always"], | |
| "no-duplicate-imports": "error", | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eslint.config.js` around lines 42 - 69, The override currently targets only
"runtime/**/*.ts" and should include TSX so the TypeScript parser and rules
(parser: tsparser, parserOptions.project: "./tsconfig.runtime-opentui.json")
apply to .tsx entrypoints too; update the files array in this override (the
object that defines plugins "@typescript-eslint" and rules like
"@typescript-eslint/no-floating-promises", "@typescript-eslint/await-thenable",
etc.) to include "runtime/**/*.tsx" alongside "runtime/**/*.ts" so future .tsx
files get the same async/promise safety and linting.
| if (isNonInteractiveMode()) { | ||
| existingStorage = null; | ||
| } else { |
There was a problem hiding this comment.
non-interactive guard placement may cause unintended behavior.
lib/codex-manager.ts:3640-3642 sets existingStorage = null in non-interactive mode, which skips the while loop at line 3643. however, this placement inside the if (existingStorage && existingStorage.accounts.length > 0) block means it only triggers when accounts exist.
if the intent is to skip all interactive prompts in non-interactive mode, the guard should be at the top of the flow or the comment should clarify this is intentional:
if (existingStorage && existingStorage.accounts.length > 0) {
if (isNonInteractiveMode()) {
+ // skip dashboard menu in non-interactive mode; proceed directly to OAuth
existingStorage = null;
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isNonInteractiveMode()) { | |
| existingStorage = null; | |
| } else { | |
| if (isNonInteractiveMode()) { | |
| // skip dashboard menu in non-interactive mode; proceed directly to OAuth | |
| existingStorage = null; | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 3640 - 3642, The non-interactive guard
(isNonInteractiveMode()) currently sits inside the conditional that checks
existingStorage && existingStorage.accounts.length > 0, so existingStorage is
only nulled when accounts exist; move the isNonInteractiveMode() check to the
start of the flow (before evaluating existingStorage and before the while loop
that prompts for selection) so non-interactive mode always skips interactive
prompts, or alternatively add a clarifying comment if the current behavior is
intentional; update references around existingStorage, isNonInteractiveMode(),
and the while prompt loop to reflect the new guard placement.
| void persistOpenTuiSettingsSave(event).catch((error: unknown) => { | ||
| console.warn( | ||
| `OpenTUI settings save failed: ${error instanceof Error ? error.message : String(error)}`, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
swallowed promise rejection in settings save callback.
lib/codex-manager.ts:3700-3704 uses void cast and .catch() to handle settings save errors with a console.warn. while this prevents unhandled rejections, it also means settings persistence failures are silently downgraded to warnings.
consider surfacing save failures to the user via the statusMessage field so they know their settings weren't persisted:
onSettingsSave: (event) => {
- void persistOpenTuiSettingsSave(event).catch((error: unknown) => {
+ persistOpenTuiSettingsSave(event).catch((error: unknown) => {
console.warn(
`OpenTUI settings save failed: ${error instanceof Error ? error.message : String(error)}`,
);
+ // optionally set a status message visible to user
});
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 3700 - 3704, The current call to
persistOpenTuiSettingsSave(event) swallows failures by only console.warn-ing in
the .catch; instead, update the error handler to surface the failure to the user
by setting the relevant statusMessage (e.g., this.statusMessage or the
manager/state object that holds statusMessage) with a clear, user-facing message
including the error details, and still log the warning for diagnostics; locate
the call to persistOpenTuiSettingsSave(event) and replace the silent .catch
handler so it sets statusMessage to something like "Failed to save OpenTUI
settings: <error message>" (include error instanceof Error ? error.message :
String(error)) while preserving existing logging.
| function resolveManagedAccountIndex(account: Pick<AuthAccountViewModel, "index" | "sourceIndex">): number { | ||
| const sourceIndex = | ||
| typeof account.sourceIndex === "number" && Number.isFinite(account.sourceIndex) | ||
| ? Math.max(0, Math.floor(account.sourceIndex)) | ||
| : undefined; | ||
| if (typeof sourceIndex === "number") return sourceIndex; | ||
| if (typeof account.index === "number" && Number.isFinite(account.index)) { | ||
| return Math.max(0, Math.floor(account.index)); | ||
| } | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
use only stable storage indices for manage actions.
in lib/codex-manager/auth-ui-controller.ts:528-534, index is rewritten to the visible row number. lib/codex-manager/auth-ui-controller.ts:193-203 then falls back to that field and also coerces arbitrary finite numbers with Math.floor() / Math.max(). once sourceIndex is missing, negative, or fractional, delete / toggle / refresh can hit the wrong saved account instead of taking the warning path. please require a non-negative integer sourceIndex here and add a vitest regression for the bad-index case.
proposed fix
-function resolveManagedAccountIndex(account: Pick<AuthAccountViewModel, "index" | "sourceIndex">): number {
- const sourceIndex =
- typeof account.sourceIndex === "number" && Number.isFinite(account.sourceIndex)
- ? Math.max(0, Math.floor(account.sourceIndex))
- : undefined;
- if (typeof sourceIndex === "number") return sourceIndex;
- if (typeof account.index === "number" && Number.isFinite(account.index)) {
- return Math.max(0, Math.floor(account.index));
- }
+function resolveManagedAccountIndex(account: Pick<AuthAccountViewModel, "sourceIndex">): number {
+ if (Number.isInteger(account.sourceIndex) && account.sourceIndex >= 0) {
+ return account.sourceIndex;
+ }
return -1;
}Also applies to: 528-534
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager/auth-ui-controller.ts` around lines 193 - 203,
resolveManagedAccountIndex currently accepts and coerces account.index as a
fallback which lets negative/fractional or UI-derived row indices be used for
storage operations; change resolveManagedAccountIndex to require a non-negative
integer sourceIndex only (i.e., return sourceIndex only when typeof
account.sourceIndex === "number", Number.isFinite(account.sourceIndex), and
Number.isInteger(account.sourceIndex) && account.sourceIndex >= 0) and remove
using/coercing account.index as a fallback so delete/toggle/refresh cannot
operate on unstable UI row indices; also add a vitest regression that passes
missing, negative, and fractional sourceIndex values (and a UI-derived index
value) and asserts the controller takes the warning/abort path rather than
operating on a saved account.
| type StreamReader = Pick<ReadableStreamDefaultReader<Uint8Array>, "read" | "cancel" | "releaseLock">; | ||
| type StreamReadResult = Awaited<ReturnType<StreamReader["read"]>>; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
type aliases duplicate definition from response-handler.ts
lib/request/response-handler.ts:11-16 defines MinimalReadableStreamReader and StreamReadResult with identical shapes. consider exporting from one location to avoid drift.
♻️ proposed consolidation
export from response-handler.ts and import here, or create a shared lib/request/types.ts:
-type StreamReader = Pick<ReadableStreamDefaultReader<Uint8Array>, "read" | "cancel" | "releaseLock">;
-type StreamReadResult = Awaited<ReturnType<StreamReader["read"]>>;
+import type { MinimalReadableStreamReader, StreamReadResult } from "./response-handler.js";
+type StreamReader = MinimalReadableStreamReader;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/request/stream-failover.ts` around lines 16 - 17, The duplicated type
aliases StreamReader and StreamReadResult here should be consolidated with the
identical MinimalReadableStreamReader and StreamReadResult in
response-handler.ts; remove the local type defs and import the shared types
instead (either export MinimalReadableStreamReader/StreamReadResult from
response-handler.ts and import them into stream-failover.ts, or move both to a
new shared module like lib/request/types.ts and import from there), updating
references to StreamReader and StreamReadResult to use the exported/shared
symbols to prevent drift.
| function createMockInput(): NodeJS.ReadStream { | ||
| const stream = new PassThrough() as PassThrough & NodeJS.ReadStream & { | ||
| setRawMode: (value: boolean) => void; | ||
| ref: () => void; | ||
| unref: () => void; | ||
| }; | ||
| Object.defineProperty(stream, "isTTY", { | ||
| value: true, | ||
| configurable: true, | ||
| }); | ||
| stream.setRawMode = () => undefined; | ||
| stream.ref = () => undefined; | ||
| stream.unref = () => undefined; | ||
| return stream; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
consider extracting shared mock stream helpers to reduce duplication.
createMockInput() and createMockOutput() are duplicated verbatim in test/ui-ink-auth-shell.test.ts, test/ui-ink-detail-flows.test.ts, and here. extracting them to a shared test/helpers/mock-streams.ts would reduce maintenance burden.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ui-ink-dashboard.test.ts` around lines 10 - 24, The createMockInput and
createMockOutput helpers are duplicated across tests; extract both functions
into a single shared helper module (e.g., export them from a mockStreams
module), preserve their signatures and the PassThrough/NodeJS.ReadStream typing
and isTTY/setRawMode/ref/unref behavior, update the test files to import these
helpers instead of declaring them inline, and ensure TypeScript types and
exports are correct so existing tests compile unchanged.
| await new Promise((resolve) => setTimeout(resolve, 30)); | ||
| input.push("2"); |
There was a problem hiding this comment.
timing-based test may be flaky on slow runners or windows ci.
the 30ms delay at test/ui-ink-dashboard.test.ts:147 before pushing input could race with Ink's render cycle. if the component hasn't mounted or subscribed to stdin yet, the keypress is lost and the test hangs or times out.
consider using a polling helper that waits for a specific render state (e.g., output contains expected text) before pushing input, or increase the delay with a comment explaining the rationale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ui-ink-dashboard.test.ts` around lines 147 - 148, The test's
timing-based wait (await new Promise(resolve => setTimeout(resolve, 30))) before
calling input.push("2") can lose the keypress if Ink hasn't mounted or
subscribed to stdin; replace the fragile timeout with a polling/wait helper that
checks the CLI render state (e.g., wait until the test's rendered output
contains a specific string) before calling input.push("2") or, if polling is not
feasible, increase the delay and add a comment; locate the call to
input.push("2") in test/ui-ink-dashboard.test.ts and modify the surrounding code
(the short setTimeout block) to await a helper like waitForOutput/awaitRender
that inspects the component's output buffer or stdout until the expected prompt
appears, then send input.
| await new Promise((resolve) => setTimeout(resolve, 30)); | ||
| for (const key of ["D", "E", "L", "E", "T", "E", "\r"]) { | ||
| input.push(key); | ||
| } | ||
|
|
||
| await expect(resultPromise).resolves.toBe(true); | ||
| }); |
There was a problem hiding this comment.
rapid key sequence may race with input handler.
test/ui-ink-detail-flows.test.ts:121-123 pushes DELETE characters in a tight loop without delays between keys. depending on how the Ink component buffers input, some characters might be missed or coalesced.
consider adding a small delay between key pushes or using a helper that waits for the input buffer to drain:
await new Promise((resolve) => setTimeout(resolve, 30));
- for (const key of ["D", "E", "L", "E", "T", "E", "\r"]) {
- input.push(key);
- }
+ for (const key of ["D", "E", "L", "E", "T", "E", "\r"]) {
+ input.push(key);
+ await new Promise((resolve) => setTimeout(resolve, 5));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await new Promise((resolve) => setTimeout(resolve, 30)); | |
| for (const key of ["D", "E", "L", "E", "T", "E", "\r"]) { | |
| input.push(key); | |
| } | |
| await expect(resultPromise).resolves.toBe(true); | |
| }); | |
| await new Promise((resolve) => setTimeout(resolve, 30)); | |
| for (const key of ["D", "E", "L", "E", "T", "E", "\r"]) { | |
| input.push(key); | |
| await new Promise((resolve) => setTimeout(resolve, 5)); | |
| } | |
| await expect(resultPromise).resolves.toBe(true); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ui-ink-detail-flows.test.ts` around lines 120 - 126, The test is pushing
DELETE characters into the input stream in a tight loop which can race with the
Ink input handler; update the loop that pushes keys (the code that iterates over
["D","E","L","E","T","E","\r"] and calls input.push) to either await a small
delay between each push (e.g., await new Promise(r => setTimeout(r, 1-10))) or
call a provided helper that flushes/waits for the input buffer to drain after
each push; ensure this change is applied where the test constructs the rapid key
sequence so resultPromise is not raced.
| it("routes the full settings hub through Ink and does not save on cancel", async () => { | ||
| const input = createMockInput(); | ||
| const output = createMockOutput(); | ||
| const stderr = createMockOutput(); | ||
| const { DEFAULT_DASHBOARD_DISPLAY_SETTINGS } = await import("../lib/dashboard-settings.js"); | ||
| const { configureInkUnifiedSettings } = await import("../lib/ui-ink/index.js"); | ||
|
|
||
| const resultPromise = configureInkUnifiedSettings(DEFAULT_DASHBOARD_DISPLAY_SETTINGS, { | ||
| stdin: input, | ||
| stdout: output, | ||
| stderr, | ||
| patchConsole: false, | ||
| exitOnCtrlC: false, | ||
| }); | ||
|
|
||
| const typing = sendKeys(input as PassThrough, ["4", "2", "q", "q"]); | ||
| await expect(resultPromise).resolves.toBe(true); | ||
| await typing; | ||
|
|
||
| expect(saveDashboardDisplaySettingsMock).not.toHaveBeenCalled(); | ||
| expect(savePluginConfigMock).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
no tests for settings save failure scenarios.
the test at test/ui-ink-settings.test.ts:213-234 verifies that cancel doesn't save, but there's no test for what happens when saveDashboardDisplaySettingsMock or savePluginConfigMock rejects. consider adding a regression test for error handling:
it("handles save failures gracefully", async () => {
saveDashboardDisplaySettingsMock.mockRejectedValueOnce(new Error("EBUSY"));
// ... trigger save flow and verify error is handled
});As per coding guidelines: "demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ui-ink-settings.test.ts` around lines 213 - 234, Add a regression test
that simulates failures from saveDashboardDisplaySettingsMock and
savePluginConfigMock and verifies configureInkUnifiedSettings handles them
without crashing: use mockRejectedValueOnce on saveDashboardDisplaySettingsMock
and savePluginConfigMock, drive the save flow via the same input/typing sequence
used in the existing test for configureInkUnifiedSettings, await the
resultPromise, and assert the function returns a handled outcome (e.g., resolves
with false or true per app semantics), that the mocks were called as expected,
and that an error message was emitted to stderr (or appropriate logger) so
concurrent/save-failure scenarios are covered.
| include: ['test/**/*.test.ts'], | ||
| exclude: [ | ||
| 'node_modules/**', | ||
| 'test/tui/**', |
There was a problem hiding this comment.
keep test/tui/** in the vitest gate.
excluding this tree moves the migrated auth/dashboard/settings regressions onto a separate runner path. that weakens the deterministic test gate the rest of the auth coverage already uses in test/auth-menu-hotkeys.test.ts:201-219, and it makes the required windows and race reproductions easier to miss.
As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vitest.config.ts` around lines 19 - 22, The vitest configuration currently
excludes the test/tui/** tree which removes those regression tests from the
deterministic Vitest gate; restore that pattern to the gate by removing
'test/tui/**' from the exclude array (or explicitly adding it to the include
array) so TUI regression tests run under Vitest alongside other
auth/dashboard/settings tests, ensuring concurrency/token-refresh/windows
regressions remain part of the deterministic test run.
| function formatRelativeTime(timestamp: number | undefined): string { | ||
| if (!timestamp) return "never"; | ||
| const days = Math.floor((Date.now() - timestamp) / 86_400_000); | ||
| if (days <= 0) return "today"; | ||
| if (days === 1) return "yesterday"; | ||
| if (days < 7) return `${days}d ago`; | ||
| if (days < 30) return `${Math.floor(days / 7)}w ago`; | ||
| return new Date(timestamp).toLocaleDateString(); | ||
| } | ||
|
|
||
| function formatDate(timestamp: number | undefined): string { | ||
| if (!timestamp) return "unknown"; | ||
| return new Date(timestamp).toLocaleDateString(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "auth-ui-controller" --type fRepository: ndycode/codex-multi-auth
Length of output: 139
🏁 Script executed:
head -200 lib/codex-manager/auth-ui-controller.ts | tail -50Repository: ndycode/codex-multi-auth
Length of output: 1896
🏁 Script executed:
sed -n '168,181p' lib/codex-manager/auth-ui-controller.tsRepository: ndycode/codex-multi-auth
Length of output: 597
🏁 Script executed:
sed -n '590,610p' lib/codex-manager/auth-ui-controller.tsRepository: ndycode/codex-multi-auth
Length of output: 926
🏁 Script executed:
fd "auth-ui-controller" --type f -name "*.test.*" -o -name "*.spec.*"Repository: ndycode/codex-multi-auth
Length of output: 298
🏁 Script executed:
rg "formatRelativeTime|formatDate" --type ts --type jsRepository: ndycode/codex-multi-auth
Length of output: 1644
🏁 Script executed:
cat -n test/auth-ui-controller.test.tsRepository: ndycode/codex-multi-auth
Length of output: 9777
🏁 Script executed:
sed -n '595,600p' lib/codex-manager/auth-ui-controller.tsRepository: ndycode/codex-multi-auth
Length of output: 302
🏁 Script executed:
rg "formatRelativeTime|formatDate" test/ --type tsRepository: ndycode/codex-multi-auth
Length of output: 50
make the detail subtitle deterministic across windows and non-tty renders
lib/codex-manager/auth-ui-controller.ts:168-181 calls Date.now() and toLocaleDateString() which read the ambient clock and system locale/timezone. line 597 bakes those into the subtitle string directly, so the same addedAt timestamp will render differently across windows/linux or under different TZ settings, breaking ink snapshots and non-tty output.
inject now as a parameter and pin the date formatter to explicit locale/timezone:
proposed fix
+const DETAIL_DATE_FORMATTER = new Intl.DateTimeFormat("en-US", {
+ timeZone: "UTC",
+ year: "numeric",
+ month: "short",
+ day: "numeric",
+});
+
-function formatRelativeTime(timestamp: number | undefined): string {
+function formatRelativeTime(timestamp: number | undefined, now: number): string {
if (!timestamp) return "never";
- const days = Math.floor((Date.now() - timestamp) / 86_400_000);
+ const days = Math.floor((now - timestamp) / 86_400_000);
if (days <= 0) return "today";
if (days === 1) return "yesterday";
if (days < 7) return `${days}d ago`;
if (days < 30) return `${Math.floor(days / 7)}w ago`;
- return new Date(timestamp).toLocaleDateString();
+ return DETAIL_DATE_FORMATTER.format(new Date(timestamp));
}
function formatDate(timestamp: number | undefined): string {
if (!timestamp) return "unknown";
- return new Date(timestamp).toLocaleDateString();
+ return DETAIL_DATE_FORMATTER.format(new Date(timestamp));
}the test at test/auth-ui-controller.test.ts:245-259 passes timestamps to buildAuthAccountDetailViewModel but doesn't assert on the subtitle output. add a vitest with a fixed clock to verify deterministic formatting across different TZ environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager/auth-ui-controller.ts` around lines 168 - 181, The subtitle
formatting is non-deterministic because formatRelativeTime and formatDate call
Date.now() and toLocaleDateString() directly; change
formatRelativeTime(timestamp) and formatDate(timestamp) to accept a now
parameter (e.g., formatRelativeTime(timestamp, now)) and use toLocaleDateString
with explicit locale/timeZone options (e.g., 'en-US' and 'UTC' or a chosen fixed
zone) so formatting does not vary by host; update the caller
buildAuthAccountDetailViewModel to pass a deterministic now value (injected from
the view-model builder) and adjust tests to supply a fixed clock (use vitest
fake timers or a fixed timestamp) and add an assertion that the subtitle output
is stable across TZs.
| function buildUnresolvableAccountMessage( | ||
| account: Pick<AuthAccountViewModel, "index" | "email" | "accountId">, | ||
| ): string { | ||
| const label = account.email?.trim() || account.accountId?.trim() || `index ${account.index + 1}`; | ||
| return `Unable to resolve saved account for action: ${label}`; | ||
| } |
There was a problem hiding this comment.
sanitize the unresolvable-account warning too.
in lib/codex-manager/auth-ui-controller.ts:205-210, the warning label uses raw email / accountId values, while the normal title path in lib/codex-manager/auth-ui-controller.ts:183-191 strips terminal control text first. a bad stored identifier can still inject escape/control output when resolution fails. reuse sanitizeTerminalText() here and add a vitest around an escaped identifier.
proposed fix
function buildUnresolvableAccountMessage(
account: Pick<AuthAccountViewModel, "index" | "email" | "accountId">,
): string {
- const label = account.email?.trim() || account.accountId?.trim() || `index ${account.index + 1}`;
+ const label =
+ sanitizeTerminalText(account.email) ||
+ sanitizeTerminalText(account.accountId) ||
+ `index ${account.index + 1}`;
return `Unable to resolve saved account for action: ${label}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function buildUnresolvableAccountMessage( | |
| account: Pick<AuthAccountViewModel, "index" | "email" | "accountId">, | |
| ): string { | |
| const label = account.email?.trim() || account.accountId?.trim() || `index ${account.index + 1}`; | |
| return `Unable to resolve saved account for action: ${label}`; | |
| } | |
| function buildUnresolvableAccountMessage( | |
| account: Pick<AuthAccountViewModel, "index" | "email" | "accountId">, | |
| ): string { | |
| const label = | |
| sanitizeTerminalText(account.email) || | |
| sanitizeTerminalText(account.accountId) || | |
| `index ${account.index + 1}`; | |
| return `Unable to resolve saved account for action: ${label}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager/auth-ui-controller.ts` around lines 205 - 210, The
unresolvable-account message uses raw account.email/accountId and can leak
terminal control sequences; update buildUnresolvableAccountMessage to pass the
chosen label through sanitizeTerminalText() (reuse the same sanitization used in
the title path) before interpolating into the returned string, and add a Vitest
that simulates an account with escape/control characters in email or accountId
to assert the returned message has those sequences stripped/escaped.
| const title = | ||
| `${formatAccountTitle(account)} [${account.status ?? "unknown"}]` + | ||
| (account.enabled === false ? " [disabled]" : ""); | ||
| const statusLabel = account.status ?? "unknown"; |
There was a problem hiding this comment.
avoid rendering disabled twice.
in lib/codex-manager/auth-ui-controller.ts:590-593, disabled accounts already surface status === "disabled", so the title becomes ...[disabled] [disabled]. drop the extra suffix or guard it with statusLabel !== "disabled".
proposed fix
- const title =
- `${formatAccountTitle(account)} [${account.status ?? "unknown"}]` +
- (account.enabled === false ? " [disabled]" : "");
const statusLabel = account.status ?? "unknown";
+ const title =
+ `${formatAccountTitle(account)} [${statusLabel}]` +
+ (account.enabled === false && statusLabel !== "disabled" ? " [disabled]" : "");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const title = | |
| `${formatAccountTitle(account)} [${account.status ?? "unknown"}]` + | |
| (account.enabled === false ? " [disabled]" : ""); | |
| const statusLabel = account.status ?? "unknown"; | |
| const statusLabel = account.status ?? "unknown"; | |
| const title = | |
| `${formatAccountTitle(account)} [${statusLabel}]` + | |
| (account.enabled === false && statusLabel !== "disabled" ? " [disabled]" : ""); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager/auth-ui-controller.ts` around lines 590 - 593, The title
construction appends a " [disabled]" suffix even when account.status is already
"disabled", causing duplicate labels; update the logic that builds title
(variable title near formatAccountTitle and statusLabel) to only append "
[disabled]" when account.enabled === false AND (account.status ?? "unknown") !==
"disabled" (or equivalently statusLabel !== "disabled"), so the extra suffix is
not added when status already indicates disabled.
There was a problem hiding this comment.
5 issues found across 40 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="eslint.config.js">
<violation number="1" location="eslint.config.js:42">
P2: This block duplicates the rules and plugins defined in the base configuration above. Since `runtime/**/*.ts` was already added to the first block's files array, you can leverage configuration cascading to simplify this block so it only overrides the `parserOptions.project`.</violation>
</file>
<file name="package.json">
<violation number="1" location="package.json:51">
P1: Including OpenTUI tests in the main test script introduces an undocumented Bun requirement that will break standard Node CI pipelines.</violation>
</file>
<file name="lib/ui/auth-menu.ts">
<violation number="1" location="lib/ui/auth-menu.ts:87">
P3: This regex definition duplicates the `sanitizeTerminalText` implementation in `lib/codex-manager/auth-ui-controller.ts`.</violation>
<violation number="2" location="lib/ui/auth-menu.ts:609">
P2: String replacement on the entire title can corrupt account emails/labels that happen to contain the status strings.</violation>
</file>
<file name="test/codex-manager-cli.test.ts">
<violation number="1" location="test/codex-manager-cli.test.ts:148">
P2: The config module mock hardcodes duplicated default values instead of reusing real defaults, which can cause test drift and stale assertions when `lib/config.ts` changes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "lint:ts:fix": "eslint . --ext .ts --fix", | ||
| "lint:scripts:fix": "eslint scripts --ext .js --fix", | ||
| "test": "vitest run", | ||
| "test": "npm run test:opentui && npm run test:vitest", |
There was a problem hiding this comment.
P1: Including OpenTUI tests in the main test script introduces an undocumented Bun requirement that will break standard Node CI pipelines.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At package.json, line 51:
<comment>Including OpenTUI tests in the main test script introduces an undocumented Bun requirement that will break standard Node CI pipelines.</comment>
<file context>
@@ -39,14 +39,18 @@
"lint:ts:fix": "eslint . --ext .ts --fix",
"lint:scripts:fix": "eslint scripts --ext .js --fix",
- "test": "vitest run",
+ "test": "npm run test:opentui && npm run test:vitest",
+ "test:opentui": "node scripts/run-bun-tests.js",
+ "test:vitest": "node --max-old-space-size=12288 ./node_modules/vitest/vitest.mjs run --maxWorkers=1 --pool=forks --execArgv=--max-old-space-size=12288",
</file context>
| "test": "npm run test:opentui && npm run test:vitest", | |
| "test": "npm run test:vitest", |
| { | ||
| files: ["runtime/**/*.ts"], | ||
| languageOptions: { | ||
| parser: tsparser, | ||
| parserOptions: { | ||
| ecmaVersion: "latest", | ||
| sourceType: "module", | ||
| project: "./tsconfig.runtime-opentui.json", | ||
| }, | ||
| }, | ||
| plugins: { | ||
| "@typescript-eslint": tseslint, | ||
| }, | ||
| rules: { | ||
| "@typescript-eslint/no-explicit-any": "error", | ||
| "@typescript-eslint/no-unused-vars": ["error", { argsIgnorePattern: "^_" }], | ||
| "@typescript-eslint/explicit-function-return-type": "off", | ||
| "@typescript-eslint/no-non-null-assertion": "warn", | ||
| "@typescript-eslint/no-floating-promises": "error", | ||
| "@typescript-eslint/no-misused-promises": "error", | ||
| "@typescript-eslint/await-thenable": "error", | ||
| "@typescript-eslint/require-await": "warn", | ||
| "no-console": "off", | ||
| "prefer-const": "error", | ||
| "no-var": "error", | ||
| "eqeqeq": ["error", "always"], | ||
| "no-duplicate-imports": "error", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
P2: This block duplicates the rules and plugins defined in the base configuration above. Since runtime/**/*.ts was already added to the first block's files array, you can leverage configuration cascading to simplify this block so it only overrides the parserOptions.project.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At eslint.config.js, line 42:
<comment>This block duplicates the rules and plugins defined in the base configuration above. Since `runtime/**/*.ts` was already added to the first block's files array, you can leverage configuration cascading to simplify this block so it only overrides the `parserOptions.project`.</comment>
<file context>
@@ -39,6 +39,35 @@ export default [
"no-duplicate-imports": "error",
},
},
+ {
+ files: ["runtime/**/*.ts"],
+ languageOptions: {
</file context>
| { | |
| files: ["runtime/**/*.ts"], | |
| languageOptions: { | |
| parser: tsparser, | |
| parserOptions: { | |
| ecmaVersion: "latest", | |
| sourceType: "module", | |
| project: "./tsconfig.runtime-opentui.json", | |
| }, | |
| }, | |
| plugins: { | |
| "@typescript-eslint": tseslint, | |
| }, | |
| rules: { | |
| "@typescript-eslint/no-explicit-any": "error", | |
| "@typescript-eslint/no-unused-vars": ["error", { argsIgnorePattern: "^_" }], | |
| "@typescript-eslint/explicit-function-return-type": "off", | |
| "@typescript-eslint/no-non-null-assertion": "warn", | |
| "@typescript-eslint/no-floating-promises": "error", | |
| "@typescript-eslint/no-misused-promises": "error", | |
| "@typescript-eslint/await-thenable": "error", | |
| "@typescript-eslint/require-await": "warn", | |
| "no-console": "off", | |
| "prefer-const": "error", | |
| "no-var": "error", | |
| "eqeqeq": ["error", "always"], | |
| "no-duplicate-imports": "error", | |
| }, | |
| }, | |
| { | |
| files: ["runtime/**/*.ts"], | |
| languageOptions: { | |
| parserOptions: { | |
| project: "./tsconfig.runtime-opentui.json", | |
| }, | |
| }, | |
| }, |
| const statusLabel = account.status ?? "unknown"; | ||
| const subtitle = `Added: ${formatDate(account.addedAt)} | Used: ${formatRelativeTime(account.lastUsed)} | Status: ${statusLabel}`; | ||
| const detail = buildAuthAccountDetailViewModel(account); | ||
| const header = detail.title |
There was a problem hiding this comment.
P2: String replacement on the entire title can corrupt account emails/labels that happen to contain the status strings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/ui/auth-menu.ts, line 609:
<comment>String replacement on the entire title can corrupt account emails/labels that happen to contain the status strings.</comment>
<file context>
@@ -601,52 +598,34 @@ export async function showAuthMenu(
- const statusLabel = account.status ?? "unknown";
- const subtitle = `Added: ${formatDate(account.addedAt)} | Used: ${formatRelativeTime(account.lastUsed)} | Status: ${statusLabel}`;
+ const detail = buildAuthAccountDetailViewModel(account);
+ const header = detail.title
+ .replace(" [active]", ` ${statusBadge("active")}`)
+ .replace(" [ok]", ` ${statusBadge("ok")}`)
</file context>
| savePluginConfig: savePluginConfigMock, | ||
| }; | ||
| }); | ||
| vi.mock("../lib/config.js", () => ({ |
There was a problem hiding this comment.
P2: The config module mock hardcodes duplicated default values instead of reusing real defaults, which can cause test drift and stale assertions when lib/config.ts changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/codex-manager-cli.test.ts, line 148:
<comment>The config module mock hardcodes duplicated default values instead of reusing real defaults, which can cause test drift and stale assertions when `lib/config.ts` changes.</comment>
<file context>
@@ -140,14 +145,100 @@ vi.mock("../lib/dashboard-settings.js", () => ({
- savePluginConfig: savePluginConfigMock,
- };
-});
+vi.mock("../lib/config.js", () => ({
+ DEFAULT_PLUGIN_CONFIG: {
+ codexMode: true,
</file context>
|
|
||
| function sanitizeTerminalText(value: string | undefined): string | undefined { | ||
| if (!value) return undefined; | ||
| const ansiPattern = new RegExp("\\u001B\\[[0-?]*[ -/]*[@-~]", "g"); |
There was a problem hiding this comment.
P3: This regex definition duplicates the sanitizeTerminalText implementation in lib/codex-manager/auth-ui-controller.ts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/ui/auth-menu.ts, line 87:
<comment>This regex definition duplicates the `sanitizeTerminalText` implementation in `lib/codex-manager/auth-ui-controller.ts`.</comment>
<file context>
@@ -84,9 +84,11 @@ function mainMenuTitleWithVersion(): string {
function sanitizeTerminalText(value: string | undefined): string | undefined {
if (!value) return undefined;
+ const ansiPattern = new RegExp("\\u001B\\[[0-?]*[ -/]*[@-~]", "g");
+ const controlPattern = new RegExp("[\\u0000-\\u001F\\u007F]", "g");
return value
</file context>
1 participant
Summary
Verification
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 migrates the auth dashboard, account detail, recovery, and settings flows onto ink, adds the opentui shell app, and extracts a shared
settings-ui-controller.tsview-model seam — a substantial scope with clear architectural intent.key issues found:
promptInkChoiceandpromptInkTextConfirmindetail-flows.tsdiscard theInstancereturned byrender(), creating stdin lock and resource leaks if anything goes wrong beforeuseApp().exit()is called.renderInkDashboardOnceindashboard.tscorrectly keeps a reference and callsunmount()/cleanup()— the same pattern must be applied here.InkTextConfirmAppauto-submits on the final keystroke — the component resolvestruethe momentnextValue === confirmText(lines 231–232), bypassing the Enter gate the footer promises. on windows, paste events can deliver multiple characters in a single ink input event, triggering an accidental account deletion before the user confirms.lib/ui-ink/settings.tsre-declares the full settings constant surface (DASHBOARD_DISPLAY_OPTIONS,DashboardDisplaySettingKey, etc.) that is already exported fromlib/codex-manager/settings-ui-controller.ts. the two definitions invite divergence over time as either file changes.lib/codex-manager.tsimports from../runtime/opentui/— this reverses the conventionallib→runtimedependency direction, pulling the entire@opentui/solidchain into every test that mockscodex-manager. the adapter should sit behind the existinglib/ui-ink/index.tsboundary.Confidence Score: 2/5
InkTextConfirmAppcan trigger account deletion without explicit Enter confirmation (windows paste risk); the leaked render instance in allpromptInkChoice/promptInkTextConfirmcalls creates stdin lock and resource leaks with no cleanup path. these are not theoretical — they affect the primary auth deletion flow and need fixes before merge.lib/ui-ink/detail-flows.ts(instance leak + auto-submit),lib/ui-ink/settings.ts(constant duplication),lib/codex-manager.ts(dependency direction)Sequence Diagram
sequenceDiagram participant CM as codex-manager.ts participant OTP as runtime/opentui/prompt.ts participant IDB as ui-ink/dashboard.ts participant IDF as ui-ink/detail-flows.ts participant BS as ui-ink/bootstrap.ts participant SH as settings-hub.ts CM->>OTP: promptOpenTuiAuthDashboard(dashboard, onSettingsSave) OTP-->>CM: LoginMenuResult | null alt openTUI not supported CM->>IDB: promptInkAuthDashboard(dashboard) IDB->>BS: resolveInkAuthShellBootstrap() BS-->>IDB: { supported: true } IDB->>IDB: renderInkDashboardOnce() → Instance captured IDB->>IDF: promptInkAccountDetails(account) IDF->>IDF: render() ⚠️ Instance NOT captured IDF-->>IDB: InkAccountDetailAction IDB->>IDF: promptInkConfirmAccountDelete / Refresh IDF->>IDF: render() ⚠️ Instance NOT captured, auto-submits without Enter IDF-->>IDB: boolean | null IDB-->>CM: LoginMenuResult end alt settings save (OpenTUI) OTP->>SH: onSettingsSave(event) SH->>SH: persistOpenTuiSettingsSave(event) SH-->>OTP: void endLast reviewed commit: 236cefa