fix(app): align server sync with tui lifecycle - #41930
Open
Hona wants to merge 8 commits into
Open
Conversation
Contributor
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.
Comment on lines
56
to
60
| 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 } }), |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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. The event-stream handshake, query ownership, catalog projection, reconnect, and route error boundary must agree about that lifecycle.
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 relevant flow was:
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
The primitive choice matters here.
TanStack Solid Query documents
createQueryoptions as reactive. Signal reads must occur inside the options accessor:This is the correct primitive for server state that belongs to a mounted Solid owner. A connection-status change updates
enabled, and TanStack starts or stops automatic query work from that state.TanStack also documents two facts that are easy to combine incorrectly:
status: "pending",fetchStatus: "idle", andisLoading: false.datais a Solid resource. A read can activateSuspensewhen data is not available.Sources:
dataas a Solid resourcequeryOptionsQueryClientmethodsThe old readiness check used
!isLoading. That is true for a disabled query with no data. An intermediate fix useddata !== undefined. That reads the resource to answer a status question.The final projection uses status fields for status:
It reads
query.dataonly after that guard.isRefetchErrorkeeps valid cached data visible when a background refresh fails.This matches the purpose of each primitive:
createQueryqueryOptionsenabledisSuccessorisRefetchErrordata, after the status guardqueryClient.fetchQuerySolid preserves getters on
createStore. Reads inside a getter remain tracked. The previous global-provider fallback did not use a getter. It copiedglobalStore.provideronce, usually while it was empty. The child store could never observe a later global catalog. The PR passes a live getter instead.Source: Solid
createStoregettersOne query cache and one key
The old provider tree had two nested
QueryClientProviderinstances. Long-lived server contexts were created under one client. Route controls and dialogs could use the other client. A refetch could update a cache thatModelsProviderdid not read.The PR keeps one QueryClient at the app base. Server contexts, route controls, and dialogs now share it.
Windows exposed a second identity split. The live netlog showed both forms:
Those strings produced different query keys. A successful response under one key did not prove that the observer under the other key had data. Directory bootstrap now uses the same
pathKeynormalization as the child observer and prompt control.After the fix, the cold-start netlog contains only the forward-slash key for that directory.
The event stream is the admission gate
V2 catalog and location data can change while plugins and integrations initialize. Starting queries before the event stream is connected creates a gap:
ServerSDKnow owns the stream lifecycle. It requiresserver.connectedas the first event. Only then doesconnection.status()becomeconnected.Global and directory queries use that status in their reactive
enabledoption. Reconnect follows the same sequence:Catalog events invalidate the exact server and directory key. Location events refresh location, VCS, skills, web search, and shell data for the affected directory. Different directories remain independent.
queryClient.fetchQueryis intentional in this event path. This work is imperative. A server fact changed, so the cache must load a new snapshot. UI ownership still uses declarativecreateQuery.“Available providers” is not “providers that can be connected”
The empty provider dialog had a second defect.
/api/providerreturns providers that are available to the model catalog. It is not the provider-definition inventory. The connect dialog used that response as if it contained all integrations.The correct source is
/api/integration.The new
loadIntegrationsQuery()uses the same server scope and normalized directory key as the catalog query.useIntegrations()owns it withcreateQuery. The connect dialog and the unpaid-model dialog use that list. The selected integration still resolves its current methods throughintegration.get.The live local server returned:
The model list continues to use
/api/providerplus/api/model. This separation is important:Integration and catalog events refresh both caches.
Session events use the same lifecycle
The stream adapter now carries current V2 events into the app projection.
ServerSessionapplies current Session events, pending inputs, forms, messages, and transient state. Transient hydration records a revision before its HTTP load. A newer stream event changes that revision, so an older HTTP result cannot overwrite the event projection.On reconnect, pinned Sessions refresh. Active Session status is reconciled in both directions. A Session that is absent from the active response becomes idle. A Session that is present becomes busy.
The event consumer also batches adjacent text, reasoning, tool-input, and compaction deltas. It keeps event order while it reduces render work.
Missing Sessions belong to the Session route
A target Session can fail before
TargetSessionRouteContentmounts. For example, target-server providers can resolve a stale Session ID during provisioning.The old Session error boundary was inside that provisioning subtree. A typed
SessionNotFoundErrorcould bypass it and reach the root renderer boundary.The route now has two boundaries:
The outer boundary catches provisioning failures. The inner boundary keeps the existing target-scoped recovery behavior. Both use the same typed Session-not-found predicate.
Local development identity
The desktop development script already set
OPENCODE_CHANNEL=local. The desktop Vite config acceptedlocal, but the shared app Vite plugin did not. It replaced the value withdev.The shared plugin now accepts
local. A coldbun dev:desktopstart showsLOCAL, uses a2.0.0-local-*server version, and keeps the local channel database.Evidence from the real process
The investigation used the running Electron renderer and its real sidecar. It did not use mocked server state.
The cold-start renderer showed:
The live model menu contained OpenCode, OpenAI, and Google groups. The real integration query returned 185 entries. The netlog showed normalized provider and model URLs. App type checking completed after the final upstream merge.
Earlier focused checks on this branch also covered the server SDK, Session projection, global sync reducers, catalog invalidation, and browser model-selection flow. No public Protocol or Server
HttpApichanged in this PR. The generated client is only consumed through its existing integration API.Result
The app now has one reproducible rule:
Each boundary has one owner, one key, and one reactive source. That removes the state in which HTTP says “success,” the control says “ready,” and the dialog still says “empty.”