[codex] Rewrite client connection architecture#2978
Open
juliusmarminge wants to merge 17 commits into
Open
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
juliusmarminge
force-pushed
the
codex/connection-state-audit
branch
from
ecd84a7 to
f5c0afe
Compare
| ); | ||
| 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).
Contributor
There was a problem hiding this comment.
🟡 Medium
The loadBranches callback no longer handles errors from branchState.refresh(). If the branch query fails, setPendingConnectionError is never called and users receive no feedback about the failure. Consider wrapping branchState.refresh() in a try-catch and calling setPendingConnectionError on error, or verify that branchState surfaces errors through a different mechanism that renders this handling unnecessary.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/threads/new-task-flow-provider.tsx around line 388:
The `loadBranches` callback no longer handles errors from `branchState.refresh()`. If the branch query fails, `setPendingConnectionError` is never called and users receive no feedback about the failure. Consider wrapping `branchState.refresh()` in a try-catch and calling `setPendingConnectionError` on error, or verify that `branchState` surfaces errors through a different mechanism that renders this handling unnecessary.
Evidence trail:
- apps/mobile/src/features/threads/new-task-flow-provider.tsx lines 388-416 (REVIEWED_COMMIT): new `loadBranches` with no error handling
- git_diff MERGE_BASE..REVIEWED_COMMIT on same file: removed try-catch with `setPendingConnectionError("Failed to load branches.")` catch block
- apps/mobile/src/state/query.ts lines 24-36 (REVIEWED_COMMIT): `useEnvironmentQuery` returns `{ error, refresh }` where `refresh` is `useAtomRefresh` (returns `void`, line 29)
- git_grep for `branchState\.error` in apps/mobile: zero results — never consumed
- apps/mobile/src/state/use-remote-environment-registry.ts lines 26-32, 114-200: `pendingConnectionError` atom is the UI error surface, only set from `setPendingConnectionError`
Comment on lines
+1808
to
+1835
| (activeThread.session !== null && activeThread.session.status !== "stopped")), |
Contributor
There was a problem hiding this comment.
🟡 Medium components/ChatView.tsx:1808
The envLocked check at line 1808 only excludes status !== "stopped", but the new session model has two additional terminal states "interrupted" and "error". Threads with these statuses will incorrectly have envLocked = true, blocking users from changing branch/env-mode settings on effectively terminated threads. Consider updating the condition to also exclude "interrupted" and "error".
- (activeThread.session !== null && activeThread.session.status !== "stopped")),
+ (activeThread.session !== null &&
+ activeThread.session.status !== "stopped" &&
+ activeThread.session.status !== "interrupted" &&
+ activeThread.session.status !== "error")),🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/ChatView.tsx around line 1808:
The `envLocked` check at line 1808 only excludes `status !== "stopped"`, but the new session model has two additional terminal states `"interrupted"` and `"error"`. Threads with these statuses will incorrectly have `envLocked = true`, blocking users from changing branch/env-mode settings on effectively terminated threads. Consider updating the condition to also exclude `"interrupted"` and `"error"`.
Evidence trail:
packages/contracts/src/orchestration.ts:249-257 (OrchestrationSessionStatus defines: idle, starting, running, ready, interrupted, stopped, error); apps/web/src/session-logic.ts:1252-1260 (derivePhase treats stopped/interrupted/error all as 'disconnected' terminal states); apps/web/src/components/ChatView.tsx:1805-1809 (envLocked check only excludes 'stopped'); apps/web/src/components/ChatView.tsx:1816 (envLocked blocks onEnvironmentChange); apps/web/src/components/ChatView.tsx:2497 (envLocked blocks canOverrideServerThreadEnvMode); apps/web/src/components/ChatView.tsx:3820 (envLocked passed to child component)
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Corrupt catalog skips legacy fallback
- Changed decodeCatalog error handling to use Effect.option and fall through to the legacy migration path instead of immediately returning EMPTY_CONNECTION_CATALOG_DOCUMENT.
- ✅ Fixed: Cloud switch blocks error retry
- Changed switch value from
connectionState !== "available"to only be true for "connected", "connecting", or "reconnecting" states, allowing users to toggle ON from error/offline states to trigger reconnect.
- Changed switch value from
Or push these changes by commenting:
@cursor push 0369ca79fd
Preview (0369ca79fd)
diff --git a/apps/mobile/src/app/settings/environments.tsx b/apps/mobile/src/app/settings/environments.tsx
--- a/apps/mobile/src/app/settings/environments.tsx
+++ b/apps/mobile/src/app/settings/environments.tsx
@@ -259,7 +259,11 @@
props.onDisconnect();
}}
onToggleError={props.onToggleError}
- value={props.environment.connectionState !== "available"}
+ value={
+ props.environment.connectionState === "connected" ||
+ props.environment.connectionState === "connecting" ||
+ props.environment.connectionState === "reconnecting"
+ }
/>
);
}
diff --git a/apps/mobile/src/connection/catalog-store.ts b/apps/mobile/src/connection/catalog-store.ts
--- a/apps/mobile/src/connection/catalog-store.ts
+++ b/apps/mobile/src/connection/catalog-store.ts
@@ -66,17 +66,25 @@
return cached.value;
}
const raw = yield* storage.getItem(CONNECTION_CATALOG_KEY);
- let catalog: ConnectionCatalogDocumentType;
+ let catalog: ConnectionCatalogDocumentType = EMPTY_CONNECTION_CATALOG_DOCUMENT;
+ let catalogLoaded = false;
if (raw !== null && raw.trim() !== "") {
- catalog = yield* decodeCatalog(raw).pipe(
- Effect.catch((error) =>
- Effect.logWarning("Discarding corrupt mobile connection catalog", error).pipe(
- Effect.andThen(storage.deleteItem(CONNECTION_CATALOG_KEY)),
- Effect.as(EMPTY_CONNECTION_CATALOG_DOCUMENT),
- ),
+ const decoded = yield* decodeCatalog(raw).pipe(
+ Effect.option,
+ Effect.tap((result) =>
+ Option.isNone(result)
+ ? Effect.logWarning("Discarding corrupt mobile connection catalog").pipe(
+ Effect.andThen(storage.deleteItem(CONNECTION_CATALOG_KEY)),
+ )
+ : Effect.void,
),
);
- } else {
+ if (Option.isSome(decoded)) {
+ catalog = decoded.value;
+ catalogLoaded = true;
+ }
+ }
+ if (!catalogLoaded) {
const legacyRaw = yield* storage.getItem(LEGACY_CONNECTIONS_KEY);
catalog =
legacyRaw === null || legacyRaw.trim() === ""You can send follow-ups to the cloud agent here.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 6 total unresolved issues (including 3 from previous reviews).
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Sign-out skips device unregister
- Added a fallback token provider using getToken and previousObservedAccount when previousTokenProviderRef is null during sign-out, ensuring unregister is attempted even if activateSession hasn't run yet.
- ✅ Fixed: Refresh bypasses registration queue
- Changed refreshAgentAwarenessRegistration to route through enqueueDeviceRegistration instead of calling registerDeviceForCurrentUser directly, ensuring serialized access to the relay endpoint.
- ✅ Fixed: Paused envs show unavailable
- Removed 'available' from the connectionState guard in deriveProjectEmptyState so that idle/paused saved connections are no longer conflated with offline or error states.
Or push these changes by commenting:
@cursor push 6e632f9d69
Preview (6e632f9d69)
diff --git a/apps/mobile/src/app/new/index.tsx b/apps/mobile/src/app/new/index.tsx
--- a/apps/mobile/src/app/new/index.tsx
+++ b/apps/mobile/src/app/new/index.tsx
@@ -36,9 +36,7 @@
}
if (
- (catalogState.connectionState === "available" ||
- catalogState.connectionState === "offline" ||
- catalogState.connectionState === "error") &&
+ (catalogState.connectionState === "offline" || catalogState.connectionState === "error") &&
!catalogState.hasLoadedShellSnapshot
) {
return {
diff --git a/apps/mobile/src/features/agent-awareness/remoteRegistration.test.ts b/apps/mobile/src/features/agent-awareness/remoteRegistration.test.ts
--- a/apps/mobile/src/features/agent-awareness/remoteRegistration.test.ts
+++ b/apps/mobile/src/features/agent-awareness/remoteRegistration.test.ts
@@ -313,6 +313,7 @@
vi.mocked(Notifications.getDevicePushTokenAsync).mockClear();
yield* refreshAgentAwarenessRegistration();
+ yield* runBackgroundOperations();
expect(Notifications.getDevicePushTokenAsync).toHaveBeenCalledTimes(1);
}).pipe(Effect.provide(relayTestLayer));
diff --git a/apps/mobile/src/features/agent-awareness/remoteRegistration.ts b/apps/mobile/src/features/agent-awareness/remoteRegistration.ts
--- a/apps/mobile/src/features/agent-awareness/remoteRegistration.ts
+++ b/apps/mobile/src/features/agent-awareness/remoteRegistration.ts
@@ -474,13 +474,9 @@
never,
ManagedRelayClient
> {
- return registerDeviceForCurrentUser().pipe(
- Effect.catch((error) =>
- Effect.sync(() => {
- logRegistrationError("device registration refresh failed", error);
- }),
- ),
- );
+ return Effect.sync(() => {
+ enqueueDeviceRegistration({}, "device registration refresh failed");
+ });
}
export function __resetAgentAwarenessRemoteRegistrationForTest(): void {
diff --git a/apps/mobile/src/features/cloud/CloudAuthProvider.tsx b/apps/mobile/src/features/cloud/CloudAuthProvider.tsx
--- a/apps/mobile/src/features/cloud/CloudAuthProvider.tsx
+++ b/apps/mobile/src/features/cloud/CloudAuthProvider.tsx
@@ -73,7 +73,15 @@
setAgentAwarenessRelayTokenProvider(null);
setManagedRelaySession(appAtomRegistry, null);
if (previousObservedAccount !== null) {
- void queueAccountCleanup(previous);
+ void queueAccountCleanup(
+ previous ??
+ (typeof previousObservedAccount === "string"
+ ? {
+ userId: previousObservedAccount,
+ provider: () => getToken(resolveRelayClerkTokenOptions()),
+ }
+ : null),
+ );
}
return;
}You can send follow-ups to the cloud agent here.
juliusmarminge
force-pushed
the
codex/connection-state-audit
branch
from
926ddd5 to
04d6d46
Compare
Comment on lines
+94
to
+101
| let current: unknown = target; | ||
| while (current && typeof current === "object") { | ||
| if (menuStack.includes(current as HTMLDivElement)) { | ||
| return true; | ||
| } | ||
| current = (current as { parent?: unknown }).parent; | ||
| } | ||
| return false; |
Contributor
There was a problem hiding this comment.
🟢 Low src/contextMenuFallback.ts:94
The fallback traversal in isNodeWithinMenuStack uses .parent (line 99), but DOM nodes expose parentNode or parentElement, not parent. When instanceof Node fails (e.g., cross-frame elements), the traversal climbs via .parent which is always undefined on real DOM nodes, so menuStack.includes never matches and clicks inside submenus incorrectly dismiss the menu.
+ let current: Node | null = target as Node;
+ while (current) {
+ if (menuStack.includes(current as HTMLDivElement)) {
+ return true;
+ }
+ current = current.parentNode;
+ }
+ return false;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/contextMenuFallback.ts around lines 94-101:
The fallback traversal in `isNodeWithinMenuStack` uses `.parent` (line 99), but DOM nodes expose `parentNode` or `parentElement`, not `parent`. When `instanceof Node` fails (e.g., cross-frame elements), the traversal climbs via `.parent` which is always `undefined` on real DOM nodes, so `menuStack.includes` never matches and clicks inside submenus incorrectly dismiss the menu.
Evidence trail:
apps/web/src/contextMenuFallback.ts lines 86-102 (function `isNodeWithinMenuStack`, fallback path with `.parent` at line 99). git_grep for `parentNode|parentElement` in contextMenuFallback.ts returns no results. Menu elements are standard `HTMLDivElement` created at line 167 via `document.createElement('div')`. DOM `Node` interface specifies `parentNode` and `parentElement`, not `parent`.
Comment on lines
+112
to
+113
| const optionByLabel = new Map(question.options.map((option) => [option.label, option])); | ||
| const resolvedValues = values.map((value) => ({ |
Contributor
There was a problem hiding this comment.
🟢 Low acp/XAiAcpExtension.ts:112
When question.options is empty, extractXAiAskUserQuestions shows the user a synthetic "OK" option, but normalizeAnswerForXAi only validates against the original empty question.options. When the user selects "OK", it doesn't match any option, gets treated as freeform text in notes, and selectedLabels incorrectly falls back to ["Other"] instead of ["OK"]. Consider using the same synthetic default logic in normalizeAnswerForXAi so the answer validation matches the displayed options.
- const optionByLabel = new Map(question.options.map((option) => [option.label, option]));
+ const effectiveOptions =
+ question.options.length > 0
+ ? question.options
+ : [{ label: "OK", description: "Continue", preview: undefined }];
+ const optionByLabel = new Map(effectiveOptions.map((option) => [option.label, option]));🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/provider/acp/XAiAcpExtension.ts around lines 112-113:
When `question.options` is empty, `extractXAiAskUserQuestions` shows the user a synthetic "OK" option, but `normalizeAnswerForXAi` only validates against the original empty `question.options`. When the user selects "OK", it doesn't match any option, gets treated as freeform text in `notes`, and `selectedLabels` incorrectly falls back to `["Other"]` instead of `["OK"]`. Consider using the same synthetic default logic in `normalizeAnswerForXAi` so the answer validation matches the displayed options.
Evidence trail:
apps/server/src/provider/acp/XAiAcpExtension.ts lines 49-65 (`extractXAiAskUserQuestions` with synthetic "OK" option at line 63), lines 103-137 (`normalizeAnswerForXAi` validates against original empty `question.options` at line 112, falls back to `["Other"]` at line 134), lines 147-153 (`makeXAiAskUserQuestionResponse` passes original question to `normalizeAnswerForXAi`).
03c78cc to
4c38462
Compare
juliusmarminge
force-pushed
the
codex/hosted-web-tracing-poc
branch
from
4c38462 to
f37abe6
Compare
juliusmarminge
force-pushed
the
codex/connection-state-audit
branch
from
04d6d46 to
582b006
Compare
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Spurious relay cleanup on load
- Added
previousObservedAccount !== undefinedguard to the signed-out branch to match the account-switch branch, preventing cleanup when no user was ever observed as signed in.
- Added
Or push these changes by commenting:
@cursor push 0d6918a8b3
Preview (0d6918a8b3)
diff --git a/apps/mobile/src/features/cloud/CloudAuthProvider.tsx b/apps/mobile/src/features/cloud/CloudAuthProvider.tsx
--- a/apps/mobile/src/features/cloud/CloudAuthProvider.tsx
+++ b/apps/mobile/src/features/cloud/CloudAuthProvider.tsx
@@ -72,7 +72,7 @@
previousTokenProviderRef.current = null;
setAgentAwarenessRelayTokenProvider(null);
setManagedRelaySession(appAtomRegistry, null);
- if (previousObservedAccount !== null) {
+ if (previousObservedAccount !== undefined && previousObservedAccount !== null) {
void queueAccountCleanup(previous);
}
return;You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 582b006. Configure here.
| setManagedRelaySession(appAtomRegistry, null); | ||
| if (previousObservedAccount !== null) { | ||
| void queueAccountCleanup(previous); | ||
| } |
Contributor
There was a problem hiding this comment.
Spurious relay cleanup on load
High Severity
When Clerk first finishes loading and the user is signed out, previousObservedAccount is still undefined, but the sign-out branch treats undefined !== null as a real account transition and runs removeRelayEnvironments() plus token cleanup. That can strip persisted T3 Cloud relay targets on every cold start while logged out, not only after an actual sign-out or account switch.
Reviewed by Cursor Bugbot for commit 582b006. Configure here.
Comment on lines
+153
to
+163
| export function useVcsInitAction(scope: SourceControlActionScope) { | ||
| const init = useAtomSet(vcsEnvironment.init, { mode: "promise" }); | ||
| const action = useCallback(async () => { | ||
| const target = requireScope(scope, "Git init is unavailable."); | ||
| return init({ | ||
| environmentId: target.environmentId, | ||
| input: { cwd: target.cwd }, | ||
| }); | ||
| }, [init, scope]); | ||
| return useAction({ kind: "init", scope, action }); | ||
| } |
Contributor
There was a problem hiding this comment.
🟢 Low state/sourceControlActions.ts:153
useVcsInitAction calls useAction without an onSuccess callback, so the VCS status query is not refreshed after git init completes. This causes the UI to continue showing "Initialize Git" instead of the actual Git controls until the status is refreshed by other means.
return useAction({ kind: "init", scope, action });
+ const status = useEnvironmentQuery(
+ scope.environmentId !== null && scope.cwd !== null
+ ? vcsEnvironment.status({
+ environmentId: scope.environmentId,
+ input: { cwd: scope.cwd },
+ })
+ : null,
+ );
return useAction({ kind: "init", scope, action, onSuccess: status.refresh });
- return useAction({ kind: "init", scope, action });🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/state/sourceControlActions.ts around lines 153-163:
`useVcsInitAction` calls `useAction` without an `onSuccess` callback, so the VCS status query is not refreshed after `git init` completes. This causes the UI to continue showing "Initialize Git" instead of the actual Git controls until the status is refreshed by other means.
Evidence trail:
apps/web/src/state/sourceControlActions.ts lines 83-126 (useAction definition, line 107 calls onSuccess), lines 153-163 (useVcsInitAction - no onSuccess), lines 165-187 (useVcsPullAction - has onSuccess: status.refresh), lines 190-254 (useGitStackedAction - has onSuccess: status.refresh), lines 257-294 (useSourceControlPublishRepositoryAction - has onSuccess: status.refresh). apps/web/src/components/GitActionsControl.tsx lines 1069-1077 (gitStatusQuery), line 1085 (isRepo = gitStatus?.isRepo ?? true), line 1616 (!isRepo check), line 1622 (void initAction.run()). apps/web/src/state/query.ts lines 24-36 (useEnvironmentQuery - no auto-polling, requires explicit refresh).
| ); | ||
| } | ||
|
|
||
| export async function clearThreadOutboxEnvironment(environmentId: EnvironmentId): Promise<void> { |
Contributor
There was a problem hiding this comment.
🟡 Medium state/thread-outbox.ts:178
clearThreadOutboxEnvironment reads the current atom value at the start, then awaits async operations (loading persisted messages and deleting files). During these awaits, enqueueThreadOutboxMessage can add a new message for a different environment to the atom. The final appAtomRegistry.set overwrites the atom with allMessages (computed from the stale pre-await current), silently dropping the newly added message from in-memory state. The message remains on disk but disappears from the UI until restart.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/state/thread-outbox.ts around line 178:
`clearThreadOutboxEnvironment` reads the current atom value at the start, then awaits async operations (loading persisted messages and deleting files). During these awaits, `enqueueThreadOutboxMessage` can add a new message for a different environment to the atom. The final `appAtomRegistry.set` overwrites the atom with `allMessages` (computed from the stale pre-await `current`), silently dropping the newly added message from in-memory state. The message remains on disk but disappears from the UI until restart.
Evidence trail:
apps/mobile/src/state/thread-outbox.ts lines 178-206: `clearThreadOutboxEnvironment` reads atom at line 179, awaits at line 180, computes `allMessages` from stale `current` at line 184, awaits again at line 187, then overwrites atom at line 200.
apps/mobile/src/state/thread-outbox.ts lines 148-161: `enqueueThreadOutboxMessage` reads atom at line 156 and sets at line 157-160 — if this runs during the awaits of `clearThreadOutboxEnvironment`, its write will be overwritten by the stale value at line 200.
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
…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.
juliusmarminge
force-pushed
the
codex/hosted-web-tracing-poc
branch
from
f37abe6 to
fe78bce
Compare
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>
- 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.
- keep subscriptions alive across transport and supervisor replacement - bound probes and improve retry, authorization, and state projections - add deterministic regression coverage for connection lifecycle failures Co-authored-by: codex <codex@users.noreply.github.com>
- self-heal persisted connection catalogs and clean stale temporary data - preserve bearer authorization across edits and attachment requests - centralize mobile runtime reuse and background outbox draining Co-authored-by: codex <codex@users.noreply.github.com>
- project server snapshots and request latency into focused atoms - surface slow RPC requests without coupling shared runtime to React - reuse the web runtime and make hosted connection discovery resilient Co-authored-by: codex <codex@users.noreply.github.com>
- recover desktop connection catalogs from partial writes - remove stale temporary catalog files after recovery - trust incoming relay trace context only after authentication Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
|
|
||
| const INLINE_HTML_TAG_PATTERN = /<\/?(?:kbd|mark|sub|sup|u)(?:\s[^>]*)?>/gi; | ||
|
|
||
| function decodeHtmlEntitiesOnce(value: string): string { |
Contributor
There was a problem hiding this comment.
🟡 Medium lib/nativeMarkdownText.ts:64
decodeHtmlEntitiesOnce passes unvalidated numeric values to String.fromCodePoint, which throws RangeError for invalid code points (>0x10FFFF or surrogate range 0xD800-0xDFFF). Inputs like � or � in markdown content will crash the parser. Consider validating the code point and returning the original entity when invalid.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/lib/nativeMarkdownText.ts around line 64:
`decodeHtmlEntitiesOnce` passes unvalidated numeric values to `String.fromCodePoint`, which throws `RangeError` for invalid code points (>0x10FFFF or surrogate range 0xD800-0xDFFF). Inputs like `�` or `�` in markdown content will crash the parser. Consider validating the code point and returning the original entity when invalid.
Evidence trail:
apps/mobile/src/lib/nativeMarkdownText.ts lines 64-92 at REVIEWED_COMMIT: `decodeHtmlEntitiesOnce` regex matches unbounded numeric values; lines 69 and 72 pass them directly to `String.fromCodePoint()` with no range validation or try-catch. ECMAScript spec: String.fromCodePoint throws RangeError for code points > 0x10FFFF or in surrogate range 0xD800-0xDFFF.
Co-authored-by: codex <codex@users.noreply.github.com>
juliusmarminge
force-pushed
the
codex/connection-state-audit
branch
from
a71ceef to
25717b6
Compare
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
Replace the legacy web and mobile connection implementations with a shared Effect-based client runtime, then move the mobile thread experience onto the same current contracts and turn lifecycle behavior as web.
@t3tools/client-runtime@t3tools/client-runtimeinto explicit subpath exports and migrate web/mobile away from their independent stores and RPC clientsserviceTierinstead of the legacy hard-coded Fast Mode toggleStack
mainReview this PR against #2995. The tracing infrastructure and release configuration remain isolated in the base PR.
Validation
vp checkvp run typecheckvp run lint:mobile