[codex] Rewrite client connection architecture#2978
Draft
juliusmarminge wants to merge 7 commits into
Draft
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
vouch:trusted
Contributor
|
🚀 Expo continuous deployment is ready!
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
juliusmarminge
force-pushed
the
codex/connection-state-audit
branch
from
16c9aba to
2a29e34
Compare
juliusmarminge
changed the title
juliusmarminge
changed the base branch from
main
to
codex/connection-infra-otel-base
juliusmarminge
force-pushed
the
codex/connection-state-audit
branch
from
2a29e34 to
b976d5f
Compare
Comment on lines
+87
to
+113
| const openDatabase = Effect.fn("web.connectionStorage.openDatabase")(function* () { | ||
| return yield* Effect.callback<IDBDatabase, ConnectionTransientError>((resume) => { | ||
| if (typeof indexedDB === "undefined") { | ||
| resume( | ||
| Effect.fail(catalogError("open", "IndexedDB is unavailable in this browser context.")), | ||
| ); | ||
| return; | ||
| } | ||
| const request = indexedDB.open(DATABASE_NAME, DATABASE_VERSION); | ||
| request.addEventListener("upgradeneeded", () => { | ||
| if (!request.result.objectStoreNames.contains(CATALOG_STORE_NAME)) { | ||
| request.result.createObjectStore(CATALOG_STORE_NAME); | ||
| } | ||
| if (!request.result.objectStoreNames.contains(SHELL_STORE_NAME)) { | ||
| request.result.createObjectStore(SHELL_STORE_NAME); | ||
| } | ||
| if (!request.result.objectStoreNames.contains(THREAD_STORE_NAME)) { | ||
| request.result.createObjectStore(THREAD_STORE_NAME); | ||
| } | ||
| }); | ||
| request.addEventListener("error", () => { | ||
| resume(Effect.fail(catalogError("open", request.error ?? "Unknown IndexedDB error"))); | ||
| }); | ||
| request.addEventListener("success", () => { | ||
| resume(Effect.succeed(request.result)); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🟡 Medium connection/webConnectionStorage.ts:87
When indexedDB.open() needs to upgrade the schema but another tab holds an older version, the blocked event fires and the Effect never completes — neither success nor error handlers execute until the blocking tab closes. If that tab never closes, openDatabase hangs indefinitely. Consider handling the blocked event by resuming with a descriptive error or triggering a retry with timeout.
+ request.addEventListener("blocked", () => {
+ resume(Effect.fail(catalogError("open", "Database upgrade blocked by another tab")));
+ });🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/connection/webConnectionStorage.ts around lines 87-113:
When `indexedDB.open()` needs to upgrade the schema but another tab holds an older version, the `blocked` event fires and the Effect never completes — neither `success` nor `error` handlers execute until the blocking tab closes. If that tab never closes, `openDatabase` hangs indefinitely. Consider handling the `blocked` event by resuming with a descriptive error or triggering a retry with timeout.
Evidence trail:
apps/web/src/connection/webConnectionStorage.ts lines 87-114 at REVIEWED_COMMIT: `openDatabase` registers handlers for `upgradeneeded` (line 96), `error` (line 107), and `success` (line 110), but not for `blocked`. IndexedDB spec: https://w3c.github.io/IndexedDB/#request-api — the `blocked` event fires on IDBOpenDBRequest when the open operation is blocked by existing connections, and `success`/`error` don't fire until the block is resolved.
fb36d5e to
8de82fb
Compare
juliusmarminge
force-pushed
the
codex/connection-state-audit
branch
4 times, most recently
from
9ba3552 to
a01b7f9
Compare
juliusmarminge
force-pushed
the
codex/connection-infra-otel-base
branch
from
ced8bdf to
22b4b24
Compare
juliusmarminge
force-pushed
the
codex/connection-state-audit
branch
4 times, most recently
from
409fd6c to
ecd84a7
Compare
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
juliusmarminge
force-pushed
the
codex/connection-state-audit
branch
from
ecd84a7 to
f5c0afe
Compare
…t and tracing - Updated the EnvironmentSupervisor to handle connection states more effectively, including new states for connecting and available. - Introduced tracing for connection attempts to capture detailed error information and improve debugging. - Removed the environment runtime state management code as it was deemed unnecessary. - Adjusted tests to reflect changes in connection state handling and ensure proper functionality. - Enhanced error handling in relay tracing to ensure safe error propagation without affecting application behavior.
- Refactored EnvironmentRegistry to utilize new EnvironmentServices and EnvironmentServicesFactory. - Replaced runtime-related methods with service-based methods for better abstraction. - Introduced new layers for environment services, including commands and threads. - Removed deprecated rpcGenerationChanges and other unused methods from EnvironmentRegistryService. - Updated tests to reflect changes in the registry and runtime structure. - Cleaned up unused imports and adjusted types accordingly.
…ment - Deleted filesystemBrowseState and sourceControlDiscoveryState modules along with their associated tests. - Removed related imports from index.ts and knownEnvironment.ts. - Updated knownEnvironment tests to remove unused HTTP base URL checks. - Refactored shellSnapshotState and threadDetailState to simplify interfaces. - Cleaned up vcsRefState and vcsStatusState by removing unused types and functions.
| ); | ||
| if (!isSignedIn || !userId) { | ||
| setManagedRelaySession(appAtomRegistry, null); | ||
| if (previousAccount !== null) { |
Contributor
There was a problem hiding this comment.
🟡 Medium cloud/managedAuth.tsx:59
On first render when signed out, previousAccount is undefined, so undefined !== null evaluates to true and queueAccountCleanup() runs unnecessarily. This triggers removeRelayEnvironments() and relay client token cache reset even though no account was ever established. Change the condition to if (previousAccount) so cleanup only happens when transitioning from an actual signed-in state.
- if (previousAccount !== null) {
+ if (previousAccount) {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/cloud/managedAuth.tsx around line 59:
On first render when signed out, `previousAccount` is `undefined`, so `undefined !== null` evaluates to `true` and `queueAccountCleanup()` runs unnecessarily. This triggers `removeRelayEnvironments()` and relay client token cache reset even though no account was ever established. Change the condition to `if (previousAccount)` so cleanup only happens when transitioning from an actual signed-in state.
Evidence trail:
apps/web/src/cloud/managedAuth.tsx lines 26, 35, 37, 57-61, 63 at REVIEWED_COMMIT. Line 26: ref initialized to `undefined`. Line 35: `previousAccount = observedAccountRef.current` (undefined on first render). Line 59: condition `previousAccount !== null` doesn't account for `undefined`. Line 63: correctly checks all three states (`!== undefined && !== null && !== userId`).
| ? (activeSavedEnvironmentRuntime?.connectionState ?? "disconnected") | ||
| : "connected"; | ||
| const primaryEnvironmentId = primaryEnvironment?.environmentId ?? null; | ||
| const activeEnvironment = |
Contributor
There was a problem hiding this comment.
🟡 Medium components/ChatView.tsx:1119
The refactored code removed the guard that excluded the primary environment from unavailability checks. Now activeEnvironmentUnavailable is computed for all environments including the primary, so if the primary's connection.phase is temporarily "connecting" or "available" during startup, the UI shows an unavailable banner and blocks message dispatch — behavior that never occurred before.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/ChatView.tsx around line 1119:
The refactored code removed the guard that excluded the primary environment from unavailability checks. Now `activeEnvironmentUnavailable` is computed for all environments including the primary, so if the primary's `connection.phase` is temporarily `"connecting"` or `"available"` during startup, the UI shows an unavailable banner and blocks message dispatch — behavior that never occurred before.
Evidence trail:
Old guard at MERGE_BASE in ChatView.tsx: `activeThread.environmentId !== primaryEnvironmentId` check in `activeSavedEnvironmentRecord` assignment; new code at REVIEWED_COMMIT ChatView.tsx lines 1119-1123 with no primary guard; `AVAILABLE_CONNECTION_STATE` defined at packages/client-runtime/src/connection/model.ts:148-154 with `phase: "available"`; `activeEnvironmentUnavailable` used to block sends at ChatView.tsx:2806, ChatView.tsx:3427; used to block revert at ChatView.tsx:2746; used for banner at ChatView.tsx:1353; `isConnecting` always false at ChatView.tsx:904 (`_setIsConnecting` unused).
| (patch: Partial<UnifiedSettings>) => { | ||
| const { serverPatch, clientPatch } = splitPatch(patch); | ||
|
|
||
| if (Object.keys(serverPatch).length > 0) { |
Contributor
There was a problem hiding this comment.
🟡 Medium hooks/useSettings.ts:204
When primaryEnvironment is null, server settings are optimistically applied locally via applySettingsUpdated() but the RPC call to serverActions.updateSettings() is skipped. The user sees the change in the UI, but on refresh the setting reverts because it was never persisted to the server.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/hooks/useSettings.ts around line 204:
When `primaryEnvironment` is `null`, server settings are optimistically applied locally via `applySettingsUpdated()` but the RPC call to `serverActions.updateSettings()` is skipped. The user sees the change in the UI, but on refresh the setting reverts because it was never persisted to the server.
Evidence trail:
apps/web/src/hooks/useSettings.ts lines 197-225 (REVIEWED_COMMIT) — independent guards for optimistic update (line 206: checks `currentServerConfig`) vs RPC persist (line 209: checks `primaryEnvironment`). apps/web/src/connection/useWebEnvironments.ts lines 59-68 — `useWebPrimaryEnvironment()` returns `WebEnvironmentPresentation | null`. apps/web/src/rpc/serverState.ts line 58 — `serverConfigAtom` initialized to `null`. apps/web/src/rpc/serverState.ts line 176-178 — `resolveServerConfig` sets the atom. apps/web/src/connection/WebServerStateProjection.tsx lines 27-43 — sets config snapshot from primary environment data, but never clears atom when `environmentId` becomes null. apps/web/src/rpc/serverState.ts lines 169-172 — only test reset exists, no production clearing of stale config.
Contributor
There was a problem hiding this comment.
🟡 Medium
When the toast is created while primaryEnvironment is loading (null), clicking "Update" silently does nothing because runUpdates captures the stale null value in its closure. The effect re-runs when primaryEnvironment resolves, but it early-returns at line 174 because activeToastRef.current is already set, so the callback is never recreated with the resolved environment value.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/ProviderUpdateLaunchNotification.tsx around line 186:
When the toast is created while `primaryEnvironment` is loading (`null`), clicking "Update" silently does nothing because `runUpdates` captures the stale `null` value in its closure. The effect re-runs when `primaryEnvironment` resolves, but it early-returns at line 174 because `activeToastRef.current` is already set, so the callback is never recreated with the resolved environment value.
Evidence trail:
apps/web/src/components/ProviderUpdateLaunchNotification.tsx lines 163-305 (REVIEWED_COMMIT): The useEffect at line 163 has primaryEnvironment in its deps (line 303). runUpdates at line 190 captures primaryEnvironment from closure; line 191 checks `!primaryEnvironment` and returns early if null. Line 179 adds notificationKey to seenProviderUpdateNotificationKeys. Line 173 checks seenProviderUpdateNotificationKeys.has(notificationKey), and line 174 checks activeToastRef.current — both are truthy on re-run, causing early return. The toast callback at line 269 (onClick: runUpdates) is never recreated with the resolved primaryEnvironment value.
Split connection, authorization, RPC, state, and platform concerns into composable modules. Migrate web and mobile consumers to narrow subpath exports and remove legacy stores, barrels, and compatibility directories. Co-authored-by: codex <codex@users.noreply.github.com>
Comment on lines
+86
to
+94
| function exitUnlessInterrupted<A, E, R>( | ||
| effect: Effect.Effect<A, E, R>, | ||
| ): Effect.Effect<Exit.Exit<A, E>, never, R> { | ||
| return Effect.matchCauseEffect(effect, { | ||
| onFailure: (cause) => | ||
| Cause.hasInterrupts(cause) ? Effect.interrupt : Effect.succeed(Exit.failCause(cause)), | ||
| onSuccess: (value) => Effect.succeed(Exit.succeed(value)), | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 Medium connection/supervisor.ts:86
exitUnlessInterrupted uses Cause.hasInterrupts(cause) on line 91, which returns true when the cause contains any interrupt reason even if it also contains typed failures. When a Cause has both an interrupt and a typed failure (e.g., from race conditions), the function calls Effect.interrupt and discards the typed failure. The caller in failureFromExit then receives { _tag: "Interrupted" } instead of the actual error. Per the Cause documentation, hasInterruptsOnly should be used when you want to propagate interrupts only when there are no other failure reasons.
function exitUnlessInterrupted<A, E, R>(
effect: Effect.Effect<A, E, R>,
): Effect.Effect<Exit.Exit<A, E>, never, R> {
return Effect.matchCauseEffect(effect, {
onFailure: (cause) =>
- Cause.hasInterrupts(cause) ? Effect.interrupt : Effect.succeed(Exit.failCause(cause)),
+ Cause.hasInterruptsOnly(cause) ? Effect.interrupt : Effect.succeed(Exit.failCause(cause)),
onSuccess: (value) => Effect.succeed(Exit.succeed(value)),
});
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/client-runtime/src/connection/supervisor.ts around lines 86-94:
`exitUnlessInterrupted` uses `Cause.hasInterrupts(cause)` on line 91, which returns `true` when the cause contains *any* interrupt reason even if it also contains typed failures. When a `Cause` has both an interrupt and a typed failure (e.g., from race conditions), the function calls `Effect.interrupt` and discards the typed failure. The caller in `failureFromExit` then receives `{ _tag: "Interrupted" }` instead of the actual error. Per the `Cause` documentation, `hasInterruptsOnly` should be used when you want to propagate interrupts only when there are no other failure reasons.
Evidence trail:
packages/client-runtime/src/connection/supervisor.ts line 91 — `Cause.hasInterrupts(cause)` used in `exitUnlessInterrupted`
packages/client-runtime/src/connection/supervisor.ts line 177 — `Cause.hasInterruptsOnly(exit.cause)` used in `failureFromExit`
packages/client-runtime/src/connection/supervisor.ts line 438 — `!Cause.hasInterruptsOnly(establishment.exit.cause)` used in defect check
packages/client-runtime/src/state/shell.ts line 134 — `Cause.hasInterruptsOnly(cause)` in identical pattern
packages/client-runtime/src/state/threads.ts line 199 — `Cause.hasInterruptsOnly(cause)` in identical pattern
pnpm-workspace.yaml line 19 — `effect: 4.0.0-beta.78`
- Implemented `useEnvironmentQuery` for handling environment data fetching and error management. - Created state management for relay, review, server, session, shell, source control, terminal, and threads. - Introduced hooks for managing terminal sessions and actions, including attaching, writing, resizing, and clearing terminals. - Added support for source control actions such as pulling, publishing repositories, and managing threads. - Enhanced VCS integration with actions for listing refs and managing worktrees.
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
1 participant
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.
Summary
Stack
Validation
vp checkvp run typecheckvp run lint:mobileThe stacked tree is byte-for-byte identical to the original pre-split PR tip.