fix(gui): gate Codex pool writes until hydrated and ignore stale strategy polls - #739
Conversation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
|
This pull request currently targets @Wibias Please retarget this PR to Its title has been prefixed with This pull request is being kept as a draft automatically. Once every issue above is resolved, it will be marked ready for review again. |
There was a problem hiding this comment.
💡 Codex Review
Lines 139 to 141 in c57bfe4
Persist credits-only quota updates
When WHAM returns only resetCredits without usage windows, this early-return branch updates the in-memory quota but skips schedulePersistAccountQuotas(). If no unrelated quota mutation follows, a restart reloads the previous credit count from disk, so the dashboard can advertise already-consumed reset credits; schedule persistence after setting the credits-only snapshot as the other mutation paths do.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import CodexAccountPool from "../components/CodexAccountPool"; | ||
| import { codexAccountModeState, type CodexAccountModeState } from "../codex-multi-state"; | ||
| import { ensureOpenAiProvider, openAiAccountProviderState, OpenAiEnableError } from "../provider-payload"; | ||
| import { readSessionListCache, writeSessionListCache } from "../session-list-cache"; |
There was a problem hiding this comment.
When the Codex Auth page is included in the GUI build, this import cannot be resolved because a repo-wide search under gui/src finds no session-list-cache module. Consequently bun run build fails before the dashboard can be packaged; add the module in this commit or remove the dependency.
AGENTS.md reference: gui/AGENTS.md:L44-L50
Useful? React with 👍 / 👎.
| </span> | ||
| <select | ||
| <Select | ||
| id={strategySelectId} |
There was a problem hiding this comment.
For every GUI build, this passes an unsupported id prop because the existing Select signature has no id field, causing a TypeScript error. Even if transpiled without checking, Select never forwards the ID to its trigger, so the declared label association and ID-based consumers cannot find this control; extend Select to accept and apply the ID or avoid relying on it.
AGENTS.md reference: gui/AGENTS.md:L44-L50
Useful? React with 👍 / 👎.
| function barFillStyle(percent: number): CSSProperties { | ||
| return { ["--bar-scale" as string]: String(barWidth(percent) / 100) }; | ||
| } |
There was a problem hiding this comment.
For any displayed quota below 100%, this helper now sets only the custom property --bar-scale, but no stylesheet in the repository consumes that property and .bar-fill no longer receives a width. As a block element it therefore fills the entire track, making values such as 10% visually appear exhausted in both compact and stacked quota bars; set width directly or add the missing CSS consumer.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| onIncrement={() => onStickyDraftChange(clampNumberDraft(stickyDraft, 1, 1, 100))} | ||
| onDecrement={() => onStickyDraftChange(clampNumberDraft(stickyDraft, -1, 1, 100))} |
There was a problem hiding this comment.
When a user presses either new sticky-limit stepper button, these handlers only update stickyDraft; onStickyCommit remains attached solely to the numeric input's blur/Enter events. If the input was not focused, no commit occurs at all, and if it was focused its blur runs before the button click and commits the old value, leaving the UI showing a limit that was never sent to the management API. Commit the computed value from the stepper interaction or move commit handling to the whole control.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| if (credits === undefined) { | ||
| return ( | ||
| <span className="badge badge-muted codex-ticket-badge-slot" aria-hidden="true"> | ||
| <IconTicket width={12} />0 |
There was a problem hiding this comment.
When a valid hydrated quota omits the optional resetCredits field, this branch still renders a visible muted ticket with 0; the new class has no CSS that hides it. Providers commonly return usage windows without reset-credit data, so those accounts now falsely advertise zero reset credits indefinitely; reserve the placeholder only while account.quota itself is null and return nothing once a loaded quota lacks the field.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| for (const observer of observers) revisions.set(observer, observer.beginActiveRead()); | ||
| if (!refreshQuota) setLoadState("loading"); | ||
| // Soft refresh when boxes are already on screen — avoid full-page loading flash. | ||
| if (!refreshQuota && !hasAccountsRef.current) setLoadState("loading"); |
There was a problem hiding this comment.
When /accounts has successfully returned an empty list, hasAccountsRef remains false, so every 30-second background poll sets loadState back to loading. The new render guard then replaces the valid empty-state UI with the full cold-start skeleton until the request completes; use a separate “has loaded” flag rather than treating zero accounts as unhydrated.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| plan={main?.plan} | ||
| threshold={threshold} | ||
| t={t} | ||
| pending={main?.quota == null} |
There was a problem hiding this comment.
When no main Codex account exists, main is undefined and this condition passes pending={true} forever, even after the accounts request has completed and there is no credential whose quota could arrive. Previously the main card rendered no quota section in this state; only show the pending bars when a real main account exists and its quota is still being fetched.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| {state === null ? ( | ||
| <span className="badge badge-accent openai-account-mode-banner__badge-slot openai-account-mode-banner__badge-slot--pending" aria-hidden="true"> | ||
| {t("codexAuth.accountModePool")} | ||
| </span> |
There was a problem hiding this comment.
Whenever state is still null, this renders the real accented “Pool mode” badge rather than a neutral placeholder. A repo-wide CSS search finds no rule for openai-account-mode-banner__badge-slot--pending, so the badge is fully visible and, if /api/config fails, remains indefinitely even though the actual mode was never established; render a non-semantic skeleton or add the intended hiding treatment.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| <div key={index} className="quota-row quota-row--skeleton" aria-hidden="true"> | ||
| <span className="quota-skel quota-skel--label" /> | ||
| <span className="quota-skel quota-skel--reset" /> | ||
| <span className="quota-skel quota-skel--day" /> | ||
| <span className="quota-skel quota-skel--time" /> | ||
| <span className="quota-skel quota-skel--bar" /> | ||
| <span className="quota-skel quota-skel--val" /> |
There was a problem hiding this comment.
While quota is pending, both generated rows contain only empty spans, and none of the new quota-skel or quota-row--skeleton classes has a CSS definition anywhere in gui. The existing grid therefore gives these rows zero content height, so the supposedly reserved slot still expands when real labels and bars arrive and does not prevent the layout shift this code was added to address; add dimensions/display styling for the skeleton cells or reserve row height directly.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c57bfe4cb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "sub.moveUp": "Move {m} up", | ||
| "sub.moveDown": "Move {m} down", | ||
| "sub.removeAria": "Remove {m}", | ||
| "sub.workspace.allModels": "All models", |
There was a problem hiding this comment.
These sub.workspace.* keys, together with the newly restored usage.workspace.* and api.workspace.* blocks in every locale, violate the repository's explicit retired-workspace invariants. The focused subagents-classic.test.ts, usage-layout.test.ts, and apikeys-layout.test.ts checks now fail, so the required GUI test gate cannot pass; remove the retired keys from all six locale modules.
AGENTS.md reference: gui/AGENTS.md:L44-L50
Useful? React with 👍 / 👎.
| else if (existing?.resetCredits !== undefined) next.resetCredits = existing.resetCredits; | ||
|
|
||
| accountQuota.set(accountId, next); | ||
| schedulePersistAccountQuotas(); |
There was a problem hiding this comment.
This persistence call is unreachable when WHAM supplies a reset-credit-only snapshot because the creditsOnly branch returns earlier. In that scenario the new credit count is updated only in memory while codex-quota-cache.json retains its previous value, so a restart within the six-hour cache window restores stale or missing reset credits; schedule persistence before that early return and add a focused credits-only persistence regression.
AGENTS.md reference: src/AGENTS.md:L22-L25
Useful? React with 👍 / 👎.
| "storage.policy.threshold": "When archived size exceeds", | ||
| "storage.policy.trigger": "Trigger", | ||
| "storage.policy.target": "Cleanup target", | ||
| "storage.policy.targetPercent": "Remove oldest archived percent", | ||
| "storage.policy.targetReduce": "Reduce archived size to (GiB)", | ||
| "storage.policy.targetPercent": "Remove oldest archived", | ||
| "storage.policy.targetReduce": "Reduce archived size to", |
There was a problem hiding this comment.
On the existing Storage cleanup-policy form, these strings are rendered directly beside plain numeric inputs with no separate unit suffix. Removing (GiB) from the threshold and reduce-to labels and the percentage wording from the percent target leaves both sighted users and assistive-technology users unable to determine what values such as 5 mean; restore the units in every locale or update the controls to expose them explicitly.
AGENTS.md reference: gui/AGENTS.md:L33-L33
Useful? React with 👍 / 👎.
| }); | ||
| return activeOk; | ||
| } | ||
| if (!hasAccountsRef.current) setLoadState("error"); |
There was a problem hiding this comment.
After any successful non-empty load—or when the new module cache seeds rows—hasAccountsRef.current is true, so a later /accounts failure never changes loadState to error. The page therefore continues presenting stale account, pause, quota, and credential state as ready with no retry warning when the management API is unavailable; keep the soft-rendered rows if desired, but expose a stale/error state instead of silently suppressing the failure.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| import { existsSync, readFileSync, unlinkSync } from "node:fs"; | ||
| import { join } from "node:path"; |
There was a problem hiding this comment.
The new quota-cache runtime path directly imports node:fs and node:path even though this routing module has no Node-compatibility role. That makes every consumer of the shared quota store depend on the Node compatibility layer and violates the repository's Bun-native runtime boundary; use Bun/Web APIs or move the disk operations behind an existing Bun-native storage abstraction.
AGENTS.md reference: src/AGENTS.md:L7-L9
Useful? React with 👍 / 👎.
| useEffect(() => { | ||
| if (!readLastActive) return; | ||
| if (savingRef.current) return; | ||
| applyActivePayload(readLastActive()); | ||
| }, [readLastActive, applyActivePayload]); |
There was a problem hiding this comment.
In the Providers workspace the account controller mounts before the conditional Accounts tab, and load() snapshots its observer set when a request starts. If the user opens the Accounts tab while the first /active request is already in flight, this one-time readLastActive() call returns undefined and the new observer is absent from that request's snapshot; when the response completes nothing reruns this effect, leaving the strategy controls disabled until the next 30-second poll. Replay the last payload when it becomes available or notify subscribers added during an in-flight read.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codex/quota.ts (1)
125-166: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMissing hydrate-before-mutate: writers can silently wipe other accounts' cached quotas on disk.
hydrateAccountQuotasFromDisk()(lines 260-278) is module-private and only invoked fromgetAccountQuota/listAccountQuotas(297-305). The three mutators —setAccountQuotaFromParsed(125-167),updateAccountQuota(222-258), and the single-account branch ofclearAccountQuota(307-312) — never call it before mutatingaccountQuotaand schedulingschedulePersistAccountQuotas()(280-295), which serializes the entire in-memory map and overwrites the disk file.Failure mode: if any mutator runs before the first reader call since process start (e.g. a background quota refresh landing before any dashboard read), the debounced persist will overwrite
codex-quota-cache.jsonwith only the just-updated account(s), permanently losing every other account's previously cached quota bars until their next live refresh. It can also drop other fields of the same account (e.g. thecreditsOnlymerge branch insetAccountQuotaFromParsedrelies onexisting = accountQuota.get(accountId), which isundefinedpre-hydration even though disk has that data).Fix by hydrating at the top of each mutator (not inside the persist callback — hydrating there would resurrect entries
clearAccountQuota(accountId)just deleted, since the merge only skips keys already present in memory):🛠️ Proposed fix
export function setAccountQuotaFromParsed( accountId: string, quota: Omit<StoredAccountQuota, "updatedAt"> | null, ): void { if (!quota) return; + hydrateAccountQuotasFromDisk(); const existing = accountQuota.get(accountId);export function updateAccountQuota( accountId: string, weekly: unknown, weeklyResetAt?: unknown, monthly?: unknown, monthlyResetAt?: unknown, resetCredits?: number, ): void { + hydrateAccountQuotasFromDisk(); const existing = accountQuota.get(accountId);export function clearAccountQuota(accountId?: string): void { if (accountId) { + hydrateAccountQuotasFromDisk(); accountQuota.delete(accountId); schedulePersistAccountQuotas(); return; }Also applies to: 222-258, 260-295, 307-324
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex/quota.ts` around lines 125 - 166, Hydrate persisted quotas before any mutation by invoking hydrateAccountQuotasFromDisk at the start of setAccountQuotaFromParsed, updateAccountQuota, and clearAccountQuota, including the single-account deletion path. Ensure hydration completes before reading or changing accountQuota and before schedulePersistAccountQuotas can persist the full map, while preserving the existing deletion behavior for clearAccountQuota.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/src/clamp-draft.ts`:
- Around line 9-10: Update the parsing logic in the clamp-draft helper so empty
or whitespace-only raw input is treated as invalid and falls back to min before
applying the delta. Preserve the existing finite-number handling for non-empty
input and the current clamping behavior for valid values.
In `@gui/src/components/AccountPoolStrategyControls.tsx`:
- Around line 77-104: Unify the blocked-state interaction behavior for the
numeric input and NumberStepper controls. In
gui/src/components/AccountPoolStrategyControls.tsx lines 77-104, replace the
sticky-limit input’s disabled gating with the shared convention used by
CodexAutoSwitchSetting.tsx lines 88-117: keep it focusable with readOnly and
aria-disabled when disabled, while leaving CodexAutoSwitchSetting.tsx unchanged.
Ensure both visually matching controls provide consistent keyboard and
screen-reader behavior.
- Around line 55-71: Remove the unsupported id prop from the Select element in
AccountPoolStrategyControls, and remove the now-unused strategySelectId value if
it has no other references. Keep the existing label prop and adjacent span
unchanged unless cleanup requires eliminating the unused identifier.
In `@gui/src/components/codex-account-pool-cards.tsx`:
- Around line 100-107: Replace the locale-unsafe concatenated pause-button
aria-label in codex-account-pool-cards.tsx and codex-account-pool-main-card.tsx
with the shared codexAuth.resumeWithHint translation key, passing
t("codexAuth.pausedHint") as its hint interpolation; add the {hint}-based key to
all six locale files.
In `@gui/src/components/codex-account-pool-helpers.tsx`:
- Around line 62-68: Update the minCh width calculation near the pause, resume,
and savingLabel translations to account for CJK locales instead of relying
solely on UTF-16 string lengths with ch units. Preserve the existing
visible-label selection and ensure the reserved width is sufficient for ja, ko,
and zh translations to prevent clipping or reflow during toggles.
In `@gui/src/components/CodexAccountPool.tsx`:
- Around line 247-248: Remove the redundant nullish fallback from the autoSwitch
threshold assignment in CodexAccountPool, using autoSwitch.threshold directly
since the CodexAutoSwitchController threshold contract is a non-null number.
In `@gui/src/components/CodexPoolStrategySetting.tsx`:
- Around line 92-154: Defer concurrent active payloads instead of dropping them
in the observer flow around acceptActiveRead and applyActivePayload. Add a ref
to retain the latest payload received while saving, then reconcile and apply
that deferred value after putCodexPoolStrategy completes in save, clearing it
appropriately while preserving revision and local-save precedence behavior.
In `@gui/src/components/QuotaBars.tsx`:
- Around line 137-139: Update barFillStyle in QuotaBars.tsx so the .bar-fill
element’s width uses the computed --bar-scale value, adding width:
calc(var(--bar-scale) * 100%) or preserving an equivalent inline width. Ensure
all quota bars using barFillStyle render according to their percentage instead
of defaulting to full width.
In `@gui/src/hooks/useCodexAccountPool.ts`:
- Around line 217-229: Update the retry logic in the useEffect keyed by
needsQuotaFill so polling continues while credentialed accounts still lack
quota, rather than stopping after the fixed 350/900/2000ms timers; use a
self-rescheduling timeout chain with cleanup that stops on dependency changes,
disabled polling, pauseCount, or resolved quota.
- Around line 84-90: Mirror lastGoodByBase for the last-good /active payload:
add a module-scoped cache keyed by apiBase, initialize lastActiveRef from that
cache in useCodexAccountPool, and update it whenever a valid active payload is
accepted. Preserve readLastThreshold() and readLastActive() behavior while
ensuring CodexAutoSwitchSetting and CodexPoolStrategySetting retain hydrated
values across controller remounts.
- Around line 117-123: Update readLastThreshold in useCodexAccountPool.ts to
import and reuse extractAutoSwitchThresholdPayload from codex-auto-switch.ts
instead of duplicating its object-field extraction logic. Keep the existing
fallback behavior for values without an autoSwitchThreshold payload, and leave
hydrateServerValue unchanged.
In `@gui/tests/codex-auto-switch-controller.test.tsx`:
- Around line 219-312: Refactor the shared test setup around mountHarness to
support an optional unresolved initial /api/codex-auth/active response, allowing
callers to mount without flushing hydration. Reuse its existing setInterval,
clearInterval, and fetch-router logic in “blocks writes while /active is still
pending” by passing the deferred response through the new parameter or helper,
and remove the duplicated mock setup from that test.
---
Outside diff comments:
In `@src/codex/quota.ts`:
- Around line 125-166: Hydrate persisted quotas before any mutation by invoking
hydrateAccountQuotasFromDisk at the start of setAccountQuotaFromParsed,
updateAccountQuota, and clearAccountQuota, including the single-account deletion
path. Ensure hydration completes before reading or changing accountQuota and
before schedulePersistAccountQuotas can persist the full map, while preserving
the existing deletion behavior for clearAccountQuota.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bea0f121-57af-49f2-a18a-e92469486d10
📒 Files selected for processing (25)
gui/src/clamp-draft.tsgui/src/codex-auto-switch.tsgui/src/components/AccountPoolStrategyControls.tsxgui/src/components/CodexAccountPool.tsxgui/src/components/CodexAutoSwitchSetting.tsxgui/src/components/CodexPoolStrategySetting.tsxgui/src/components/NumberStepper.tsxgui/src/components/QuotaBars.tsxgui/src/components/codex-account-pool-cards.tsxgui/src/components/codex-account-pool-helpers.tsxgui/src/components/codex-account-pool-main-card.tsxgui/src/hooks/useCodexAccountPool.tsgui/src/hooks/useCodexAutoSwitch.tsgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/CodexAuth.tsxgui/src/styles/provider-quota.cssgui/tests/account-pool-strategy.test.tsxgui/tests/codex-account-auto-switch.test.tsxgui/tests/codex-auto-switch-controller.test.tsxsrc/codex/quota.ts
| const parsed = Number(raw); | ||
| const base = Number.isFinite(parsed) ? parsed : min; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Empty/whitespace draft isn't treated as "non-finite" — breaks the documented min-fallback contract.
Number("") is 0 (finite), so a cleared input is parsed as base 0 instead of falling back to min. For the current min=1 callers this happens to still clamp into range, but produces an off-by-one result relative to the documented behavior (clicking + on an empty field should act like starting from min, i.e. yield min + delta, not 0 + delta). Since this is a shared, general-purpose helper, please make empty/whitespace explicitly invalid.
🛠️ Proposed fix
export function clampNumberDraft(
raw: string,
delta: number,
min: number,
max: number,
step = 1,
): string {
- const parsed = Number(raw);
+ const trimmed = raw.trim();
+ const parsed = trimmed === "" ? NaN : Number(trimmed);
const base = Number.isFinite(parsed) ? parsed : min;📝 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 parsed = Number(raw); | |
| const base = Number.isFinite(parsed) ? parsed : min; | |
| const trimmed = raw.trim(); | |
| const parsed = trimmed === "" ? NaN : Number(trimmed); | |
| const base = Number.isFinite(parsed) ? parsed : min; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/clamp-draft.ts` around lines 9 - 10, Update the parsing logic in the
clamp-draft helper so empty or whitespace-only raw input is treated as invalid
and falls back to min before applying the delta. Preserve the existing
finite-number handling for non-empty input and the current clamping behavior for
valid values.
| <input | ||
| id={stickyInputId} | ||
| className="input mono codex-auto-switch-input" | ||
| type="number" | ||
| min={1} | ||
| max={100} | ||
| step={1} | ||
| inputMode="numeric" | ||
| value={stickyDraft} | ||
| disabled={disabled} | ||
| aria-label={t("accountPool.stickyLimitAria")} | ||
| onChange={(event) => onStickyDraftChange(event.target.value)} | ||
| onBlur={() => onStickyCommit()} | ||
| onKeyDown={(event) => { | ||
| if (event.nativeEvent.isComposing || disabled) return; | ||
| if (event.key === "Enter") { | ||
| event.preventDefault(); | ||
| onStickyCommit(); | ||
| } | ||
| }} | ||
| /> | ||
| <NumberStepper | ||
| disabled={disabled} | ||
| incrementLabel={t("accountPool.stickyLimitInc")} | ||
| decrementLabel={t("accountPool.stickyLimitDec")} | ||
| onIncrement={() => onStickyDraftChange(clampNumberDraft(stickyDraft, 1, 1, 100))} | ||
| onDecrement={() => onStickyDraftChange(clampNumberDraft(stickyDraft, -1, 1, 100))} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Unify the "blocked until hydrated/saving" gating mechanism for numeric stepper inputs. Both files add near-identical numeric input + NumberStepper controls in this PR, but gate interaction differently: one strips the element from the tab order (disabled), the other keeps it focusable but non-editable (readOnly + aria-disabled). This is the same requirement solved two different ways, causing inconsistent keyboard/screen-reader behavior between visually-matching controls.
gui/src/components/AccountPoolStrategyControls.tsx#L77-L104: switch the sticky-limit<input>fromdisabled={disabled}toreadOnly={disabled} aria-disabled={disabled}to match the auto-switch threshold input's pattern (or vice versa — pick one convention and apply it to both).gui/src/components/CodexAutoSwitchSetting.tsx#L88-L117: keep as-is if thisreadOnly+aria-disabledpattern is adopted as the shared convention; otherwise align to whichever mechanism is chosen.
📍 Affects 2 files
gui/src/components/AccountPoolStrategyControls.tsx#L77-L104(this comment)gui/src/components/CodexAutoSwitchSetting.tsx#L88-L117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/components/AccountPoolStrategyControls.tsx` around lines 77 - 104,
Unify the blocked-state interaction behavior for the numeric input and
NumberStepper controls. In gui/src/components/AccountPoolStrategyControls.tsx
lines 77-104, replace the sticky-limit input’s disabled gating with the shared
convention used by CodexAutoSwitchSetting.tsx lines 88-117: keep it focusable
with readOnly and aria-disabled when disabled, while leaving
CodexAutoSwitchSetting.tsx unchanged. Ensure both visually matching controls
provide consistent keyboard and screen-reader behavior.
| <button | ||
| type="button" | ||
| className={`btn btn-sm ${a.paused ? "btn-primary" : "btn-ghost"}`} | ||
| className="btn btn-sm btn-ghost codex-auth-action-btn" | ||
| onClick={() => onTogglePause(a)} | ||
| disabled={pauseBusy} | ||
| title={a.paused ? t("codexAuth.pausedHint") : undefined} | ||
| aria-label={a.paused ? `${t("codexAuth.resume")}. ${t("codexAuth.pausedHint")}` : t("codexAuth.pause")} | ||
| > |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Locale-unsafe aria-label concatenation repeated in two files. Both the pool-card and main-card pause/resume buttons build their aria-label by concatenating two separately-translated strings with a hardcoded ". " separator, which breaks down for locales with different clause-joining conventions.
gui/src/components/codex-account-pool-cards.tsx#L100-L107: replace`${t("codexAuth.resume")}. ${t("codexAuth.pausedHint")}`with a single interpolated key, e.g.t("codexAuth.resumeWithHint", { hint: t("codexAuth.pausedHint") }), and add that key (with a{hint}placeholder) to all six locale files.gui/src/components/codex-account-pool-main-card.tsx#L100-L113: apply the same replacement using the same new translation key for consistency between the pool-card and main-card pause buttons.
📍 Affects 2 files
gui/src/components/codex-account-pool-cards.tsx#L100-L107(this comment)gui/src/components/codex-account-pool-main-card.tsx#L100-L113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/components/codex-account-pool-cards.tsx` around lines 100 - 107,
Replace the locale-unsafe concatenated pause-button aria-label in
codex-account-pool-cards.tsx and codex-account-pool-main-card.tsx with the
shared codexAuth.resumeWithHint translation key, passing
t("codexAuth.pausedHint") as its hint interpolation; add the {hint}-based key to
all six locale files.
| const pause = t("codexAuth.pause"); | ||
| const resume = t("codexAuth.resume"); | ||
| const savingLabel = t("common.saving"); | ||
| const visible = saving ? savingLabel : paused ? resume : pause; | ||
| // Proportional fonts: ch is approximate, but avoids the height bugs of hidden | ||
| // multi-line measure stacks / ::before sizers. | ||
| const minCh = Math.max(pause.length, resume.length, savingLabel.length); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ch-based width reservation is a rough approximation for CJK locales.
minCh is derived from .length (UTF-16 code units) of the translated pause/resume/saving strings, but ch sizes to the "0" glyph width, which is much narrower than typical CJK glyphs. For ja/ko/zh locales this can under-reserve space, causing the label to clip or reflow briefly on toggle. This is already called out as an approximation in the inline comment, and the failure mode is cosmetic/self-recovering, so treating this as a nice-to-have rather than a blocker.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/components/codex-account-pool-helpers.tsx` around lines 62 - 68,
Update the minCh width calculation near the pause, resume, and savingLabel
translations to account for CJK locales instead of relying solely on UTF-16
string lengths with ch units. Preserve the existing visible-label selection and
ensure the reserved width is sufficient for ja, ko, and zh translations to
prevent clipping or reflow during toggles.
| const readLastThreshold = useCallback(() => { | ||
| const active = lastActiveRef.current?.value; | ||
| if (active && typeof active === "object" && active !== null && "autoSwitchThreshold" in active) { | ||
| return (active as { autoSwitchThreshold: unknown }).autoSwitchThreshold; | ||
| } | ||
| return active; | ||
| }, []); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate payload-extraction logic — reuse extractAutoSwitchThresholdPayload.
This reimplements extractAutoSwitchThresholdPayload from gui/src/codex-auto-switch.ts verbatim instead of importing it. useCodexAutoSwitch.ts's hydrateServerValue then re-applies the same extraction to the already-extracted value it receives from readLastThreshold() — harmless today (a number fails the typeof === "object" guard so it's a no-op), but it's duplicate logic that can silently diverge if the /active payload shape changes in only one place.
♻️ Reuse the shared helper
+import { extractAutoSwitchThresholdPayload } from "../codex-auto-switch";
+
/** Last threshold an actual read returned, or undefined when none has succeeded yet. */
- const readLastThreshold = useCallback(() => {
- const active = lastActiveRef.current?.value;
- if (active && typeof active === "object" && active !== null && "autoSwitchThreshold" in active) {
- return (active as { autoSwitchThreshold: unknown }).autoSwitchThreshold;
- }
- return active;
- }, []);
+ const readLastThreshold = useCallback(
+ () => extractAutoSwitchThresholdPayload(lastActiveRef.current?.value),
+ [],
+ );📝 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 readLastThreshold = useCallback(() => { | |
| const active = lastActiveRef.current?.value; | |
| if (active && typeof active === "object" && active !== null && "autoSwitchThreshold" in active) { | |
| return (active as { autoSwitchThreshold: unknown }).autoSwitchThreshold; | |
| } | |
| return active; | |
| }, []); | |
| import { extractAutoSwitchThresholdPayload } from "../codex-auto-switch"; | |
| /** Last threshold an actual read returned, or undefined when none has succeeded yet. */ | |
| const readLastThreshold = useCallback( | |
| () => extractAutoSwitchThresholdPayload(lastActiveRef.current?.value), | |
| [], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/hooks/useCodexAccountPool.ts` around lines 117 - 123, Update
readLastThreshold in useCodexAccountPool.ts to import and reuse
extractAutoSwitchThresholdPayload from codex-auto-switch.ts instead of
duplicating its object-field extraction logic. Keep the existing fallback
behavior for values without an autoSwitchThreshold payload, and leave
hydrateServerValue unchanged.
| test("blocks writes while /active is still pending", async () => { | ||
| const active = deferred<Response>(); | ||
| const activeResponses: Array<Promise<Response> | Response> = [active.promise]; | ||
| const putResponses: Array<Promise<Response> | Response> = []; | ||
| const writes: number[] = []; | ||
| let refreshCallback: (() => void) | null = null; | ||
|
|
||
| Object.defineProperty(testWindow, "setInterval", { | ||
| configurable: true, | ||
| value: (callback: () => void, delay?: number) => { | ||
| if (delay === 30_000) refreshCallback = callback; | ||
| return 1; | ||
| }, | ||
| }); | ||
| Object.defineProperty(testWindow, "clearInterval", { | ||
| configurable: true, | ||
| value: () => {}, | ||
| }); | ||
|
|
||
| const fetchRouter = async (input: string | URL | Request, init?: RequestInit): Promise<Response> => { | ||
| const url = typeof input === "string" ? input : input instanceof URL ? input.href : input.url; | ||
| const method = init?.method ?? (input instanceof Request ? input.method : "GET"); | ||
| if (url.endsWith("/api/codex-auth/accounts") && method === "GET") { | ||
| return Response.json({ accounts: [] }); | ||
| } | ||
| if (url.endsWith("/api/codex-auth/active") && method === "GET") { | ||
| const response = activeResponses.shift(); | ||
| if (response) return await response; | ||
| return Response.json({ | ||
| activeCodexAccountId: null, | ||
| autoSwitchThreshold: 55, | ||
| accountPoolStrategy: "quota", | ||
| accountPoolStickyLimit: 1, | ||
| }); | ||
| } | ||
| if (url.endsWith("/api/codex-auth/pool-strategy") && (method === "PUT" || method === "PATCH")) { | ||
| return Response.json({ | ||
| ok: true, | ||
| accountPoolStrategy: "quota", | ||
| accountPoolStickyLimit: 1, | ||
| }); | ||
| } | ||
| if (url.endsWith("/api/codex-auth/auto-switch") && method === "PUT") { | ||
| const body = JSON.parse(String(init?.body)) as { threshold: number }; | ||
| writes.push(body.threshold); | ||
| const response = putResponses.shift(); | ||
| if (!response) throw new Error("unexpected auto-switch write"); | ||
| return await response; | ||
| } | ||
| throw new Error(`unexpected fetch: ${method} ${url}`); | ||
| }; | ||
| Object.defineProperty(globalThis, "fetch", { configurable: true, value: fetchRouter }); | ||
|
|
||
| const container = document.createElement("div"); | ||
| document.body.append(container); | ||
| const { createRoot } = await import("react-dom/client"); | ||
| const root = createRoot(container); | ||
| mountedRoot = root; | ||
| await act(async () => { | ||
| root.render( | ||
| <LanguageProvider> | ||
| <CodexAccountPool apiBase="http://localhost" /> | ||
| </LanguageProvider>, | ||
| ); | ||
| await flush(); | ||
| }); | ||
|
|
||
| const toggle = container.querySelector<HTMLButtonElement>("button.toggle[aria-pressed]"); | ||
| expect(toggle).not.toBeNull(); | ||
| expect(toggle?.disabled).toBe(true); | ||
|
|
||
| await act(async () => { | ||
| toggle?.click(); | ||
| await flush(); | ||
| }); | ||
| expect(writes).toEqual([]); | ||
|
|
||
| await act(async () => { | ||
| active.resolve(Response.json({ | ||
| activeCodexAccountId: null, | ||
| autoSwitchThreshold: 55, | ||
| accountPoolStrategy: "quota", | ||
| accountPoolStickyLimit: 1, | ||
| })); | ||
| await flush(); | ||
| }); | ||
|
|
||
| const readyToggle = container.querySelector<HTMLButtonElement>("button.toggle[aria-pressed]"); | ||
| expect(readyToggle?.disabled).toBe(false); | ||
| expect(container.querySelector<HTMLInputElement>('input[aria-label="Switch threshold, percent"]')?.value).toBe("55"); | ||
| expect(writes).toEqual([]); | ||
| expect(refreshCallback).not.toBeNull(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract shared fetch/interval-mock setup instead of re-duplicating mountHarness's boilerplate.
This new test re-implements ~90 lines of setInterval/clearInterval overrides and the /api/codex-auth/{accounts,active,pool-strategy,auto-switch} fetch router that already exist almost verbatim in mountHarness (lines 108-216). The duplication exists because mountHarness always flushes the initial /active GET to completion before returning, so it can't be reused to assert on a still-pending hydration — but that's exactly a signal that mountHarness should accept an optional "don't await hydration" mode (e.g. an initialActiveResponse parameter accepting a Promise<Response> instead of a resolved default) rather than hand-rolling a second copy of the router. Two independent copies of this routing logic will drift silently — e.g. a future new endpoint added to one router but not the other creates false negatives/positives without an obvious diff signal.
♻️ Sketch of a parameterized `mountHarness`
-async function mountHarness(): Promise<Harness> {
- const activeResponses: Array<Promise<Response> | Response> = [
- Response.json({ activeCodexAccountId: null, autoSwitchThreshold: 80 }),
- ];
+async function mountHarness(
+ initialActive: Promise<Response> | Response = Response.json({ activeCodexAccountId: null, autoSwitchThreshold: 80 }),
+): Promise<Harness> {
+ const activeResponses: Array<Promise<Response> | Response> = [initialActive];Then the "blocks writes while /active is still pending" test becomes:
const active = deferred<Response>();
const harness = await mountHarnessWithoutFlushingHydration(active.promise); // or similar🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/tests/codex-auto-switch-controller.test.tsx` around lines 219 - 312,
Refactor the shared test setup around mountHarness to support an optional
unresolved initial /api/codex-auth/active response, allowing callers to mount
without flushing hydration. Reuse its existing setInterval, clearInterval, and
fetch-router logic in “blocks writes while /active is still pending” by passing
the deferred response through the new parameter or helper, and remove the
duplicated mock setup from that test.
|
@coderabbitai review |
c57bfe4 to
f6f9c9c
Compare
|
✅ Action performedReview finished.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
f6f9c9c to
f70c3e7
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
…tegy polls Keep auto-switch / rotation controls from overwriting server state before /active settles, and drop shared /active reads that started before a PUT.
46e2c95 to
297ceef
Compare
Merge-ready summaryReview follow-ups from CodeRabbit/Codex that were necessary or useful are addressed. Remaining bot notes are either already fixed on tip, intentionally deferred (e.g. versioned alias family for pathological Merge order (bottom-up): #738 → #739 → #740 → #741. After each merge, retarget the next PR to Codex cloud review is currently usage-capped on this account; local/CI verification was used instead for later rounds. This PR (#739)
Merge-ready after #738 (then retarget to |
Summary
/activehydrates (defaults still paint for CLS)/activepolls that started before a pool-strategy PUT (revision + saving guards)Stack
stack/gui-catalog-cold-load) — fix(gui): gate Codex pool writes until hydrated and ignore stale strategy polls #739Test plan
bun test gui/tests/account-pool-strategy.test.tsx gui/tests/codex-auto-switch-controller.test.tsx gui/tests/codex-account-auto-switch.test.tsx/activeafter strategy change does not roll UI back/activeSummary by CodeRabbit
New Features
Bug Fixes