[codex] Preserve stable snapshots and stabilize Electron e2e#734
Conversation
ashione
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2358562877
ℹ️ 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".
tests/e2e/fixtures/electron.ts
Outdated
| ]).then(() => { | ||
| closed = true; | ||
| }), |
There was a problem hiding this comment.
closeElectronApp sets closed = true after Promise.allSettled(...) regardless of whether app.waitForEvent('close') actually fired, so a timeout/rejection is treated as success and the function returns early without running the app.close()/SIGKILL fallback. In teardown paths where electronApp.quit() does not close the process promptly, this leaves orphaned Electron processes and reintroduces the CI flakiness this helper is meant to prevent.
Useful? React with 👍 / 👎.
src/pages/Models/index.tsx
Outdated
| ? fetchState.data.filter((entry) => !shouldHideUsageEntry(entry)) | ||
| : []; | ||
| const stableUsageHistory = fetchState.stableData.filter((entry) => !shouldHideUsageEntry(entry)); | ||
| const visibleUsageHistory = resolveVisibleUsageHistory(usageHistory, stableUsageHistory); |
There was a problem hiding this comment.
This now falls back to stableUsageHistory even when isGatewayRunning is false, so after a disconnect/restart the Models page can continue showing stale token history from the previous session instead of reflecting the reset state. Because reset only clears data, this path keeps old records visible while the gateway is down, which is misleading and differs from the previous behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Improves UI resilience during background refreshes/errors by preserving “stable” snapshots (token usage, agents/channels listings, chat history) and expands/stabilizes Electron Playwright E2E coverage, including a setup-bypass path for navigation smoke tests.
Changes:
- Add stable snapshot helpers/hook and apply them to Models token usage and Agents/Channels pages to avoid blanking during refresh/error states.
- Harden chat history reload to avoid clearing messages on transient failures and to drop stale results after session switches.
- Add/expand Electron Playwright smoke coverage and improve macOS E2E shutdown reliability (allow close-to-quit in E2E + stronger teardown).
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/models-usage-history.test.ts | Adds unit coverage for stable/visible usage-history resolution helpers. |
| tests/unit/chat-history-actions.test.ts | Adds unit coverage for preserving messages on reload failure + stale-session result dropping. |
| tests/unit/channels-page.test.tsx | Adds unit coverage ensuring Channels keeps last snapshot visible during pending refresh. |
| tests/unit/agents-page.test.tsx | Adds unit coverage ensuring Agents keeps last snapshot visible during refresh and still shows initial blocking spinner when no snapshot exists. |
| tests/e2e/main-navigation.spec.ts | New E2E spec for core navigation when setup is bypassed. |
| tests/e2e/fixtures/electron.ts | Adds helpers for stable window selection and more robust app shutdown; extends launcher options (skipSetup). |
| tests/e2e/app-smoke.spec.ts | Uses new close helper to reduce flaky teardown / orphaned processes. |
| src/stores/chat/history-actions.ts | Prevents clearing current messages on transient history failures; guards against stale session overwrites. |
| src/stores/chat.ts | Mirrors chat history hardening in the main store loadHistory implementation. |
| src/pages/Models/usage-history.ts | Introduces stable/visible usage history resolution helpers. |
| src/pages/Models/index.tsx | Preserves stable usage snapshot across refreshes and adjusts loading/refreshing UI behavior. |
| src/pages/Channels/index.tsx | Uses shared stable snapshot hook to keep channels/agents visible during refresh/error states; adds testid/spinner behavior. |
| src/pages/Agents/index.tsx | Uses shared stable snapshot hook to keep agent/channel-account data visible during refresh; adds testid/spinner behavior. |
| src/hooks/use-stable-snapshot.ts | New generic hook to persist and optionally display the last stable snapshot. |
| src/App.tsx | Allows setup redirect bypass via query param for E2E navigation tests. |
| electron/main/index.ts | Propagates E2E setup-bypass flag into renderer URL/query; adjusts quit/close behavior for E2E. |
| README.md | Documents Electron E2E tests and headless Linux xvfb guidance. |
| README.zh-CN.md | Same documentation additions (Chinese). |
| README.ja-JP.md | Same documentation additions (Japanese). |
| pnpm-lock.yaml | Bumps Playwright packages to support updated E2E behavior. |
| package.json | Formatting-only change (final newline/brace alignment). |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
src/pages/Channels/index.tsx:263
isUsingStableValuecan become true whenshouldUseStableis true due to an error state (sinceshouldUseStable: loading || Boolean(error)), which makes the refresh icon spin even when no refresh is in flight. Consider basing the spinner animation onloading(or a dedicatedisRefreshing) rather than onisUsingStableValuefor error-driven stable rendering.
{gatewayStatus.state !== 'running' && (
<div className="mb-8 p-4 rounded-xl border border-yellow-500/50 bg-yellow-500/10 flex items-center gap-3">
<AlertCircle className="h-5 w-5 text-yellow-600 dark:text-yellow-400" />
<span className="text-yellow-700 dark:text-yellow-400 text-sm font-medium">
{t('gatewayWarning')}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/pages/Models/index.tsx
Outdated
| }; | ||
| case 'reset': | ||
| return { status: 'idle', data: [] }; | ||
| return { ...state, status: 'idle', data: [] }; |
There was a problem hiding this comment.
In the reducer reset case, stableData is not cleared. Because stableUsageHistory is derived from fetchState.stableData without an isGatewayRunning guard, the UI can continue to render the last stable token-usage history even after the gateway stops (when dispatchFetch({ type: 'reset' }) runs). Consider clearing stableData on reset, or gating stableUsageHistory/visibleUsageHistory behind isGatewayRunning so stale usage data is not shown while the gateway is offline.
| return { ...state, status: 'idle', data: [] }; | |
| return { ...state, status: 'idle', data: [], stableData: [] }; |
tests/e2e/fixtures/electron.ts
Outdated
| Promise.allSettled([ | ||
| app.waitForEvent('close', { timeout: timeoutMs }), | ||
| app.evaluate(({ app: electronApp }) => { | ||
| electronApp.quit(); | ||
| }), | ||
| ]).then(() => { | ||
| closed = true; | ||
| }), |
There was a problem hiding this comment.
closeElectronApp marks the app as closed unconditionally once Promise.allSettled(...) resolves. Because allSettled also resolves when waitForEvent('close') rejects due to timeout, this can cause teardown to return early and skip the app.close() / SIGKILL fallback even when the app never actually closed. Track whether the close event fired (or inspect the settled results) before deciding to return.
| Promise.allSettled([ | |
| app.waitForEvent('close', { timeout: timeoutMs }), | |
| app.evaluate(({ app: electronApp }) => { | |
| electronApp.quit(); | |
| }), | |
| ]).then(() => { | |
| closed = true; | |
| }), | |
| (async () => { | |
| const [closeResult] = await Promise.allSettled([ | |
| app.waitForEvent('close', { timeout: timeoutMs }), | |
| app.evaluate(({ app: electronApp }) => { | |
| electronApp.quit(); | |
| }), | |
| ]); | |
| if (closeResult && closeResult.status === 'fulfilled') { | |
| closed = true; | |
| } | |
| })(), |
src/stores/chat/history-actions.ts
Outdated
| const hasMessages = get().messages.length > 0; | ||
| set((state) => ({ | ||
| loading: false, | ||
| error: !quiet && errorMessage ? errorMessage : state.error, | ||
| ...(hasMessages ? {} : { messages: [] as RawMessage[] }), | ||
| })); |
There was a problem hiding this comment.
applyLoadFailure reads get().messages.length outside the set((state) => ...) callback. If messages change between that read and the update, the logic can clear messages incorrectly. Consider deriving hasMessages from state.messages.length inside the setter callback.
| const hasMessages = get().messages.length > 0; | |
| set((state) => ({ | |
| loading: false, | |
| error: !quiet && errorMessage ? errorMessage : state.error, | |
| ...(hasMessages ? {} : { messages: [] as RawMessage[] }), | |
| })); | |
| set((state) => { | |
| const hasMessages = state.messages.length > 0; | |
| return { | |
| loading: false, | |
| error: !quiet && errorMessage ? errorMessage : state.error, | |
| ...(hasMessages ? {} : { messages: [] as RawMessage[] }), | |
| }; | |
| }); |
src/stores/chat/history-actions.ts
Outdated
| // Async: load missing image previews from disk (updates in background) | ||
| loadMissingPreviews(finalMessages).then((updated) => { | ||
| if (!isCurrentSession()) return; | ||
| if (updated) { | ||
| // Create new object references so React.memo detects changes. |
There was a problem hiding this comment.
loadMissingPreviews(...).then(...) sets messages from the captured finalMessages snapshot. Even with the new session guard, this can overwrite newer messages added later in the same session. Prefer updating based on the current state.messages (e.g., patching only affected messages/files) instead of replacing from the stale snapshot.
src/stores/chat.ts
Outdated
| const hasMessages = get().messages.length > 0; | ||
| set((state) => ({ | ||
| loading: false, | ||
| error: !quiet && errorMessage ? errorMessage : state.error, | ||
| ...(hasMessages ? {} : { messages: [] as RawMessage[] }), | ||
| })); |
There was a problem hiding this comment.
applyLoadFailure computes hasMessages via get() outside the set((state) => ...) callback, which can race with other updates and clear messages incorrectly. Use state.messages.length inside the setter callback to decide whether to preserve/clear messages.
| const hasMessages = get().messages.length > 0; | |
| set((state) => ({ | |
| loading: false, | |
| error: !quiet && errorMessage ? errorMessage : state.error, | |
| ...(hasMessages ? {} : { messages: [] as RawMessage[] }), | |
| })); | |
| set((state) => { | |
| const hasMessages = state.messages.length > 0; | |
| return { | |
| loading: false, | |
| error: !quiet && errorMessage ? errorMessage : state.error, | |
| ...(hasMessages ? {} : { messages: [] as RawMessage[] }), | |
| }; | |
| }); |
src/stores/chat.ts
Outdated
| // Async: load missing image previews from disk (updates in background) | ||
| loadMissingPreviews(finalMessages).then((updated) => { | ||
| if (!isCurrentSession()) return; | ||
| if (updated) { | ||
| // Create new object references so React.memo detects changes. |
There was a problem hiding this comment.
This preview-hydration path sets messages from the captured finalMessages snapshot, which can overwrite newer same-session messages appended after the history load. The new isCurrentSession() guard prevents cross-session overwrites but not same-session races. Prefer patching state.messages in-place (or merging by id/timestamp) rather than writing the snapshot back.
| let mounted = true; | ||
| // eslint-disable-next-line react-hooks/set-state-in-effect | ||
| void Promise.all([fetchAgents(), fetchChannelAccounts(), refreshProviderSnapshot()]); | ||
| void Promise.all([fetchAgents(), fetchChannelAccounts(), refreshProviderSnapshot()]).finally(() => { | ||
| if (mounted) { | ||
| setHasCompletedInitialLoad(true); |
There was a problem hiding this comment.
hasCompletedInitialLoad is only set after Promise.all([fetchAgents(), fetchChannelAccounts(), refreshProviderSnapshot()]) settles, but stable snapshot persistence is gated on hasCompletedInitialLoad && !loading. This can delay persisting a stable snapshot even after agents are already loaded/rendered, so a user refresh during that window can still fall back to the blocking spinner because hasStableValue is still false. Consider persisting as soon as the primary page data (agents/channelGroups) is available, rather than waiting on the provider snapshot.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af3296ed31
ℹ️ 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".
src/pages/Models/usage-history.ts
Outdated
| previousStableEntries: UsageHistoryEntry[], | ||
| nextEntries: UsageHistoryEntry[], | ||
| ): UsageHistoryEntry[] { | ||
| return nextEntries.length > 0 ? nextEntries : previousStableEntries; |
There was a problem hiding this comment.
resolveStableUsageHistory always falls back to previousStableEntries when nextEntries is empty, and the Models reducer uses this on every completed fetch. That means once the page has shown any token history, a later successful fetch returning [] can never clear it, so users keep seeing stale usage rows even when the backend has no records (for example after transcript/history reset).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b07493e60c
ℹ️ 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".
| } | ||
| }; | ||
| }, [isGatewayRunning, gatewayStatus.connectedAt, gatewayStatus.pid, usageFetchMaxAttempts]); | ||
| }, [isGatewayRunning, gatewayStatus.connectedAt, gatewayStatus.pid, usageFetchMaxAttempts, usageRefreshNonce]); |
There was a problem hiding this comment.
Adding usageRefreshNonce to this effect causes token-history fetches to rerun every 15s/focus, but the fetch success path still calls setUsagePage(1). As a result, once there are enough records to paginate, users on page 2+ are repeatedly forced back to page 1 during background refreshes, which makes browsing older entries impractical and is a regression introduced by the new auto-refresh loop.
Useful? React with 👍 / 👎.
What changed
Modelstoken usage during background refreshes and transient empty responsesuseStableSnapshothook and apply it toAgentsandChannelsso those pages keep rendering prior data during refresh/error statesapp.close()/SIGKILLWhy
Several pages were clearing visible content or falling back to blocking loading states during transient refreshes. That caused the token usage view to flash to empty, the chat view to lose visible history on failed reloads, and list pages to momentarily blank during background fetches.
The accompanying Electron coverage was needed because these regressions are easiest to miss when only unit tests are exercised. During validation, E2E shutdown on macOS also proved flaky and left residual Electron processes behind, so that path is fixed in the same PR.
User impact
Validation
pnpm exec vitest run tests/unit/agents-page.test.tsx tests/unit/channels-page.test.tsx tests/unit/models-usage-history.test.ts tests/unit/chat-history-actions.test.tspnpm run typecheckpnpm exec playwright test tests/e2e/main-navigation.spec.ts --workers=1pnpm exec playwright test tests/e2e/app-smoke.spec.ts --workers=1pnpm run test:e2e