fix(app): unify v2 server and session lifecycle - #41930
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aligns the app’s server-scoped data flow with the event-stream lifecycle by gating server queries on the initial server.connected handshake, refreshing/invalidation on reconnect, and extending V2 session projection/hydration so transient state can be safely rehydrated after reconnects.
Changes:
- Gate server-scoped queries and bootstrapping on the event-stream handshake and add reconnect-aware refresh/invalidation hooks.
- Introduce catalog/connection/location sync helpers to refresh connection-sensitive query data from events.
- Improve V2 session projection to include transient pending inputs/forms and handle additional V2 event cases (including removals and non-initial instruction updates).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/app/src/pages/session/composer/session-composer-controls.ts | Gate provider queries on connection status and adjust provider-loading behavior in composer controls. |
| packages/app/src/context/server-sync/location.ts | New event-driven location sync to refresh per-directory caches and project shell events. |
| packages/app/src/context/server-sync/location.test.ts | Tests for location sync shell projection and refresh triggers. |
| packages/app/src/context/server-sync/connection.ts | New connection sync to invalidate disconnected state and react to handshake completion. |
| packages/app/src/context/server-sync/connection.test.ts | Tests for connection invalidation and handshake synchronization. |
| packages/app/src/context/server-sync/catalog.ts | New catalog sync to invalidate/reload provider catalog per location and after reconnect. |
| packages/app/src/context/server-sync/catalog.test.ts | Tests for catalog invalidation by directory and on connection. |
| packages/app/src/context/server-sync.tsx | Wire handshake-gated queries, reconnect refresh, catalog/connection/location sync, and transient hydration. |
| packages/app/src/context/server-sync.test.ts | Add coverage for active session status reconciliation after reconnect. |
| packages/app/src/context/server-session.ts | Project additional V2 transient events (pending inputs/forms), add transient hydration guards, and tweak V2 message projection. |
| packages/app/src/context/server-session.test.ts | Tests for pending/forms projection and transient hydration race protection. |
| packages/app/src/context/server-session-v2-reducer.ts | Extend reducer to append admitted inputs, record removals, and project non-initial instruction updates. |
| packages/app/src/context/server-session-v2-reducer.test.ts | Tests for instruction updates and updated pending-input folding behavior. |
| packages/app/src/context/server-sdk.tsx | Require server.connected as stream handshake; add connection status tracking and reconnect loop with timeout/backoff. |
| packages/app/src/context/server-sdk.test.ts | Tests for handshake requirement helper and related stream behaviors. |
| packages/app/src/context/global.tsx | Remove per-server QueryClient creation from global server context return value. |
| packages/app/src/context/global-sync/event-reducer.ts | Stop using server.connected as a global refresh trigger (delegated to connection sync). |
| packages/app/src/context/global-sync/event-reducer.test.ts | Update tests to reflect server.connected refresh responsibility shift. |
| packages/app/src/context/global-sync/child-store.ts | Gate per-directory queries on connection handshake and tighten provider readiness detection. |
| packages/app/src/context/global-sync/child-store.test.ts | Tests for handshake gating and provider readiness when provider query is cancelled/unsuccessful. |
| packages/app/src/context/global-sync/bootstrap.ts | Refactor bootstrap async sequencing without changing behavior (remove nested async IIFE). |
| packages/app/src/app.tsx | Wrap target session route content in a session-scoped error boundary; remove QueryProvider wrapper usage in shell. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| loading: | ||
| (local.agent.visible() && agentsQuery.isLoading) || | ||
| providersQuery.isLoading || | ||
| globalProvidersQuery.isLoading, | ||
| !providersQuery.isSuccess || | ||
| !globalProvidersQuery.isSuccess, | ||
| }, |
| scope: serverSDK.scope, | ||
| queryClient, | ||
| active: () => Object.keys(children.children).filter(children.active).map(pathKey), | ||
| info: (directory) => serverSDK.api.location.get({ location: { directory } }), |
8886ceb to
7674941
Compare
|
Follow-ups from the full event-handling parity audit (TUI vs desktop, every event in 1. Go-upsell and rate-limit dialogs are dead on V2 (server design needed) 2. TUI gaps (separate PR)
3. Desktop pty parity (feature-level) 4. Dead app code cleanup (small follow-up PR)
5. Server: undeclared 204 during location boot |
|
Correction to item 5 of the follow-up comment above: the 204 readiness claim is retracted. The reviewer is right. Re-probing with request-method correlation shows the 204s were CORS preflight responses, not A direct The corrected root cause for the empty agent dropdown: the first agents fetch failed with transport errors while the background service was crash-looping (see #41968), the retry window exhausted, and the heal path was defeated by the re-bootstrap queue guard plus the |
|
This PR cannot be merged into the beta branch due to: Merge conflicts with v2 branch Please resolve this issue to include this PR in the next beta release. |
|
This PR cannot be merged into the beta branch due to: Merge failed Please resolve this issue to include this PR in the next beta release. |
# Conflicts: # packages/app/src/context/server-session-v2-reducer.ts # packages/app/src/context/server-session.ts
|
This PR cannot be merged into the beta branch due to: Merge failed Please resolve this issue to include this PR in the next beta release. |
# Conflicts: # packages/app/src/components/dialog-connect-provider.tsx # packages/app/src/context/server-sdk.tsx # packages/app/src/context/server-sync.tsx
|
This PR cannot be merged into the beta branch due to: Merge failed Please resolve this issue to include this PR in the next beta release. |
|
This PR cannot be merged into the beta branch due to: Merge conflicts with v2 branch Please resolve this issue to include this PR in the next beta release. |
# Conflicts: # packages/app/src/i18n/parity.test.ts
Server sync is a lifecycle, not a fetch
The visible failures looked unrelated:
They had one common cause. The app treated server state as independent HTTP results. The V2 server exposes one ordered lifecycle: an event-stream handshake admits queries, events carry every later fact, and reconnection replays the admission. The handshake, query ownership, catalog projection, event handling, reconnect, and route error boundary must agree about that lifecycle. This PR makes them agree, and it aligns each mechanism with the TUI client, which already lives by these rules.
The contradiction that found the catalog bug
The live Electron app gave a useful contradiction.
GET /api/provider,GET /api/model, andGET /api/model/defaultall returned200. The model control also rendered. That control only rendered after its direct provider queries had reachedisSuccess. However,ModelsProvideranduseProviders()still returned an empty catalog.This excluded the server response and the normalizer. The loss occurred between the TanStack query observer and the child-store projection.
The prompt control and the model list did not use the same readiness source. The prompt control created two extra query observers. The model list read the child-store projection. One path could say "success" while the other path still said "empty." This PR makes the child projection the single source for both readiness and data.
The TanStack contract
TanStack Solid Query documents
createQueryoptions as reactive. Signal reads must occur inside the options accessor:Two documented facts are easy to combine incorrectly:
status: "pending",fetchStatus: "idle", andisLoading: false.datais a Solid resource. A read can activateSuspensewhen data is not available.The old readiness check used
!isLoading, which is true for a disabled query. The projection now uses status fields for status and readsdataonly afterisSuccess || isRefetchError:createQueryqueryOptionsenabledisSuccessorisRefetchErrordata, after the status guardqueryClient.fetchQuerySolid preserves getters on
createStore, so the global-provider fallback is now a live getter instead of a one-time copy taken while the global catalog was still empty.One query cache and one key
The old provider tree had two nested
QueryClientProviderinstances. A refetch could update a cache thatModelsProviderdid not read. The PR keeps one QueryClient at the app base.Windows exposed a second identity split. The live netlog showed the same directory under two names:
Bootstrap wrote under raw backslash keys while observers read under normalized keys, so a successful response under one key proved nothing about the observer under the other. Every directory-scoped bootstrap query key (providers, agents, path, references, mcp, mcp resources) now uses the same normalization as the observers. The live capture after the fix shows one key form and no duplicate fetches.
The connection loop mirrors the TUI client
ServerSDKowns the stream lifecycle with the same shape aspackages/tui/src/context/client.tsx:connect()reports failures as values,{ error, connectedAt }, and never throws into the loop.server.connected; a closed or invalid stream is an error value.console.errormaterial, and a dead server no longer floods the log.Global and directory queries use
connection.status()in their reactiveenabledoption. Reconnection distinguishes itself from first connection: pinned Sessions force-reload only on a true reconnect, because a first connection has no gap to recover, while directory bootstraps queue on both, since a directory opened before the handshake has no other recovery path.Events are the healing mechanism, not a hint
A field bug proved the queue-based refresh insufficient. Swapping to a project while the connection was degraded left the agent dropdown empty forever:
[].agent.updatedevent only queued a re-bootstrap.ensureQueryDatareturned the cached store on the next one. Nothing ever healed.The rule that fixes the class: an event that names changed state must fetch that state and write the projection directly, not hope a future bootstrap notices.
agent.updatedfetches agents and writes the child store.command.updatedfetches commands and writes the child store. The old path only re-bootstrapped, and the command reload inside bootstrap was gated on MCP enablement, so MCP-off directories never refreshed slash commands.project.directories.updatednow works at all: it is a location-scoped event, and the only handler lived in the global branch that location-scoped events can never reach.catalog.updated,integration.updated,integration.connection.updated) invalidate and reload the exact server and directory entry. Location events refresh vcs, skills, websearch, and shell data.A full event audit (every type in
packages/schema/src/event-manifest.ts, TUI handling versus app handling) drove two more fixes:file.watcher.updated. The V2 stream sendsfilesystem.changed. The listener was dead, and the file tree went stale on every external change.session.idle, which has no V2 publisher. They now key onsession.execution.succeededand.interrupted, the TUI's rule; failures keep their separate error path.Remaining audit findings that need design or belong to other packages are tracked in the PR comments.
"Available providers" is not "providers that can be connected"
/api/providerreturns providers available to the model catalog, not the provider-definition inventory. The connect dialog used it as if it were complete and showed almost nothing. The correct source is/api/integration(185 entries on the live server). The newloadIntegrationsQuery()anduseIntegrations()own that list declaratively; the connect and unpaid-model dialogs consume it; a connected integration absent from/api/providerfalls back to its integration metadata.Session events use the same lifecycle
The stream adapter carries current V2 events into the app projection: pending inputs, forms, messages, and transient state. Transient hydration records a revision before its HTTP load, so an older response cannot overwrite a newer event. On reconnect, active session statuses reconcile in both directions. Adjacent text, reasoning, tool-input, and compaction deltas batch without reordering.
Missing Sessions belong to the Session route
A target Session can fail before
TargetSessionRouteContentmounts, so the route now has an outer boundary around target-server provisioning and an inner boundary for target-scoped recovery. Both use the same typed Session-not-found predicate. A stale Session ID no longer reaches the renderer error boundary.The test harness must honor the transport contract
The mock server used to fulfill
/api/eventwith a finite body. The stream closed instantly, the app reconnected every second, and every reconnect redelivered the same events with the same IDs. Real servers never redeliver an event on one stream and mint a freshserver.connectedper connection. Compensating in product code would be the wrong layer, so the harness now honors the contract: mock/api/eventstreams stay open through a fetch patch, each connection gets a fresh handshake ID, and a pump forwards each queued event exactly once. Spec-local mocks that never sentserver.connectednow do.One flake this exposed was upstream and is fixed separately (#41965): the cached-tab paint probe counted the markdown fallback-to-parsed hydration swap as first-paint teardown under CPU load.
A false lead, kept for the record
A capture appeared to show
/api/agentanswering204during location boot. It did not: the204s were CORS preflightOPTIONSresponses, misattributed because the probe filtered by URL without correlating request methods. The server declares and serves200with location-gated handlers. The empty-dropdown root cause was the failed-fetch-plus-broken-heal chain above. Probe lesson: correlaterequestWillBeSentmethod withresponseReceivedstatus before blaming an endpoint.Local development identity
The desktop development script sets
OPENCODE_CHANNEL=local, but the shared app Vite plugin mapped unknown channels todev. It now acceptslocal: a coldbun dev:desktopshowsLOCAL, uses a2.0.0-local-*server version, and keeps the channel database (opencode-local.db).Evidence from the real process
All investigation ran against the live Electron renderer and its real sidecar, no mocked server state:
LOCALbadge, populated model control, full model menu, 185 integrations, seven featured providers.Timeline events are projection data, not card decoration
The V2 stream already carries agent, model, location, skill, compaction, restart, shell, and subagent facts as ordered
SessionMessageInfovalues. Rendering those facts inside tool cards made their order depend on whichever card happened to own them. The timeline now projects them asNoticerows beside user messages, assistant parts, dividers, retries, and errors.This keeps the durable protocol order intact. A completed background shell can say
Shell finished, a subagent can sayExplore failed, and a model switch stays between the same neighboring messages that the CLI uses. The renderer supplies labels and muted detail; it does not reconstruct lifecycle order from component state.Background work has one root-session surface
Foreground shell and subagent calls can block the current drain.
Ctrl+Bnow calls the V2session.backgroundoperation and moves that blocking work behind the normal command boundary. Background state comes from the same client-backed session projection and shell list used elsewhere; blocking jobs are excluded from the already-backgrounded count.The composer is the only action surface. Its pullout can show two independent lines:
Child Sessions do not show the control. Running subagent cards navigate to their child Session, but they do not grow a second background action. Todo and background state share one pullout shell so their geometry, collapse motion, and composer lift remain consistent.
Virtualized callbacks still need a Solid owner
The timeline virtualizer calls resize and anchor hooks after component setup. A dynamic boolean JSX prop such as
shouldAnchorBottom={condition()}is compiled as a lazy getter. With cached measurements, its first read could occur inside a later virtualizer callback, after Solid had left the component owner. Solid then created an undisposable memo and warned:The component interface remains value-based, as normal Solid props should be.
MessageTimelineVieweagerly derives owned local memos fromshouldAnchorBottomandhasScrollGesture, and only those owned accessors cross into the imperative virtualizer callbacks. The parent does not pass signal functions as a special prop protocol.Development warnings now keep their evidence
Source maps can map a stack, but they cannot invent one for a plain
console.warn. The desktop development renderer now appends a source stack to warnings and errors beforeelectron-logmirrors them to the terminal. Identical messages from the same call site are grouped for one second, so a lifecycle bug gives one useful stack and one repeat count instead of a wall of context-free text.Chromium's ResizeObserver loop warning has no responsible JavaScript stack. Development builds therefore record observer creation stacks and the targets touched by recently active observers. When Chromium emits the loop warning, the log names those candidates. Resize callbacks record only a timestamp and target list; production bundles remove the entire diagnostic branch through
import.meta.env.DEV.Server identity is not project identity
The merged settings redesign adds a server selector to provider, model, project, and extension settings. That exposed the same identity rule from the catalog investigation: provider, model, and agent catalogs are directory projections, not server-global lists.
Opening settings from a Session now carries both its selected server and its directory through
SettingsServerScope. The scope reuses the app's one QueryClient, while query keys retain server and normalized-directory identity. Switching to another server clears the directory projection instead of applying the previous server's project path to the new server. A future project selector can provide that missing directory without changing the catalog boundary.CI found two boundary assumptions
The background shell list is advisory UI data. A mock or transitional response with no payload now becomes an empty list before it reaches TanStack Query, whose data contract rejects
undefined. This fixed the same seeded-history smoke failure on Linux and Windows.The new background and notice copy remains in the translation dictionary. It is intentionally English-only for now; no user-facing English is hardcoded at call sites. The old all-locales parity test was removed rather than teaching a general translation gate about temporary exceptions.
Result
One reproducible rule set:
Each boundary has one owner, one key, and one reactive source. That removes the states in which HTTP says "success," the control says "ready," and the dialog says "empty" ΓÇö and the state in which the server says "changed" and the app never hears it.