perf(gui): progressive dashboard paint and Startup safety CLS - #740
Conversation
|
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. |
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cbe980c8e
ℹ️ 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".
| if (count === 0) return true; | ||
| if (count > 1) return false; | ||
| const value = new RegExp(`<RunLevel(?:\\s[^>]*?)?>\\s*([^<]*?)\\s*<\\/RunLevel>`, "i").exec(principal)?.[1]?.trim().toLowerCase(); | ||
| return value === "leastprivilege" || value === "highestavailable"; |
There was a problem hiding this comment.
On Windows accounts that are local administrators, accepting HighestAvailable marks the task healthy even though it invokes the VBS and batch assets stored under the user's %OPENCODEX_HOME%. Those assets remain writable by the same user's non-elevated processes, so a medium-integrity process can replace them and have Task Scheduler execute the replacement elevated at the next logon or manual run. Keep verification restricted to LeastPrivilege, or protect the executable assets across integrity levels before accepting HighestAvailable.
Useful? React with 👍 / 👎.
| onInstall: (action: StartupInstallAction, opts?: { repair?: boolean }) => void; | ||
| }) { | ||
| const { t } = useI18n(); | ||
| const serviceNeedsRepair = data.serviceSupported && data.serviceInstalled && !data.serviceViable; |
There was a problem hiding this comment.
On Windows, an installed Task Scheduler entry that is disabled, has unhealthy registered XML, or conflicts with WinSW satisfies this predicate and receives a Repair button. The new scheduler repair path only rewrites the on-disk assets and calls /run; it never enables or re-registers the task, while conflicts are rejected outright, so these users either get an immediate error or remain unhealthy after a reported repair attempt. Gate the action to asset-only staleness or make the backend repair the registered task, including any required elevation.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| ]); | ||
|
|
||
| const sidecar = await requireJson<SidecarData>(scRes); | ||
| let shadowCall: ShadowCallData | null = null; |
There was a problem hiding this comment.
When a shadow-call poll starts before a settings mutation and resolves while that mutation is in flight, settingsPollMayCommit correctly rejects the response, but shadowCall remains null and the consuming effect then overwrites the optimistic/current value with null. The panel consequently disappears until a later poll restores it. Retain an undefined “do not commit” sentinel, as the previous implementation did, and only assign null for an authoritative endpoint failure.
Useful? React with 👍 / 👎.
| ...attachStaleAppServerHint(result), | ||
| ...result, | ||
| ...(result.ok ? {} : { error: result.message }), | ||
| staleAppServerHint: "If Codex App still shows an older model list, restart its long-lived app-server process after sync.", |
There was a problem hiding this comment.
A successful no-op sync (catalogWritten === false and cacheSynced === false) now always receives staleAppServerHint, so the dashboard tells users to restart Codex even though no catalog or cache changed. This also breaks the existing /api/sync contract test that explicitly requires the hint only after a write. Preserve the attachStaleAppServerHint(result) predicate, or apply the same catalogWritten || cacheSynced guard inline.
Useful? React with 👍 / 👎.
| if (command === "repair") { | ||
| assertServiceEnvironmentMatchesInstall(); | ||
| assertServiceAuthEnvironment(); | ||
| await repairService(); | ||
| console.log("✅ opencodex background service repaired (assets refreshed, no Task Scheduler re-registration)."); |
There was a problem hiding this comment.
This introduces the public ocx service repair command and a dashboard Repair action, but no docs-site/ content changed: the background-service table and examples in docs-site/src/content/docs/reference/cli.md still omit repair, including its platform-specific no-re-registration behavior and limitations. Update the CLI reference and Startup Safety documentation, keeping translated references consistent.
AGENTS.md reference: src/AGENTS.md:L24-L28
Useful? React with 👍 / 👎.
| const [data, setData] = useState<StartupHealthData | null>(() => cached?.data ?? null); | ||
| const [loading, setLoading] = useState(() => !cached?.data); | ||
| const [failed, setFailed] = useState(() => Boolean(cached?.data?.diagnosticStale)); | ||
| const [copied, setCopied] = useState<string | null>(null); | ||
| const [tray, setTray] = useState<TrayStatusData | null>(null); | ||
| const [trayLoading, setTrayLoading] = useState(true); | ||
| const [tray, setTray] = useState<TrayStatusData | null>(() => cached?.tray ?? null); | ||
| const [trayLoading, setTrayLoading] = useState(() => !cached?.data); |
There was a problem hiding this comment.
When this cache contains prior health, the page immediately renders that snapshot as actionable state while the new health request is still pending; the service/shim buttons do not check loading, and cached tray actions remain enabled because revalidation deliberately leaves trayLoading false. If service state changed in another terminal—for example, a native service was installed since the cached “not installed” snapshot—the stale Install action can switch it to the scheduler backend, and stale tray controls can stop or uninstall the wrong current state. Treat the cache as display-only until the first refresh succeeds, or disable all mutating actions during revalidation.
Useful? React with 👍 / 👎.
| useKeyedClientResource( | ||
| `add-provider-usage:${apiBase}`, | ||
| [apiBase], | ||
| async (signal) => { | ||
| const res = await fetch(`${apiBase}/api/usage?range=30d`, { signal }); |
There was a problem hiding this comment.
Once the Providers workspace is rendered, this prefetch starts /api/usage?range=30d while ProviderWorkspaceShell independently schedules the same request at ProviderWorkspaceShell.tsx:168. With a cached provider config both requests start in the same turn, and the server usage cache has no in-flight deduplication, so a cold visit can read and summarize the usage log twice while competing with the progressive paint this change is intended to improve. Share one client resource or seed the catalog rank from the workspace request instead of issuing a second request.
Useful? React with 👍 / 👎.
| if (!settingsRes.ok) return null; | ||
| return await settingsRes.json() as { codexRuntime?: CodexRuntimeSettings }; | ||
| }) | ||
| .catch(() => null); |
There was a problem hiding this comment.
If startup health succeeds but /api/settings transiently returns non-OK or rejects, these branches convert the failure to null, which is then treated as an authoritative runtime with no warning. Lines 135–153 consequently clear the previously cached clamp/older-binary warning and persist that cleared value for the rest of the session, hiding an actionable warning until a manual refresh succeeds. Use a failure sentinel and retain the last-good notice instead of conflating request failure with a valid empty notice.
Useful? React with 👍 / 👎.
| }>(usageCacheKey); | ||
| const [usageTotals, setUsageTotals] = useState<Record<string, ProviderUsageTotals>>(() => cachedUsage?.totals ?? {}); | ||
| const [usageModels, setUsageModels] = useState<Record<string, ProviderModelUsageRow[]>>(() => cachedUsage?.models ?? {}); | ||
| const [quotaReports, setQuotaReports] = useState<Record<string, ProviderQuotaReportView>>(() => cachedQuotas ?? {}); |
There was a problem hiding this comment.
After a quota report has been cached, logging out or removing the active credential can make a successful /api/provider-quotas response omit that provider. This initializer restores the old report, and the refresh path starts from { ...prev }, never deletes absent rows, and writes the same stale map back to session storage, so the logged-out account's quota remains visible across page remounts indefinitely. Invalidate the affected cached row on account changes, or distinguish a successful omission from a failed probe and remove rows that are no longer applicable.
Useful? React with 👍 / 👎.
| if (diag.backend === "native") { | ||
| await (deps.repairNative ?? (() => installWinswService(defaultWinswEntry(import.meta.dir))))(); | ||
| return; |
There was a problem hiding this comment.
When a native WinSW service is stale because the recorded Bun or CLI path no longer exists, installWinswService rewrites the XML with current paths and restarts the service, but this branch returns without rewriting service-state.json. bakedServicePathsDiagnostic() therefore continues reading the obsolete paths and reports the repaired service as stale, so Startup Safety remains unhealthy and keeps offering Repair after a successful response. Mirror the scheduler branch by persisting writeServiceInstallState("native") after the native repair succeeds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cbe980c8e
ℹ️ 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 "./styles-storage-workspace.css"; | ||
| @import "./styles-subagents-workspace.css"; | ||
| @import "./styles-usage-workspace.css"; | ||
| @import "./styles-claudecode-workspace.css"; | ||
| @import "./styles-apikeys-workspace.css"; |
There was a problem hiding this comment.
These five imports reference stylesheets that do not exist anywhere in the reviewed commit's tree, so Vite cannot resolve styles-storage-workspace.css and the GUI build fails before producing dist. The existing layout tests also explicitly require these split stylesheet names not to be imported from the consolidated styles.css; remove these imports or add the intended tracked files and update the contracts.
AGENTS.md reference: gui/AGENTS.md:L44-L50
Useful? React with 👍 / 👎.
| const attentionCount = attention.length; | ||
| const reauthCount = useMemo( | ||
| () => sections.needsSetup.filter(p => p.activeNeedsReauth).length, | ||
| () => [...sections.ready, ...sections.needsSetup].filter(p => p.activeNeedsReauth).length, |
There was a problem hiding this comment.
When an otherwise-ready OAuth provider needs reauthentication, this now finds it in sections.ready, but the summary still counts every such provider as Ready and uses only sections.needsSetup.length for the Needs attention card. With one configured provider needing reauth and no missing credentials, the page therefore shows “Ready 1” and “Needs attention 0” while listing that provider under Attention required. Derive the summary counts from binProviderStatus or use attentionCount and exclude ready rows tagged for reauth.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/src/pages/dashboard-core-poll.ts (1)
151-181: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBlocked shadow-call commits now overwrite live state with
nullinstead of being skipped.
shadowCallis initialized tonulland only assigned whensettingsPollMayCommit(...)passes, so a blocked commit (mutation in flight, or a newer request/mutation epoch) returns{ shadowCall: null }. The consumer ingui/src/pages/use-dashboard-data.ts(Line 321) applies it unconditionally:setShadowCall(data.shadowCall). Net effect: while aPUT /api/shadow-call-settingsis in flight, a concurrent 5s poll wipesshadowCall, which disables the toggle and select (disabled={!shadowCall || ...}indashboard-overview-sections.tsxLines 305/315) and flipsaria-busy— exactly the stale-write the epoch gate exists to prevent. The same applies to thecatchbranch, where the gate is checked only to assign the value it already had.Model "do not commit" as
undefinedand have the effect skip it.🔒 Proposed fix: distinguish "no commit" from "null"
export type DashboardSidecarPoll = { sidecar: SidecarData; - shadowCall: ShadowCallData | null; + /** `undefined` = epoch-blocked, callers must keep prior state. */ + shadowCall: ShadowCallData | null | undefined; };const sidecar = await requireJson<SidecarData>(scRes); - let shadowCall: ShadowCallData | null = null; + let shadowCall: ShadowCallData | null | undefined = undefined; try { if (shRes.ok) { const nextShadow = await shRes.json() as ShadowCallData;And in
gui/src/pages/use-dashboard-data.ts:- setShadowCall(data.shadowCall); + if (data.shadowCall !== undefined) setShadowCall(data.shadowCall);🤖 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/pages/dashboard-core-poll.ts` around lines 151 - 181, Change the shadow-call polling result to represent blocked commits as undefined rather than null: update the shadowCall declaration and both gated assignments in the polling function so only an allowed successful response commits its value, while parse failures commit null only when allowed. Update the consumer effect in use-dashboard-data to call setShadowCall only when data.shadowCall is defined, preserving the existing null behavior for valid committed absence.
🤖 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/components/provider-workspace/ProviderAuthPanel.tsx`:
- Around line 55-74: Initialize reserveQuotaSlots from the current accounts list
using the existing quota-enrichment condition so the placeholder is reserved on
the first render, rather than only after useEffect runs. Update the useEffect
around reserveQuotaSlots to retain the existing timeout and cleanup behavior for
clearing the reservation after QUOTA_ENRICH_RESERVE_MS.
In `@gui/src/components/provider-workspace/ProviderOverviewDashboard.tsx`:
- Line 55: Update the summary card counts in ProviderOverviewDashboard so
providers with activeNeedsReauth are excluded from the Ready count and included
in the Needs attention count, matching the existing reauthentication detection
over sections.ready and sections.needsSetup. Keep the displayed counts
consistent with binProviderStatus(...)=“needs-setup” and the management API
state.
In `@gui/src/components/provider-workspace/ProviderWorkspaceShell.tsx`:
- Around line 215-228: Move the writeSessionListCache call out of the
setQuotaReports updater in ProviderWorkspaceShell. Keep the updater limited to
computing and returning the merged quota map, then persist the committed
quotaReports value in a separate effect or step keyed to quotaReports and
quotasCacheKey.
- Around line 109-120: Update ProviderWorkspaceShell’s cache initialization to
read quotas and usage from sessionStorage only once via mount-stable refs or
equivalent snapshots, rather than calling readSessionListCache during every
render. Use those mount-time snapshots to seed the state and determine only the
initial loading state; update the quota and usage refresh effects to raise
loading whenever their refresh keys change after the initial render, even when
the original cache snapshot was populated, so stale account data is not
presented as settled.
In `@gui/src/pages/dashboard-core-poll.ts`:
- Around line 230-245: Update fetchDashboardOverview’s catch block to rethrow
abort errors using the existing isAbortError helper, while retaining the current
fallback snapshot for non-abort failures. Preserve the Promise.all and response
parsing flow unchanged so aborted generations do not return error data or mark
the overview as ready.
- Around line 212-228: Update fetchDashboardMaMode to return an explicit
unknown/failure result for non-OK responses, malformed payloads, and non-abort
exceptions instead of fabricating maMode: "default"; preserve abort propagation.
In the use-dashboard-data.ts flow around setMaMode, setMaModeResolved, and
writeSessionListCache, recognize the unknown result and retain the existing
prior or cached mode without marking it resolved or overwriting the session
cache.
In `@gui/src/pages/dashboard-overview-head.tsx`:
- Around line 66-75: Remove the inert aria-busy attributes from the plain .stat
containers in the dashboard stats markup, including the healthLoading instances
shown around the status, version, uptime, and providers values. Hoist the
relevant busy state onto the existing live-region container, matching the
aria-live="polite" treatment used by .startup-health-slot; preserve usageLoading
only if it is required for styling or behavior.
In `@gui/src/pages/dashboard-overview-sections.tsx`:
- Line 252: The sidecar panels currently derive `aria-busy` and disabled state
from missing data, conflating loading, failure, and epoch-blocked states. Add or
expose explicit loading state from the dashboard data hook, following the
`usageLoading`/`healthLoading` pattern, and use it in the `sidecar`,
`shadowCall`, and related panel rendering; when polling settles without data,
render the existing error affordance instead of leaving panels permanently busy
or disabled.
In `@gui/src/pages/Providers.tsx`:
- Around line 72-83: Update the usage-ranking loader passed to
useKeyedClientResource so non-OK /api/usage responses throw an error instead of
returning an empty rank map. Preserve the existing successful response parsing
and ranking behavior, matching the failure contract already used by the presets
loader so the shared resource records the error and can retry.
- Around line 101-107: Remove the hardcoded "Codex App login" comparison from
the authentication logic in Providers.tsx. Expose and reuse a dedicated
app-login sentinel or flag from the Codex account-pool layer when computing
mainIsReal, so synthetic main accounts never make codexLoggedIn true; preserve
the existing real-account and pool-account behavior.
In `@gui/src/pages/use-dashboard-data.ts`:
- Around line 345-349: Update the cache-writing block in fetchDashboardSettings
to persist controls only when the fetch produced a real committed settings
value; skip writeCachedControls when data.settings is null from an in-flight or
stale epoch. Preserve the existing cached settings entry during blocked polls so
the progressive-paint seed remains available.
- Around line 279-291: Update the overview polling effect around
overviewPoll.data so setProviders only runs when data.providers contains a valid
successful result, matching the existing data.health guard. Preserve the cached
or last-good providers list when fetchDashboardOverview returns its failure
payload with empty providers, while continuing to set errors normally.
- Around line 67-81: Remove the duplicated storage logic from readCachedControls
and writeCachedControls, and make these functions typed wrappers around the
imported readSessionListCache and writeSessionListCache helpers. Preserve the
CachedControls return type and existing corrupt-JSON-to-null and write-failure
behavior through the shared helpers, using the existing cache key construction.
In `@gui/src/pages/use-providers-fetch.ts`:
- Line 46: Update the provider query construction in the status fetch to apply
encodeURIComponent to the provider id before interpolating it into the URL,
preserving correct lookup for provider keys containing reserved or whitespace
characters and the existing fallback behavior.
In `@gui/src/styles-dashboard-workspace.css`:
- Around line 28-33: Update the `.dashboard-workspace-main .tbl-wrap` padding to
remove the top inset while preserving horizontal and bottom spacing, allowing
the sticky `thead` to own the top edge and preventing rows from painting above
it. Also apply the same top-padding asymmetry to the related `.tbl-wrap` styling
in `styles.css` used by `.api-models-scroll` and `.logs-table`.
In `@gui/src/styles.css`:
- Around line 1369-1371: Remove the deprecated word-break: break-word
declaration from the stylesheet rule containing overflow-wrap: anywhere, leaving
overflow-wrap: anywhere unchanged to preserve the intended wrapping behavior and
clear the Stylelint error.
- Around line 1095-1103: Update the 7-day chart rendering in Usage.tsx around
the daybar-stack element to replace the inline height style with the
--daybar-scale custom property, matching the existing --bar-scale pattern in
QuotaBars.tsx and allowing the CSS transform to reflect each percentage.
In `@gui/tests/collapse-store.test.ts`:
- Around line 78-84: Update the expectations in the test named “every family
folds by default” to include only families whose counts are 0, matching
defaultCollapsedFamilies behavior: for each fixture, remove non-empty families
from the expected sorted arrays while retaining all zero-count families.
In `@gui/tests/dashboard-contracts.test.ts`:
- Around line 46-101: Replace the source-text substring assertions in the
dashboard contract tests with behavioral coverage for each polling fetcher,
using the existing abort-test setup and symbols such as fetchDashboardSidecars
and fetchDashboardMaMode. Stub globalThis.fetch with non-OK responses and
exercise the relevant gate/ref states, asserting sidecars do not commit
shadowCall: null when shadowCallMutationInFlightRef is active and MA mode does
not fabricate "default" on a 500. Keep assertions focused on independent fetch
behavior and payloads.
In `@gui/tests/session-list-cache.test.ts`:
- Around line 25-35: Move the per-test setup into a beforeEach hook using
install, add beforeEach to the bun:test imports, and remove the explicit install
call from individual tests. Keep afterEach(restore) paired with this setup so
every test initializes a fresh testWindow and captured globals before teardown.
In `@src/server/management/config-routes.ts`:
- Around line 232-237: Update the /api/sync response construction around
syncModelsToCodex so staleAppServerHint is included only when result.ok is true.
Keep failed responses limited to the existing error data and HTTP 500 status,
while preserving the success response and hint unchanged.
In `@tests/service.test.ts`:
- Around line 796-852: Pin process.platform to "win32" for the entire "service
repair" suite using the existing beforeEach/afterEach platform-stubbing pattern
from the Windows scheduler tests, and restore the original platform afterward.
Update the repairService dependency objects in the non-rejection tests to
provide stubs for all platform repair paths, including repairLaunchd and
repairSystemd, so no real installer can execute and the existing call-order
assertions remain valid.
---
Outside diff comments:
In `@gui/src/pages/dashboard-core-poll.ts`:
- Around line 151-181: Change the shadow-call polling result to represent
blocked commits as undefined rather than null: update the shadowCall declaration
and both gated assignments in the polling function so only an allowed successful
response commits its value, while parse failures commit null only when allowed.
Update the consumer effect in use-dashboard-data to call setShadowCall only when
data.shadowCall is defined, preserving the existing null behavior for valid
committed absence.
🪄 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: 30bfff42-701e-418c-af49-6d2a503a7f08
📒 Files selected for processing (35)
gui/src/components/provider-catalog/ProviderCatalog.tsxgui/src/components/provider-workspace/ProviderAuthPanel.tsxgui/src/components/provider-workspace/ProviderOverviewDashboard.tsxgui/src/components/provider-workspace/ProviderRail.tsxgui/src/components/provider-workspace/ProviderWorkspaceShell.tsxgui/src/pages/Providers.tsxgui/src/pages/Startup.tsxgui/src/pages/dashboard-core-poll.tsgui/src/pages/dashboard-overview-head.tsxgui/src/pages/dashboard-overview-sections.tsxgui/src/pages/startup-sections.tsxgui/src/pages/use-dashboard-data.tsgui/src/pages/use-providers-fetch.tsgui/src/provider-workspace/catalog.tsgui/src/provider-workspace/usage.tsgui/src/session-list-cache.tsgui/src/startup-health-ui.tsgui/src/styles-dashboard-workspace.cssgui/src/styles.cssgui/src/styles/provider-overview-dashboard.cssgui/src/styles/provider-workspace-shell.cssgui/src/ui.tsxgui/tests/collapse-store.test.tsgui/tests/dashboard-contracts.test.tsgui/tests/session-list-cache.test.tssrc/server/management/config-routes.tssrc/server/management/context.tssrc/server/startup-action-control.tssrc/service.tstests/provider-workspace-data.test.tstests/service.test.tstests/startup-action-control-elevation.test.tstests/startup-action-control.test.tstests/startup-health-ui.test.tstests/windows-scheduler-install-verification.test.ts
| const [reserveQuotaSlots, setReserveQuotaSlots] = useState(false); | ||
| const deviceCodeCopy = useCopyFeedback<string>(); | ||
| const doctorCopy = useCopyFeedback<string>(); | ||
|
|
||
| // Soft "a=1 enrichment lands after the local account list. Reserve stacked | ||
| // bar height briefly so bars don't shove rows when WHAM returns. | ||
| useEffect(() => { | ||
| if (accounts.length === 0) { | ||
| setReserveQuotaSlots(false); | ||
| return; | ||
| } | ||
| const needsFill = accounts.some(a => a.quota == null && !a.quotaUnavailable); | ||
| if (!needsFill) { | ||
| setReserveQuotaSlots(false); | ||
| return; | ||
| } | ||
| setReserveQuotaSlots(true); | ||
| const timer = window.setTimeout(() => setReserveQuotaSlots(false), QUOTA_ENRICH_RESERVE_MS); | ||
| return () => window.clearTimeout(timer); | ||
| }, [accounts]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first, then inspect the relevant slices with line numbers.
ast-grep outline gui/src/components/provider-workspace/ProviderAuthPanel.tsx --view expanded || true
echo
echo "===== lines 1-140 ====="
sed -n '1,140p' gui/src/components/provider-workspace/ProviderAuthPanel.tsx | cat -n
echo
echo "===== lines 220-320 ====="
sed -n '220,320p' gui/src/components/provider-workspace/ProviderAuthPanel.tsx | cat -nRepository: lidge-jun/opencodex
Length of output: 13710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline gui/src/components/provider-workspace/ProviderAuthPanel.tsx --view expanded || true
echo
echo "===== lines 1-140 ====="
sed -n '1,140p' gui/src/components/provider-workspace/ProviderAuthPanel.tsx | cat -n
echo
echo "===== lines 220-320 ====="
sed -n '220,320p' gui/src/components/provider-workspace/ProviderAuthPanel.tsx | cat -nRepository: lidge-jun/opencodex
Length of output: 13710
Reserve the quota placeholder on the initial render. In gui/src/components/provider-workspace/ProviderAuthPanel.tsx:55-74, reserveQuotaSlots is only enabled inside useEffect, so accounts that already need quota enrichment paint once without the reserved quota row and then insert it after paint. If the goal is to prevent layout shift, initialize the state from accounts.some(a => a.quota == null && !a.quotaUnavailable) and keep the timeout in the effect, or derive the placeholder directly from that condition.
🤖 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/provider-workspace/ProviderAuthPanel.tsx` around lines 55
- 74, Initialize reserveQuotaSlots from the current accounts list using the
existing quota-enrichment condition so the placeholder is reserved on the first
render, rather than only after useEffect runs. Update the useEffect around
reserveQuotaSlots to retain the existing timeout and cleanup behavior for
clearing the reservation after QUOTA_ENRICH_RESERVE_MS.
Source: Path instructions
| const quotasCacheKey = `ocx.providers.quotas.v1:${apiBase}`; | ||
| const usageCacheKey = `ocx.providers.usage.v1:${apiBase}`; | ||
| const cachedQuotas = readSessionListCache<Record<string, ProviderQuotaReportView>>(quotasCacheKey); | ||
| const cachedUsage = readSessionListCache<{ | ||
| totals: Record<string, ProviderUsageTotals>; | ||
| models: Record<string, ProviderModelUsageRow[]>; | ||
| }>(usageCacheKey); | ||
| const [usageTotals, setUsageTotals] = useState<Record<string, ProviderUsageTotals>>(() => cachedUsage?.totals ?? {}); | ||
| const [usageModels, setUsageModels] = useState<Record<string, ProviderModelUsageRow[]>>(() => cachedUsage?.models ?? {}); | ||
| const [quotaReports, setQuotaReports] = useState<Record<string, ProviderQuotaReportView>>(() => cachedQuotas ?? {}); | ||
| const [usageLoading, setUsageLoading] = useState(() => !cachedUsage); | ||
| const [quotasLoading, setQuotasLoading] = useState(() => !cachedQuotas); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
readSessionListCache runs on every render, and the effects then read that always-fresh value as if it were a mount-time snapshot.
Lines 111-115 call readSessionListCache directly in the component body, so sessionStorage.getItem + a full JSON.parse of the usage payload (every provider × every model row) executes on every render of the shell — each search keystroke (Line 351), each filter toggle, each setRailFocusName on rail focus. That is a synchronous main-thread parse in the hottest interactive path of this page, which works directly against the "progressive paint" goal of this PR.
The second-order effect is a behavioral bug. The guards at Line 167 (if (!cachedUsage) setUsageLoading(true)) and Line 209 (if (!cachedQuotas) setQuotasLoading(true)) read the current render's value, not the mount-time one. Once the first successful fetch writes the cache (Lines 195 and 226), cachedQuotas is truthy forever, so on a subsequent quotaRefreshKey change — which fires on every OAuth account switch, per the comment on Lines 237-238 — quotasLoading is never raised again. The overview keeps rendering the previous account's quota bars with aria-busy unset (ProviderOverviewDashboard, aria-busy={quotasLoading || undefined}), so both sighted and screen-reader users see stale numbers presented as settled. The same applies to usageLoading if apiBase ever changes.
Reading the cache once into a ref fixes both: the parse happens at mount only, and the effects can distinguish "first paint, seeded" from "refetch of a new key".
🐛 Proposed fix: snapshot the cache at mount, gate the skeleton on first paint only
const quotasCacheKey = `ocx.providers.quotas.v1:${apiBase}`;
const usageCacheKey = `ocx.providers.usage.v1:${apiBase}`;
- const cachedQuotas = readSessionListCache<Record<string, ProviderQuotaReportView>>(quotasCacheKey);
- const cachedUsage = readSessionListCache<{
- totals: Record<string, ProviderUsageTotals>;
- models: Record<string, ProviderModelUsageRow[]>;
- }>(usageCacheKey);
+ const seedRef = useRef<{
+ quotas: Record<string, ProviderQuotaReportView> | null;
+ usage: {
+ totals: Record<string, ProviderUsageTotals>;
+ models: Record<string, ProviderModelUsageRow[]>;
+ } | null;
+ } | null>(null);
+ if (seedRef.current === null) {
+ seedRef.current = {
+ quotas: readSessionListCache<Record<string, ProviderQuotaReportView>>(quotasCacheKey),
+ usage: readSessionListCache<{
+ totals: Record<string, ProviderUsageTotals>;
+ models: Record<string, ProviderModelUsageRow[]>;
+ }>(usageCacheKey),
+ };
+ }
+ const cachedQuotas = seedRef.current.quotas;
+ const cachedUsage = seedRef.current.usage;Then make the skeleton guard depend on whether this key was seeded, not on the ever-truthy snapshot:
- if (!cachedQuotas) setQuotasLoading(true);
+ // Seeded only suppresses the skeleton on the very first paint; later refetches
+ // (account switch via quotaRefreshKey) must surface aria-busy again.
+ if (!seededOnceRef.current) setQuotasLoading(true);🤖 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/provider-workspace/ProviderWorkspaceShell.tsx` around
lines 109 - 120, Update ProviderWorkspaceShell’s cache initialization to read
quotas and usage from sessionStorage only once via mount-stable refs or
equivalent snapshots, rather than calling readSessionListCache during every
render. Use those mount-time snapshots to seed the state and determine only the
initial loading state; update the quota and usage refresh effects to raise
loading whenever their refresh keys change after the initial render, even when
the original cache snapshot was populated, so stale account data is not
presented as settled.
| setQuotaReports(prev => { | ||
| const next = { ...prev }; | ||
| for (const report of data.reports ?? []) { | ||
| if (!report?.provider) continue; | ||
| next[report.provider] = { | ||
| label: report.label, | ||
| source: report.source, | ||
| updatedAt: typeof report.updatedAt === "number" ? report.updatedAt : Date.now(), | ||
| quota: report.quota, | ||
| }; | ||
| } | ||
| writeSessionListCache(quotasCacheKey, next); | ||
| return next; | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
React 19 setState updater function must be pure called twice StrictMode
💡 Result:
In React 19, just as in previous versions, state updater functions passed to the useState setter must be pure [1][2]. Strict Mode intentionally calls these updater functions twice during development to help identify accidental impurities [1][3][4]. This is a development-only behavior and does not affect production builds [1][3][5]. Key details regarding this behavior: Pure Requirement: Updater functions must be pure, meaning they should only calculate the next state based on the pending state and return it without performing side effects or mutating existing state [1][2][6]. Because they are pure, calling them multiple times with the same input will consistently yield the same output, ensuring that React's double-invocation in development does not change the resulting state [1][3][5]. Development-Only: This double-invocation is designed to surface bugs early [3][4]. If an updater function is impure (e.g., it modifies a variable outside its scope), the extra call will likely cause observable, incorrect behavior, making the bug easier to detect [3][4][5]. React 19 Context: While React 19 introduced several improvements and changes—such as updates to how Strict Mode handles effects in certain scenarios (e.g., keyed elements) [7] and changes to the arity of the setter function [8]—the fundamental requirement for updater functions to be pure and the development-only double-invocation behavior remain unchanged [1][3][4]. If you observe side effects occurring twice in development, it serves as a signal that the updater function or component body is impure and should be refactored to remove those side effects [3][4]. Event handlers, by contrast, are not subject to this double-invocation [1].
Citations:
- 1: https://react.dev/reference/react/useState
- 2: https://react.dev/learn/queueing-a-series-of-state-updates
- 3: https://de.react.dev/reference/react/StrictMode
- 4: https://react.dev/reference/react/StrictMode
- 5: https://react.dev/learn/keeping-components-pure
- 6: https://react.dev/reference/rules/components-and-hooks-must-be-pure
- 7: Bug:
StrictModereruns effects when a child is moved in an array react/react#32561 - 8: [React 19] useState's setter function becomes arity 2 react/react#31578
Move writeSessionListCache out of the setQuotaReports updater
gui/src/components/provider-workspace/ProviderWorkspaceShell.tsx:215-228 calls writeSessionListCache(quotasCacheKey, next) from inside the setQuotaReports(prev => …) callback. React state updaters must stay pure; if React replays or discards a render, this can persist a cache entry that never became committed state. Compute the merged map first, then write it in a separate step/effect keyed on quotaReports.
🤖 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/provider-workspace/ProviderWorkspaceShell.tsx` around
lines 215 - 228, Move the writeSessionListCache call out of the setQuotaReports
updater in ProviderWorkspaceShell. Keep the updater limited to computing and
returning the merged quota map, then persist the committed quotaReports value in
a separate effect or step keyed to quotaReports and quotasCacheKey.
| export async function fetchDashboardMaMode( | ||
| apiBase: string, | ||
| signal: AbortSignal, | ||
| ): Promise<DashboardMaModePoll> { | ||
| try { | ||
| const v2Res = await fetch(`${apiBase}/api/v2`, { signal }); | ||
| if (!v2Res.ok) return { maMode: "default" }; | ||
| const v2Data = await v2Res.json() as { multiAgentMode?: unknown }; | ||
| if (v2Data.multiAgentMode === "v1" || v2Data.multiAgentMode === "v2") { | ||
| return { maMode: v2Data.multiAgentMode }; | ||
| } | ||
| return { maMode: "default" }; | ||
| } catch (error) { | ||
| if (isAbortError(error, signal)) throw error; | ||
| return { maMode: "default" }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
/api/v2 failures are indistinguishable from a real "default" mode, and the hook persists the fabricated value.
A non-OK response or a malformed body returns { maMode: "default" }. use-dashboard-data.ts (Lines 293-298) then does setMaMode(...), setMaModeResolved(true) and writeSessionListCache(MA_MODE_CACHE_PREFIX...) unconditionally, so a single transient 500 on /api/v2 (a) visibly flips the radiogroup of a v2 user to default and (b) poisons the session cache so the next mount also seeds default. Return an explicit "unknown" signal on failure and let the hook keep the prior/cached mode.
♻️ Proposed shape change
export type DashboardMaModePoll = {
- maMode: "v1" | "default" | "v2";
+ /** `undefined` = endpoint unavailable/malformed; callers keep prior state. */
+ maMode: "v1" | "default" | "v2" | undefined;
};- if (!v2Res.ok) return { maMode: "default" };
+ if (!v2Res.ok) return { maMode: undefined };
const v2Data = await v2Res.json() as { multiAgentMode?: unknown };
if (v2Data.multiAgentMode === "v1" || v2Data.multiAgentMode === "v2") {
return { maMode: v2Data.multiAgentMode };
}
return { maMode: "default" };
} catch (error) {
if (isAbortError(error, signal)) throw error;
- return { maMode: "default" };
+ return { maMode: undefined };
}🤖 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/pages/dashboard-core-poll.ts` around lines 212 - 228, Update
fetchDashboardMaMode to return an explicit unknown/failure result for non-OK
responses, malformed payloads, and non-abort exceptions instead of fabricating
maMode: "default"; preserve abort propagation. In the use-dashboard-data.ts flow
around setMaMode, setMaModeResolved, and writeSessionListCache, recognize the
unknown result and retain the existing prior or cached mode without marking it
resolved or overwriting the session cache.
| function restore() { | ||
| testWindow.close(); | ||
| for (const key of globals) { | ||
| Object.defineProperty(globalThis, key, { configurable: true, value: previousGlobals[key] }); | ||
| } | ||
| } | ||
|
|
||
| afterEach(restore); | ||
|
|
||
| test("session list cache round-trips non-secret JSON shapes", () => { | ||
| install(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
afterEach(restore) is unconditional while install() is per-test — one forgotten call and teardown throws.
restore() dereferences testWindow.close(); any future test in this file that omits the first-line install() makes teardown fail with a TypeError (or, worse, restores globals captured by an earlier test). Pair them via beforeEach so the lifecycle can't drift.
♻️ Proposed refactor
-afterEach(restore);
+beforeEach(install);
+afterEach(restore);
test("session list cache round-trips non-secret JSON shapes", () => {
- install();
writeSessionListCache("ocx.test", { range: "30d", surface: "claude", n: 3 });(and drop the install() call in the second test; add beforeEach to the bun:test import.)
📝 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 restore() { | |
| testWindow.close(); | |
| for (const key of globals) { | |
| Object.defineProperty(globalThis, key, { configurable: true, value: previousGlobals[key] }); | |
| } | |
| } | |
| afterEach(restore); | |
| test("session list cache round-trips non-secret JSON shapes", () => { | |
| install(); | |
| function restore() { | |
| testWindow.close(); | |
| for (const key of globals) { | |
| Object.defineProperty(globalThis, key, { configurable: true, value: previousGlobals[key] }); | |
| } | |
| } | |
| beforeEach(install); | |
| afterEach(restore); | |
| test("session list cache round-trips non-secret JSON shapes", () => { | |
| writeSessionListCache("ocx.test", { range: "30d", surface: "claude", n: 3 }); |
🤖 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/session-list-cache.test.ts` around lines 25 - 35, Move the per-test
setup into a beforeEach hook using install, add beforeEach to the bun:test
imports, and remove the explicit install call from individual tests. Keep
afterEach(restore) paired with this setup so every test initializes a fresh
testWindow and captured globals before teardown.
| const result = await syncModelsToCodex(undefined, config, null); | ||
| // Hint only after a real catalog/cache write — never enumerate processes here | ||
| // (WMIC/PowerShell would block Bun's event loop on every dashboard sync). | ||
| return jsonResponse({ | ||
| ...attachStaleAppServerHint(result), | ||
| ...result, | ||
| ...(result.ok ? {} : { error: result.message }), | ||
| staleAppServerHint: "If Codex App still shows an older model list, restart its long-lived app-server process after sync.", | ||
| }, result.ok ? 200 : 500); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "attachStaleAppServerHint"
rg -n "staleAppServerHint" gui/src srcRepository: lidge-jun/opencodex
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the cited lines
wc -l src/server/management/config-routes.ts
sed -n '200,260p' src/server/management/config-routes.ts
# Search for the hint and the helper across the repo
rg -n "attachStaleAppServerHint|staleAppServerHint|syncModelsToCodex" src gui || trueRepository: lidge-jun/opencodex
Length of output: 5077
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files src/server/management/config-routes.ts
sed -n '220,250p' src/server/management/config-routes.tsRepository: lidge-jun/opencodex
Length of output: 1648
Keep staleAppServerHint tied to success
In src/server/management/config-routes.ts:232-237, /api/sync always attaches staleAppServerHint, even when result.ok is false and the route returns HTTP 500. That makes the “restart its long-lived app-server process after sync” advice misleading on failed syncs. The GUI already treats the hint as optional (gui/src/pages/dashboard-overview-sections.tsx:199), so gate this field on result.ok and keep the error response free of success-only guidance.
🤖 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/server/management/config-routes.ts` around lines 232 - 237, Update the
/api/sync response construction around syncModelsToCodex so staleAppServerHint
is included only when result.ok is true. Keep failed responses limited to the
existing error data and HTTP 500 status, while preserving the success response
and hint unchanged.
c57bfe4 to
f6f9c9c
Compare
|
@coderabbitai review |
6cbe980 to
8cfa8a0
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
dd0be07 to
16b0aaf
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
16b0aaf to
877247d
Compare
Split slow dashboard peers, cache non-secret page seeds, and keep Startup chrome stable while diagnostics settle.
46e2c95 to
297ceef
Compare
877247d to
a2072d5
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 (#740)
Merge-ready after #738–#739 (then retarget to |
Summary
Stack
stack/gui-codex-pool-hydration) — perf(gui): progressive dashboard paint and Startup safety CLS #740Test plan
bun test gui/tests/dashboard-contracts.test.ts gui/tests/session-list-cache.test.ts tests/startup-health-ui.test.ts tests/service.test.tsSummary by CodeRabbit