[codex] Rewrite client connection architecture#2978
Open
juliusmarminge wants to merge 1 commit 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
Comment on lines
-59
to
+77
| ); | ||
| 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).
Comment on lines
+1808
to
+2190
| (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)
a71ceef to
25717b6
Compare
![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 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Stale tokens break selection mapping
- Extracted a currentValidTokens() helper that filters tokens against the current value string, and used it in displayOffset(forSourceOffset:) so stale tokens no longer participate in offset calculations.
Or push these changes by commenting:
@cursor push b5abc57d3c
Preview (b5abc57d3c)
diff --git a/apps/mobile/modules/t3-composer-editor/ios/T3ComposerEditorView.swift b/apps/mobile/modules/t3-composer-editor/ios/T3ComposerEditorView.swift
--- a/apps/mobile/modules/t3-composer-editor/ios/T3ComposerEditorView.swift
+++ b/apps/mobile/modules/t3-composer-editor/ios/T3ComposerEditorView.swift
@@ -464,12 +464,7 @@
let result = NSMutableAttributedString()
let source = value as NSString
var cursor = 0
- let validTokens = tokens.filter {
- $0.start >= cursor &&
- $0.end > $0.start &&
- $0.end <= source.length &&
- source.substring(with: NSRange(location: $0.start, length: $0.end - $0.start)) == $0.source
- }
+ let validTokens = currentValidTokens()
for token in validTokens {
if token.start < cursor {
@@ -678,11 +673,12 @@
private func displayOffset(forSourceOffset sourceOffset: Int) -> Int {
let boundedOffset = max(0, min((value as NSString).length, sourceOffset))
+ let active = currentValidTokens()
var collapsedLength = 0
- for token in tokens where token.end <= boundedOffset {
+ for token in active where token.end <= boundedOffset {
collapsedLength += max(0, token.end - token.start - 1)
}
- if let token = tokens.first(where: { $0.start < boundedOffset && boundedOffset < $0.end }) {
+ if let token = active.first(where: { $0.start < boundedOffset && boundedOffset < $0.end }) {
return token.start - collapsedLength + 1
}
return boundedOffset - collapsedLength
@@ -723,9 +719,9 @@
return try? JSONDecoder().decode(type, from: data)
}
- private func tokensMatchCurrentValue() -> Bool {
+ private func currentValidTokens() -> [ComposerTokenPayload] {
let source = value as NSString
- return tokens.allSatisfy {
+ return tokens.filter {
$0.start >= 0 &&
$0.end > $0.start &&
$0.end <= source.length &&
@@ -733,19 +729,12 @@
}
}
+ private func tokensMatchCurrentValue() -> Bool {
+ currentValidTokens().count == tokens.count
+ }
+
private func documentMatchesExpectedTokens() -> Bool {
- let source = value as NSString
- let expectedSources = tokens.compactMap { token -> String? in
- guard token.start >= 0,
- token.end > token.start,
- token.end <= source.length,
- source.substring(
- with: NSRange(location: token.start, length: token.end - token.start)
- ) == token.source else {
- return nil
- }
- return token.source
- }
+ let expectedSources = currentValidTokens().map(\.source)
var renderedSources: [String] = []
textView.attributedText.enumerateAttribute(
.attachment,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 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Recording lock survives tab close
- Added a check in closeTab to clear recordingTabIdRef when the closed tab matches the currently recording tab, preventing the stale lock from blocking future recordings.
Or push these changes by commenting:
@cursor push 49b0b52408
Preview (49b0b52408)
diff --git a/apps/desktop/src/preview/Manager.ts b/apps/desktop/src/preview/Manager.ts
--- a/apps/desktop/src/preview/Manager.ts
+++ b/apps/desktop/src/preview/Manager.ts
@@ -1130,6 +1130,10 @@
const tab = (yield* SynchronizedRef.get(tabsRef)).get(tabId);
if (!tab) return;
yield* cancelPickElement(tabId);
+ const recordingTabId = yield* Ref.get(recordingTabIdRef);
+ if (Option.isSome(recordingTabId) && recordingTabId.value === tabId) {
+ yield* Ref.set(recordingTabIdRef, Option.none());
+ }
if (tab.webContentsId != null) {
yield* Effect.all(
[detachControlSession(tab.webContentsId), detachListeners(tab.webContentsId)],You can send follow-ups to the cloud agent here.
Comment on lines
+323
to
+326
| const editor = resolveAndPersistPreferredEditor(serverConfig.availableEditors); | ||
| if (!editor) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🟢 Low routes/__root.tsx:323
When resolveAndPersistPreferredEditor returns null, the onClick handler returns silently at line 325 without showing any feedback to the user. Previously this threw new Error("No available editors found.") which was caught and displayed in an error toast. Now the button appears to do nothing when no editor is available.
if (!editor) {
+ toastManager.add(
+ stackedThreadToast({
+ type: "error",
+ title: "Unable to open keybindings file",
+ description: "No available editors found.",
+ }),
+ );
return;
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/routes/__root.tsx around lines 323-326:
When `resolveAndPersistPreferredEditor` returns `null`, the `onClick` handler returns silently at line 325 without showing any feedback to the user. Previously this threw `new Error("No available editors found.")` which was caught and displayed in an error toast. Now the button appears to do nothing when no editor is available.
Evidence trail:
apps/web/src/routes/__root.tsx lines 319-342 at REVIEWED_COMMIT (new code with silent return). git_diff base=MERGE_BASE head=REVIEWED_COMMIT path=apps/web/src/routes/__root.tsx shows old code had `throw new Error("No available editors found.")` inside a `.then()` caught by `.catch()` that displayed an error toast. Commit a01d712751f8 introduced `resolveAndPersistPreferredEditor` with the same throw pattern in the previous iteration.
| @@ -423,14 +438,25 @@ function UserTimelineRow({ row }: { row: Extract<TimelineRow, { kind: "message" | |||
| const userImages = row.message.attachments ?? []; | |||
| const displayedUserMessage = deriveDisplayedUserMessageState(row.message.text); | |||
Contributor
There was a problem hiding this comment.
🟠 High chat/MessagesTimeline.tsx:439
Line 449 calls extractTrailingElementContexts(visibleText) on text that has already had element contexts stripped by deriveDisplayedUserMessageState on line 439, so elementContextState.contexts is always empty. The element contexts are actually stored in displayedUserMessage.elementContexts, but the code ignores them and renders an empty chip list instead. Use displayedUserMessage.elementContexts instead of elementContextState.contexts on lines 497-505.
Also found in 1 other location(s)
apps/mobile/modules/t3-composer-editor/ios/T3ComposerEditorView.swift:679
The
displayOffset(forSourceOffset:)method calculates collapsed lengths using the rawtokensarray, butmakeAttributedDocument()filters tokens for validity (checkingsourcesubstring matches). If any token intokenshas an invalid/mismatchedsource,displayOffsetwill include its collapsed length in calculations while the actual attributed document won't contain that attachment. This causes incorrect cursor/selection positioning. For example, ifvalue="hello world"andtokenscontains a token withsource="wrong"(not matching the actual substring), the selection conversion will be off.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 439:
Line 449 calls `extractTrailingElementContexts(visibleText)` on text that has already had element contexts stripped by `deriveDisplayedUserMessageState` on line 439, so `elementContextState.contexts` is always empty. The element contexts are actually stored in `displayedUserMessage.elementContexts`, but the code ignores them and renders an empty chip list instead. Use `displayedUserMessage.elementContexts` instead of `elementContextState.contexts` on lines 497-505.
Evidence trail:
apps/web/src/components/chat/MessagesTimeline.tsx lines 436-510 (REVIEWED_COMMIT) — UserTimelineRow component showing the double extraction. apps/web/src/lib/terminalContext.ts lines 248-262 (REVIEWED_COMMIT) — deriveDisplayedUserMessageState calls extractTrailingElementContexts internally and stores results in elementContexts, returning visibleText with element contexts already stripped.
Also found in 1 other location(s):
- apps/mobile/modules/t3-composer-editor/ios/T3ComposerEditorView.swift:679 -- The `displayOffset(forSourceOffset:)` method calculates collapsed lengths using the raw `tokens` array, but `makeAttributedDocument()` filters tokens for validity (checking `source` substring matches). If any token in `tokens` has an invalid/mismatched `source`, `displayOffset` will include its collapsed length in calculations while the actual attributed document won't contain that attachment. This causes incorrect cursor/selection positioning. For example, if `value="hello world"` and `tokens` contains a token with `source="wrong"` (not matching the actual substring), the selection conversion will be off.
Comment on lines
+131
to
+165
| const disconnect = Effect.fn("PreviewAutomationBroker.disconnect")(function* ( | ||
| clientId: string, | ||
| queue: ClientConnection["queue"], | ||
| ) { | ||
| const toFail = yield* SynchronizedRef.modify(state, (current) => { | ||
| if (current.clients.get(clientId)?.queue !== queue) { | ||
| return [[] as ReadonlyArray<PendingRequest>, current] as const; | ||
| } | ||
| const clients = new Map(current.clients); | ||
| const owners = new Map(current.owners); | ||
| const pending = new Map(current.pending); | ||
| const disconnected: PendingRequest[] = []; | ||
| clients.delete(clientId); | ||
| owners.delete(clientId); | ||
| for (const [requestId, entry] of pending) { | ||
| if (entry.clientId === clientId) { | ||
| pending.delete(requestId); | ||
| disconnected.push(entry); | ||
| } | ||
| } | ||
| return [disconnected, { ...current, clients, owners, pending }] as const; | ||
| }); | ||
| yield* Effect.forEach( | ||
| toFail, | ||
| ({ deferred }) => | ||
| Deferred.fail( | ||
| deferred, | ||
| new PreviewAutomationUnavailableError({ | ||
| message: "The preview automation client disconnected.", | ||
| }), | ||
| ), | ||
| { discard: true }, | ||
| ); | ||
| yield* Queue.shutdown(queue); | ||
| }); |
Contributor
There was a problem hiding this comment.
🟠 High mcp/PreviewAutomationBroker.ts:131
When a client reconnects with the same clientId, disconnect is called with the old queue but checks against the new queue stored in state, causing the equality check at line 136 to fail. The function returns early without shutting down the old queue or failing its pending requests, leaving them hanging indefinitely and leaking the queue.
const disconnect = Effect.fn("PreviewAutomationBroker.disconnect")(function* (
clientId: string,
queue: ClientConnection["queue"],
) {
const toFail = yield* SynchronizedRef.modify(state, (current) => {
- if (current.clients.get(clientId)?.queue !== queue) {
+ const stored = current.clients.get(clientId);
+ // Only skip if this exact connection is still registered
+ if (stored?.queue === queue) {
return [[] as ReadonlyArray<PendingRequest>, current] as const;
}
const clients = new Map(current.clients);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/PreviewAutomationBroker.ts around lines 131-165:
When a client reconnects with the same `clientId`, `disconnect` is called with the old queue but checks against the new queue stored in state, causing the equality check at line 136 to fail. The function returns early without shutting down the old queue or failing its pending requests, leaving them hanging indefinitely and leaking the queue.
67ca323 to
d83dfa7
Compare
juliusmarminge
changed the base branch from
codex/hosted-web-tracing-poc
to
main
juliusmarminge
force-pushed
the
codex/connection-state-audit
branch
from
d83dfa7 to
7edd34a
Compare
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).
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Markdown links ignore taps
- Changed getTouchChild to iterate _view.subviews (the Fabric contentView where T3MarkdownTextRun children are mounted) instead of self.subviews, which only contained _textView and the empty content container.
- ✅ Fixed: Catalog reads empty without encryption
- Replaced the silent Option.none() return with a dedicated DesktopConnectionCatalogStoreEncryptionUnavailableError when an encrypted catalog file exists on disk but safe-storage encryption is unavailable, so the client correctly surfaces the error instead of treating saved connections as missing.
Or push these changes by commenting:
@cursor push 10af3a0dce
Preview (10af3a0dce)
diff --git a/apps/desktop/src/app/DesktopConnectionCatalogStore.ts b/apps/desktop/src/app/DesktopConnectionCatalogStore.ts
--- a/apps/desktop/src/app/DesktopConnectionCatalogStore.ts
+++ b/apps/desktop/src/app/DesktopConnectionCatalogStore.ts
@@ -88,11 +88,20 @@
}
}
+export class DesktopConnectionCatalogStoreEncryptionUnavailableError extends Data.TaggedError(
+ "DesktopConnectionCatalogStoreEncryptionUnavailableError",
+)<{}> {
+ override get message() {
+ return "Cannot read the encrypted connection catalog because encryption is unavailable.";
+ }
+}
+
export interface DesktopConnectionCatalogStoreShape {
readonly get: Effect.Effect<
Option.Option<string>,
| DesktopConnectionCatalogStoreReadError
| DesktopConnectionCatalogStoreDecodeError
+ | DesktopConnectionCatalogStoreEncryptionUnavailableError
| DesktopConnectionCatalogStoreMigrationError
| ElectronSafeStorage.ElectronSafeStorageAvailabilityError
| ElectronSafeStorage.ElectronSafeStorageDecryptError
@@ -300,7 +309,7 @@
return yield* migrateLegacyCatalog;
}
if (!(yield* safeStorage.isEncryptionAvailable)) {
- return Option.none<string>();
+ return yield* new DesktopConnectionCatalogStoreEncryptionUnavailableError();
}
const decrypted = yield* decodeSecretBytes(document.value.encryptedCatalog).pipe(
Effect.flatMap(safeStorage.decryptString),
diff --git a/apps/mobile/modules/t3-markdown-text/ios/T3MarkdownText.mm b/apps/mobile/modules/t3-markdown-text/ios/T3MarkdownText.mm
--- a/apps/mobile/modules/t3-markdown-text/ios/T3MarkdownText.mm
+++ b/apps/mobile/modules/t3-markdown-text/ios/T3MarkdownText.mm
@@ -616,7 +616,7 @@
];
int currIndex = -1;
- for (UIView* child in self.subviews) {
+ for (UIView* child in _view.subviews) {
if (![child isKindOfClass:[T3MarkdownTextRun class]]) {
continue;
}You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 7edd34a. Configure here.
| ignoreWhitespace: false, | ||
| }); | ||
|
|
||
| useEffect(() => { |
Contributor
There was a problem hiding this comment.
🟢 Low review/useReviewSections.ts:133
When the user switches from section A to section B, loadingTurnIds[A] remains true forever because the effect on lines 133-138 only updates the entry for the current activeSectionId. If the user returns to section A, the UI still shows it as loading even though no request is in flight. Consider clearing the previous section's loading state when activeSectionId changes, or using a lookup keyed by section id that reflects the actual pending query for that specific section.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/features/review/useReviewSections.ts around line 133:
When the user switches from section A to section B, `loadingTurnIds[A]` remains `true` forever because the effect on lines 133-138 only updates the entry for the current `activeSectionId`. If the user returns to section A, the UI still shows it as loading even though no request is in flight. Consider clearing the previous section's loading state when `activeSectionId` changes, or using a lookup keyed by section id that reflects the actual pending query for that specific section.
Evidence trail:
apps/mobile/src/features/review/useReviewSections.ts lines 133-138 (effect only writes current activeSectionId, no cleanup); apps/mobile/src/features/review/reviewState.ts lines 201-218 (setReviewTurnDiffLoading sets/deletes by sectionId); apps/mobile/src/features/review/reviewModel.ts line 544 (loadingTurnIds[id] === true used to set isLoading on UI items)
Comment on lines
+218
to
+248
| export async function clearThreadOutboxEnvironment(environmentId: EnvironmentId): Promise<void> { | ||
| await serializeThreadOutboxMutation(async () => { | ||
| const current = flattenQueues(appAtomRegistry.get(queuedMessagesByThreadKeyAtom)); | ||
| const persisted = await loadPersistedMessages().catch((error) => { | ||
| console.warn("[thread-outbox] failed to load messages while clearing environment", error); | ||
| return []; | ||
| }); | ||
| const allMessages = flattenQueues(groupQueuedThreadMessages([...persisted, ...current])); | ||
| const removed = allMessages.filter((message) => message.environmentId === environmentId); | ||
|
|
||
| await Promise.all( | ||
| removed.map(async (message) => { | ||
| try { | ||
| const file = await getMessageFile(message.messageId); | ||
| if (file.exists) { | ||
| file.delete(); | ||
| } | ||
| } catch (error) { | ||
| console.warn("[thread-outbox] failed to clear persisted message", error); | ||
| } | ||
| }), | ||
| ); | ||
|
|
||
| appAtomRegistry.set( | ||
| queuedMessagesByThreadKeyAtom, | ||
| groupQueuedThreadMessages( | ||
| allMessages.filter((message) => message.environmentId !== environmentId), | ||
| ), | ||
| ); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 Medium state/thread-outbox.ts:218
clearThreadOutboxEnvironment can race with the pending loadPromise from ensureThreadOutboxLoaded. If loading is in progress, this function deletes files and clears the atom, but then the pending disk read (which captured the old file contents before deletion) completes and adds those cleared messages back into the atom. This causes messages for the cleared environment to reappear in-memory after the function returns, until the next app restart.
Consider awaiting loadPromise before proceeding, or ensuring the load operation respects concurrent deletions.
export async function clearThreadOutboxEnvironment(environmentId: EnvironmentId): Promise<void> {
+ if (loadPromise !== null) {
+ await loadPromise;
+ }
await serializeThreadOutboxMutation(async () => {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/state/thread-outbox.ts around lines 218-248:
`clearThreadOutboxEnvironment` can race with the pending `loadPromise` from `ensureThreadOutboxLoaded`. If loading is in progress, this function deletes files and clears the atom, but then the pending disk read (which captured the old file contents before deletion) completes and adds those cleared messages back into the atom. This causes messages for the cleared environment to reappear in-memory after the function returns, until the next app restart.
Consider awaiting `loadPromise` before proceeding, or ensuring the load operation respects concurrent deletions.
Evidence trail:
apps/mobile/src/state/thread-outbox.ts lines 55-65 (mutationQueue and serializeThreadOutboxMutation), lines 164-182 (ensureThreadOutboxLoaded - loadPersistedMessages resolves first, THEN queues the mutation via .then), lines 218-248 (clearThreadOutboxEnvironment - immediately queues mutation). The race: load starts disk I/O → clear queues and runs mutation → disk I/O resolves → load queues mutation AFTER clear → load re-adds cleared messages from stale persistedMessages.
Comment on lines
+183
to
+186
| Effect.all( | ||
| [ | ||
| Effect.promise(() => clearThreadOutboxEnvironment(environmentId)), | ||
| Effect.promise(() => clearComposerDraftsEnvironment(environmentId)), |
Contributor
There was a problem hiding this comment.
🟢 Low connection/platform.ts:183
Effect.catch on line 190 will never execute its error handler because Effect.promise produces an effect with error type never. When clearThreadOutboxEnvironment or clearComposerDraftsEnvironment reject, the rejection becomes an untyped defect rather than a typed error, so the warning log never records the failure. Consider using Effect.tryPromise instead of Effect.promise to capture rejections as typed errors that Effect.catch can handle.
- Effect.promise(() => clearThreadOutboxEnvironment(environmentId)),
- Effect.promise(() => clearComposerDraftsEnvironment(environmentId)),
+ Effect.tryPromise({
+ try: () => clearThreadOutboxEnvironment(environmentId),
+ catch: (error) => error,
+ }),
+ Effect.tryPromise({
+ try: () => clearComposerDraftsEnvironment(environmentId),
+ catch: (error) => error,
+ }),🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/src/connection/platform.ts around lines 183-186:
`Effect.catch` on line 190 will never execute its error handler because `Effect.promise` produces an effect with error type `never`. When `clearThreadOutboxEnvironment` or `clearComposerDraftsEnvironment` reject, the rejection becomes an untyped defect rather than a typed error, so the warning log never records the failure. Consider using `Effect.tryPromise` instead of `Effect.promise` to capture rejections as typed errors that `Effect.catch` can handle.
Evidence trail:
apps/mobile/src/connection/platform.ts lines 183-196 (Effect.promise and Effect.catch usage); pnpm-workspace.yaml line 19 (effect version 4.0.0-beta.78); Effect v4 migration docs at https://github.com/Effect-TS/effect-smol/blob/main/migration/error-handling.md (Effect.catchAll → Effect.catch, only handles recoverable errors); Effect documentation at https://effect.website/docs/error-management/expected-errors/ ('Effect.catchAll only handles recoverable errors. It will not recover from unrecoverable defects.')
f4d8453 to
c8fcac4
Compare
juliusmarminge
changed the base branch from
main
to
codex/hosted-web-tracing-poc
| // Even if the uiTextView prop is set, we can still default to using | ||
| // normal selection (i.e. base RN text) if the text doesn't need to be | ||
| // selectable | ||
| if ((!props.selectable || !props.uiTextView) && !isAncestor) { |
Contributor
There was a problem hiding this comment.
🟢 Low src/MarkdownTextPrimitive.tsx:98
MarkdownTextPrimitiveInner checks !props.selectable on line 98, but selectable defaults to true via textDefaults. When a caller sets uiTextView={true} without explicitly passing selectable, the component incorrectly returns RNText instead of MarkdownTextPrimitiveChild because undefined is treated as falsy. Consider checking props.selectable === false to respect the intended default behavior.
- if ((!props.selectable || !props.uiTextView) && !isAncestor) {
+ if ((props.selectable === false || !props.uiTextView) && !isAncestor) {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/mobile/modules/t3-markdown-text/src/MarkdownTextPrimitive.tsx around line 98:
`MarkdownTextPrimitiveInner` checks `!props.selectable` on line 98, but `selectable` defaults to `true` via `textDefaults`. When a caller sets `uiTextView={true}` without explicitly passing `selectable`, the component incorrectly returns `RNText` instead of `MarkdownTextPrimitiveChild` because `undefined` is treated as falsy. Consider checking `props.selectable === false` to respect the intended default behavior.
c8fcac4 to
1584952
Compare
juliusmarminge
changed the base branch from
main
to
codex/mobile-native-composer-markdown
juliusmarminge
force-pushed
the
codex/connection-state-audit
branch
from
1584952 to
a5670a8
Compare
juliusmarminge
force-pushed
the
codex/connection-state-audit
branch
5 times, most recently
from
6a22a8d to
fc808e6
Compare
Move web, desktop, server, and mobile connection state onto the shared client runtime while preserving the separately reviewed native mobile UI stack. Co-authored-by: codex <codex@users.noreply.github.com>
juliusmarminge
force-pushed
the
codex/connection-state-audit
branch
from
fc808e6 to
2eaface
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 independent web and mobile connection implementations with a shared Effect-based client runtime.
@t3tools/client-runtimeScope
The native mobile composer, markdown renderer, Clerk sheet routing, turn-fold presentation, and Pierre icons were reviewed separately in #3101 and are now part of
main. This PR contains the client connection/runtime rearchitecture on top of that merged base.Validation
node scripts/release-smoke.tsvp checkvp run typecheckvp run lint:mobileNote
High Risk
Large cross-platform auth, credential, and persistence changes (encrypted catalogs, migration, account transitions, connection phase renames) with high blast radius if imports or session field migrations are missed.
Overview
This PR replaces per-app connection wiring with a shared
@t3tools/client-runtimeconnection layer (atoms, platform storage, supervision, relay/cloud hooks) and moves imports off the package root onto explicit subpaths (/connection,/relay,/rpc,/authorization, etc.).Desktop gains an encrypted connection catalog (
DesktopConnectionCatalogStore) with IPC read/write/clear, one-time migration from legacy saved environments/secrets, and stricter persistence behavior (malformed registry/catalog surfaces read errors instead of silently emptying). Electron safe-storage types move toElectronSafeStorageServicefor reuse in tests and the new store.Mobile adds a full connection platform: secure-store catalog with legacy
t3code.connectionsmigration, file-backed shell/thread snapshot caches,expo-network/ app-state wakeups, and SSH explicitly blocked. Screens pivot touseWorkspaceState, entity hooks, anduseThreadOutboxDrain; cloud environments useuseConnectionController, account-transition cleanup inCloudAuthProvider, coalesced APNs device registration, and richer status/trace UI. HTTP assets (e.g. favicons) use shared remote auth headers including DPoP.Saved environments on desktop still exist for migration but main composition prefers the catalog store layer; cloud auth fetch fixes header passing for Effect HTTP client.
CI drops two web browser test files from the bundled components job.
Reviewed by Cursor Bugbot for commit 2eaface. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Replace WebSocket/store connection layer with environment-scoped atom registry
connection/subsystem inpackages/client-runtimewithEnvironmentRegistry,ConnectionSupervisor,ConnectionDriver,ConnectionResolver, andConnectionOnboardingservices that manage environment lifecycle, backoff/retry, and credential resolution.state/shell,state/threads,state/terminal,state/vcs,state/preview,state/server, etc.) backed by a newconnectionAtomRuntimethat replaces the priorwebRuntime/mobileRuntimeexports.useEnvironmentQuery/useAtomSet/ atom-based actions;readLocalApiand directEnvironmentApiusage are removed from the browser layer.EnvironmentScopedProjectShell→EnvironmentProject,cwd→workspaceRoot, sessionorchestrationStatus→status, andclosed→stoppedthroughout.DesktopConnectionCatalogStoreand exposes get/set/clear via IPC; mobile adds aSecureCatalogStorage-backedCatalogStorewith legacy migration.state/thread-outbox) on mobile with deduplication, capped retry/backoff, and auseThreadOutboxDrainhook that delivers queued messages once the environment is connected.@t3tools/client-runtimeno longer has a root export; all consumers must import from explicit subpaths (e.g.@t3tools/client-runtime/state/shell). Session status values and several field names have changed, which may affect any code not updated in this PR.Macroscope summarized 2eaface.