Repository navigation
🤖 refactor: settings, hooks and contexts keep shown data while reloading - #6351
Conversation
…wn data while reloading
…shown data while reloading
…ancel is still reported A server cancel on a live socket is a plain AbortError, and isAbortError() matched it too. Swallowing it left the section stuck: no new client comes to load again. The reloads now ignore only isConnectionLostError(); each effect's own cleanup abort stays handled by its cancelled flag or request check.
The settings sections keep their loaded values while Xum reconnects. Their server-backed controls stayed interactive offline and dropped the write without a word. Each one now reports the offline state or is disabled while there is no live client: - ServerAccess: Refresh and the revoke buttons are disabled. - Secrets: Save is disabled (Reset stays: it is local). - General: the archive selects, keep-awake, API debug logs, SSH host and default project directory are disabled. The transcript-width switch reports the failure through the live client. - Heartbeat: the fields and ConfigSwitchSetting are disabled. A save the lost connection cut off clears the edit mark, so the reload shows the saved value instead of an unsaved one that looks saved. - Tasks: a change made offline stays pending with an offline message, and the autosave sends it through the next client. A retry never takes the pending change while offline. - Experiments: the Portable Desktop warning's buttons are disabled. Backup already shows that its settings are unavailable while disconnected.
ThomasK33
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 9a3c6fd809
ℹ️ 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".
Four review findings on the kept-data Settings and context changes: - Secrets: a reload reads the form's dirtiness when it answers, not when it started, so an edit typed while the reload is in flight survives it. - Coder presets: another template clears the shown presets even offline, so the old template's presets are never selectable. - Tasks: every control is disabled while offline. An offline edit lived only in the section's state and was lost when the user left the section before the reconnect. - Workspaces: a new client shows the loader again while no snapshot has loaded, so a disconnect before the first snapshot does not leave the app with an empty metadata map.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71ee55a8e3
ℹ️ 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".
…ntil it answers Four review findings on the kept-data reloads: - Tasks: the controls stay disabled until a load answers. The autosave does not track an edit made before the reload answers, so the answer would have replaced it. - Coder presets: every online load sets `loadingPresets`, a reconnect reload too. ChatInput blocks a Coder send on it, so a send cannot use a preset the server removed while offline. The shown presets stay. - Creation branches: every load clears `branchesLoaded`, the send barrier for a trunk branch (#6033). The shown branches stay. - MCP prompts: a new client lists the prompts again, even inside the 30 s cache window, because they can change while offline.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 714dd37416
ℹ️ 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".
Review findings on the kept-data reloads: - Coder presets: a reload that no longer lists the selected preset selects a listed one, so a preset removed while offline is not sent. - General: the two archive selects are disabled until each config read answers. They are written together, so a change before the read would send a stale value for the other one. - Heartbeat: Save waits for the new client's read (`isReloading`): the kept form would overwrite changes made on the server while offline. - Backup: Save waits for a pending settings read, for the same reason. A test also pins that a drop before the first metadata snapshot keeps the startup loader: the ConnectionLostError is not a load failure.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cce8c99764
ℹ️ 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".
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: cce8c99764
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
A dirty Secrets form survives a reconnect, and Save was enabled as soon as the connection came back. Save writes the whole list, so a click before the new read answered could overwrite secrets changed on the server while offline (including injectAll). Save is now disabled while a read is pending.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc4981ac8a
ℹ️ 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".
Merge decisionCodex reviewed this head (Code Review and Security Review both Completed) with no unresolved findings, but posted no approval comment or 👍. This PR is past the review-round cap (rounds 4+ are blockers-only). A clean-context advisor checked the threads, the deferred findings and the Required checks, and recommended: ready with tracked follow-ups, merge on this silent completion without another round. I accept that recommendation. Deferred findings are tracked in #6383. Required CI passes on this head. Generated with |
…ngs-keep-data Resolve useCreationWorkspace against #6417 (branchesLoaded became branchLoad): keep main's per-load branch status, and keep the runtimes shown across a reconnect reload. The reconnect test reads branchLoad.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 341d3a197b
ℹ️ 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".
…, and cache a viewed wizard (T3 C step 2 PR 1b) (coder#6379) ## Summary `SplashScreenProvider` removes the static first-run view (`#xum-pre`) once its splash queue is known and the onboarding wizard is not first. It also records a new paint cache, so the server's early script (a later PR) can skip its API calls for a browser that has seen the wizard marked viewed. Nothing inserts `#xum-pre` yet. **Stack** (T3 coder#5971, Option C step 2, PR 1 split at the 300-line limit into 3 layers, each inert without `#xum-pre`): 1. coder#6378: entry and connection guard (this stack's bottom) 2. coder#6379: splash queue and paint cache 3. coder#6381: wizard handoff Plan: section 5.6 "C step 2: the static first-run view on `/` (P1)" of the T3 Phase 3 plan. Nothing in this stack inserts `#xum-pre`: a later PR (4b) makes `xum server` paint it for `/`. Until then every change here is a no-op, and the gates (Lighthouse, page timing, first-load bytes) run once on the stack's top against the merge base; their table is on coder#6381. ## Implementation - `useFirstRunViewQueueGuard(loaded, wizardIsFirst)` removes the view in a layout effect when the queue is loaded and the wizard is not first: already viewed, the viewed list could not be read, splash screens off or no API, or another splash comes first. - `onboardingWizardViewed` (`storage.ts`, registered as `"cache"` like `uiTheme`): `cacheViewedWizard()` sets it when the loaded viewed list contains the wizard id, and after `dismiss()` marks the wizard viewed. - coder#6351 (open) changes the same file to keep a shown splash on a lost connection. That rule stands: a lost connection is the `reconnecting` status, where coder#6378's guard removes the static view, not the queue. ## Validation | Test (jest, `tests/ui/onboarding/providerHandoff.test.ts`) | Mutation that fails it | |---|---| | keeps the static view while the wizard is first | Q2: remove before the queue is loaded | | removes it when viewed / read failed / another splash first / no API | Q1: no queue guard | | caches a viewed wizard on load and on dismiss | Q3: no load-time write. Q4: no dismiss write | - `make crap-check` passes: the effect and the cache write are a named hook and a function with two callers, so `SplashScreenProvider` keeps its CC. - The storage registry tests (`storage.test.ts`, `persistedStateBudget.test.ts`, `legacyLocalStorageCleanup.test.ts`) pass with the new key. ## Risks Low. Without `#xum-pre` the only change is one new 4-byte localStorage value per browser (`true`), written when the wizard is viewed. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$289.12`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=289.12 -->
1 participant
Summary
This PR is layer 5b of the reconnect stack (base: #6331). The settings sections, data hooks and contexts now keep what they show when Xum reconnects. Each new client re-runs their load effects. Before this PR, many of them first cleared their data, showed "Loading…" again or marked a section "not loaded" (which disables its controls). Now a re-run keeps the shown data and replaces it only on success. While there is no live client, kept settings controls that write are disabled or report the offline state. Nothing waits to be sent to a later client.
Like #6331, this is a declared behavior change before the switch (PR 6).
Rules
isConnectionLostError) is ignored. A server cancel on a live socket is a plainAbortErrorand is still reported. Otherwise the pane would wait forever, because no new client comes.Sites
ProjectContext,WorkspaceContextloadingcounts only the first load. Reloads keeploaded. Lost-connection failures are ignored.AgentContext,UILayoutsContextuseAnalytics,useCoderWorkspace,useWorkflowRunById,useWorkspaceHeartbeatuseComposerSuggestions,useCreationWorkspaceSplashScreenProvider; App update channelsConfigSwitchSetting. An offline Tasks edit stays pending with a message.Not changed:
AppLoader(already on the live client),SshPromptDialog(clearing is needed: the server owns the prompt queue),useGoalDefaults(nothing reads its loading flag).Validation
null-gaptoday,kept-gapafter the switch) for each changed family, in the contexts', hooks' and sections' test files. I reverted each production file and ran its test: each fails. Some fail in one mode only, because that mode never reaches the old reset.ServerAccessSection.test.tsx).make static-check,make crap-checkand the touched tests pass.GeneralSection.test.tsxfails 2 tests that also fail on origin/main on my host (🤖 fix: double-failure rollbacks and API-server settings ordering after config write failures #4748 and one that varies).Follow-ups
MCPSettingsSection.refresh()still resets its list on each client change.loadGoalDefaultsturns a lost-connection error into the default goal settings.Risks
Medium. The main risk is data of one scope shown under another. The identity tests cover the project, workspace, template and query keys.
WorkspaceContext.loadingno longer goes back to true on a reconnect, so effects that waited for it now run with the old metadata until the new snapshot lands.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$109.50