Repository navigation
🤖 refactor: right-sidebar panes and dialogs keep shown data while reloading - #6331
Conversation
ThomasK33
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ 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: 837e6c6078
ℹ️ 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".
837e6c6 to
d1620a3
Compare
d1620a3 to
1e4054a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e4054a039
ℹ️ 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".
6c55db0 to
19d6f33
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19d6f33c2f
ℹ️ 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".
19d6f33 to
f4b6520
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4b6520cce
ℹ️ 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 Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48cb81d355
ℹ️ 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".
48cb81d to
f9ab315
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9ab315d69
ℹ️ 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 Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbd1e36b36
ℹ️ 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 note: this PR passed its 6-round review limit, and the findings kept coming from one design choice: kept panes stayed editable offline and queued their writes for the next client. Each fix exposed a new edge case (unmount, modal close, server cancel). I am simplifying the design instead of adding more mechanism. While there is no live client, a kept pane stays visible but read-only: its write controls are disabled, and no write is queued. The 6 open threads (run-header actions, DevTools Clear, expanded card offline, unsent edits after unmount for instructions and goal defaults, manual useCallback) are all addressed by this one change. I will answer each of them when it lands. Generated with |
983d414 to
249077c
Compare
…le reloading A new API client after a reconnect re-runs each dialog's read. The dialog now keeps what it shows until the new read lands, ignores reads cut off by the lost connection, and resets only for another workspace, task or process (the viewers are keyed by it). Signed-off-by: Thomas Kosiewski <tk@coder.com>
A new API client re-subscribes; the shown runs (and the Workflows empty state with its script list) stay until the new snapshot replaces them. Only another workspace clears them. Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ep shown data while reloading A new API client re-runs each load. The panes keep what they show until the new read lands: no loading state over loaded data, no reset when a read is cut off by the lost connection. The goal defaults stay editable during a reload and an edit made meanwhile wins over the reload's older answer; another workspace still clears its override. Signed-off-by: Thomas Kosiewski <tk@coder.com>
Signed-off-by: Thomas Kosiewski <tk@coder.com>
…connecting Without a client, the browser tab keeps its discovered sessions and page tabs, and the sidebar keeps the last known debug-logs and desktop values (so it does not drop those tabs). A listing or read cut off by the lost connection shows no error and changes nothing. The two sidebar reads move into named hooks. Signed-off-by: Thomas Kosiewski <tk@coder.com>
…rview keeps servers per project The chat-instructions editor and the goal defaults stay editable while Xum reconnects. An edit that no client received (made without a client, or its save cut off by the lost connection) looked saved, and the reload skipped it as a local edit, so the backend never got it. Both now keep that edit and send it to the next client, and the next read does not show over it. ProjectPage keeps the MCP overview mounted across project navigation. The overview now keeps the shown servers next to their project, so a cut-off load for project B no longer leaves project A's servers on screen.
…client reads The Workflows tab, the plan dialog and the transcript dialog could open while there was no client. They then stopped loading (the plan dialog also kept its offline error), so after the reconnect they showed the empty state or 'No transcript loaded' during the first read. Each now shows its loading state until its first load lands, and keeps already shown data on ordinary reconnects.
… its error This layer ignored abort-named failures as "the connection was lost". isAbortError() also matches a server cancel on a live socket (a plain AbortError). No new client comes to re-run the effect then, so the pane stopped for good: the MCP overview kept its spinner over the old list, and a process dialog stopped polling without an error. Every check this layer added now uses isConnectionLostError(). The effects' own cleanup aborts stay handled by their cancelled flags.
Panes keep what they show while Xum reconnects, but controls that need the server did nothing: - Workflows: Run and Start are disabled without a live client. - Goal board: promote, remove, archive, revive, reorder, queue and edit report "Not connected" without a live client; an edit stays open. - Browser tab: picking another session while offline clears the previous session's page tabs. - Background process: a viewer opened offline (or whose first read was cut off) ends its loading state and shows the error; it shows "Loading…" again once a client reads. - DevTools: an expanded run whose detail read was cut off by the lost connection reads it again on the next client instead of keeping the connection error.
… later client Simplify the reconnect design. A pane that keeps its data while Xum reconnects is now read-only until a live client is back, instead of staying editable and queueing its writes for the next client: - Chat instructions: the textarea and toggle are disabled offline. The unsent-edit replay is gone; a save cut off by the lost connection shows its error. A settled save still clears the dirty flag. - Goal defaults: the override controls (toggles, inputs, checkbox) are disabled offline. The writer no longer keeps an unsent write; a failed write (a lost connection included) shows its error. Writes stay serialized, and a read that began before a write does not show over it. - Workflow run header (Interrupt, Resume, Retry, Re-run) and DevTools Clear are disabled without a live client. - An expanded DevTools run with no steps shows its spinner until the detail loads (it loads on the next client), not "No steps recorded". - The manual useCallback hooks this layer added to the chat-instructions editor are gone (React Compiler memoizes).
249077c to
97d2a4a
Compare
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0897428cc6
ℹ️ 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".
…ing (coder#6351) ## Summary This PR is layer 5b of the reconnect stack (base: coder#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 coder#6331, this is a declared behavior change before the switch (PR 6). ## Rules 1. A re-run keeps the shown data. "Loading" shows only when nothing is shown yet. A change of the selected scope (project, workspace, template, query) still clears the old data. 2. Only a lost connection (`isConnectionLostError`) is ignored. A server cancel on a live socket is a plain `AbortError` and is still reported. Otherwise the pane would wait forever, because no new client comes. 3. Settings forms never overwrite a value the user is editing, and a re-run never marks a loaded section "not loaded". 4. Offline, a kept control that writes is disabled or reports the offline state. ## Sites | Area | Change | |---|---| | `ProjectContext`, `WorkspaceContext` | `loading` counts only the first load. Reloads keep `loaded`. Lost-connection failures are ignored. | | `AgentContext`, `UILayoutsContext` | Agents clear only for another scope. Presets stay offline. | | `useAnalytics`, `useCoderWorkspace`, `useWorkflowRunById`, `useWorkspaceHeartbeat` | The data records which query it answers. Spinners show only on first load or a new scope. A window focus while offline no longer clears the Coder config. | | `useComposerSuggestions`, `useCreationWorkspace` | Plugin commands carry their workspace ID. A lost request is not cached as loaded. Branch loading returns only for a new project. | | Backup, Experiments, General, Heartbeat, Tasks, Secrets, ServerAccess, MCP sections; `SplashScreenProvider`; App update channels | Keep shown values. The rule 4 controls are disabled offline: ServerAccess revoke/refresh, Secrets Save, the General toggles and inputs, the Heartbeat fields, Experiments warning buttons, `ConfigSwitchSetting`. 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 - Both-mode tests (`null-gap` today, `kept-gap` after 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. - One test checks that a server cancel while connected is still reported (`ServerAccessSection.test.tsx`). - `make static-check`, `make crap-check` and the touched tests pass. `GeneralSection.test.tsx` fails 2 tests that also fail on origin/main on my host (coder#4748 and one that varies). ## Follow-ups - `MCPSettingsSection.refresh()` still resets its list on each client change. - `loadGoalDefaults` turns 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.loading` no 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`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$109.50`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=109.50 -->
1 participant
Summary
This PR is layer 5a of the reconnect stack (base: #6323). Right-sidebar panes and dialogs 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 or swapped it for a spinner, so the pane blanked or flashed although nothing had changed. Now a re-run keeps the shown data, replaces it only on success, and ignores a load cut off by the lost connection. A change of the selected workspace, process or session still clears the old data.
This is a declared behavior change before the switch (PR 6): it also stops the flash at today's reconnects.
Implementation
PlanFileDialogPlanFileBody(keyed by workspace, mounted only while open) and ausePlanFilehook. Offline keeps the plan. A cut-off read shows no error. The markdown is keyed by content: Streamdown does not re-render a block whose position in the source is the same.SubagentTranscriptDialogtaskIdand starts loading. A cut-off read shows no error.BackgroundBashOutputDialogworkspaceId:processId.useDevToolsSubscription,useWorkflowRuns,WorkflowsTab(useWorkflowLauncher)useGoalBoard,GoalDefaultsSection(useWorkspaceGoalOverride)ProjectMCPOverviewInstructionsTab,AdditionalSystemContextScratchpadBrowserTabWorkflowRunHeader,DevToolsTabRightSidebar(useLlmDebugLogsEnabled,useDesktopAvailable)false, which removed the Debug or Desktop tab from the saved layout.Not changed:
ReviewPanel(its loading flag drives only the refresh spinner),useReadMore(the shown context stays, the loading row replaces only the button),OutputTab(the listed line is a Delete handler, not a loader).The new hooks and child components also keep each function inside the CRAP gate.
Validation
null-gaptoday,kept-gapafter the switch), 25 tests:dialogsReconnect.test.tsx,feedsReconnect.test.tsx,loadersReconnect.test.tsx,BrowserTab.reconnect.test.tsx,RightSidebar.reconnect.test.tsx, plusofflinePanes.test.tsx(each kept pane is read-only while offline and editable again after the reconnect). Each shows data, drops, reconnects with a reload that is still pending or fails, and checks that the data stays, then that the new data shows. The identity tests check that another workspace or process clears the old data.kept-gap: innull-gapthe effect cleanup runs before the cut-off call settles.make static-checkandmake crap-checkpass.bun teston the touched folders: 1120 pass, 1 fail (HighlightedCode.test.tsx: "a mounted block highlights once a worker is ready after a start time-out"). That test also fails on origin/main on my host.Known gaps (follow-ups, tracked in #6383)
BrowserTab's connect effect still disconnects the browser bridge when there is no client.useReadMore'sreadFileLines(...).thenhas no.catch. A drop during the read leaves an unhandled rejection and a stuck loading row.Risks
Medium, limited to the right sidebar and its dialogs. The main risk is stale data under a new identity. The identity tests cover the workspace, process and session keys.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$81.58