feat(mobile): make archives one-swipe and recoverable - #5205
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 5 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
github-actions
Bot
added
size:XL
github-actions
Bot
added
size:XXL
There was a problem hiding this comment.
One finding on Effect service conventions in apps/mobile/src/persistence/mobile-storage.ts. Everything else in the changed Effect scope (service interface additions, make/layer shape, namespace imports, imperative boundary adapter) looks consistent with the conventions.
Posted via Macroscope — Effect Service Conventions
| Effect.catch((cause) => | ||
| Effect.logWarning("Ignored an invalid retained background thread.").pipe( | ||
| Effect.annotateLogs({ | ||
| storageKey: BACKGROUND_CONNECTION_RETAINED_THREAD_KEY, | ||
| cause: String(cause), | ||
| }), | ||
| Effect.andThen(secureStorage.removeItem(BACKGROUND_CONNECTION_RETAINED_THREAD_KEY)), | ||
| Effect.as(null), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
The recovery path serializes the decode failure into a parallel log payload (cause: String(cause)), which copies arbitrary defect text (and the malformed stored value it usually embeds) into logs instead of preserving the exact underlying value as a structured cause. The existing MobileStorageDecodeError already carries key + cause and is used for the same situation in parseJson above; logging it keeps the context bounded and the error chain intact.
| Effect.catch((cause) => | |
| Effect.logWarning("Ignored an invalid retained background thread.").pipe( | |
| Effect.annotateLogs({ | |
| storageKey: BACKGROUND_CONNECTION_RETAINED_THREAD_KEY, | |
| cause: String(cause), | |
| }), | |
| Effect.andThen(secureStorage.removeItem(BACKGROUND_CONNECTION_RETAINED_THREAD_KEY)), | |
| Effect.as(null), | |
| ), | |
| ), | |
| Effect.catch((cause) => | |
| Effect.logWarning( | |
| new MobileStorageDecodeError({ | |
| key: BACKGROUND_CONNECTION_RETAINED_THREAD_KEY, | |
| cause, | |
| }), | |
| ).pipe( | |
| Effect.andThen(secureStorage.removeItem(BACKGROUND_CONNECTION_RETAINED_THREAD_KEY)), | |
| Effect.as(null), | |
| ), | |
| ), |
Posted via Macroscope — Effect Service Conventions
| val wasEnabled = isEnabled(context) | ||
| // Commit before starting the service so a process death cannot start a | ||
| // sticky service whose boot-time preference still says it is disabled. | ||
| preferences(context).edit().putBoolean(ENABLED_KEY, enabled).commit() |
There was a problem hiding this comment.
🟡 Medium t3backgroundconnection/T3BackgroundConnectionState.kt:63
setEnabled ignores the boolean returned by SharedPreferences.Editor.commit(), so a failed synchronous write leaves the persisted preference unchanged while the method proceeds as though the toggle succeeded. When enabling, ensureStarted rereads the old disabled value and never starts the service; when disabling, the service keeps running because requestOrderlyStop rereads the old enabled value. The async API still resolves successfully, so the user's requested setting silently fails. Consider checking the return value of commit() and surfacing the failure to the caller instead of continuing.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/modules/t3-background-connection/android/src/main/java/expo/modules/t3backgroundconnection/T3BackgroundConnectionState.kt around line 63:
`setEnabled` ignores the boolean returned by `SharedPreferences.Editor.commit()`, so a failed synchronous write leaves the persisted preference unchanged while the method proceeds as though the toggle succeeded. When enabling, `ensureStarted` rereads the old disabled value and never starts the service; when disabling, the service keeps running because `requestOrderlyStop` rereads the old enabled value. The async API still resolves successfully, so the user's requested setting silently fails. Consider checking the return value of `commit()` and surfacing the failure to the caller instead of continuing.
| let preferencesRelease: (() => void) | null = null; | ||
| let threadsRelease: (() => void) | null = null; | ||
|
|
||
| const reduce = () => { |
There was a problem hiding this comment.
🟡 Medium agent-notifications/thread-notification-service.ts:43
When dependencies.present rejects for an event, that event remains marked as emitted in state.emittedEventIds and is never retried, yet its ID is still persisted via persistEventIds. The notification is permanently lost. The reduce function assigns state = reduction.state (which already records every event ID as emitted) before the async present calls run, so the .catch handler only logs the failure without removing the IDs from state. Consider updating state only after the events are successfully presented, or removing failed event IDs from state.emittedEventIds in the .catch handler so they can be retried on the next reduction.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/agent-notifications/thread-notification-service.ts around line 43:
When `dependencies.present` rejects for an event, that event remains marked as emitted in `state.emittedEventIds` and is never retried, yet its ID is still persisted via `persistEventIds`. The notification is permanently lost. The `reduce` function assigns `state = reduction.state` (which already records every event ID as emitted) before the async `present` calls run, so the `.catch` handler only logs the failure without removing the IDs from state. Consider updating `state` only after the events are successfully presented, or removing failed event IDs from `state.emittedEventIds` in the `.catch` handler so they can be retried on the next reduction.
| append(retainedThread); | ||
| } | ||
| for (const thread of threadShells) { | ||
| if (thread.session?.status === "starting" || thread.session?.status === "running") { |
There was a problem hiding this comment.
🟡 Medium background-connection/target-selection.ts:31
selectBackgroundConnectionThreadTargets only checks thread.session?.status for "starting" or "running", but a thread is also considered active when thread.latestTurn?.state === "running" with a null or "ready" session. A shell in that state is omitted from the target list, so background-root.ts releases (or never acquires) its detail lease while the thread still has an active turn. That thread stops receiving background state updates until the session status transitions to "starting"/"running" or the turn finishes. The "running" latest-turn condition should be included alongside the session status check.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/background-connection/target-selection.ts around line 31:
`selectBackgroundConnectionThreadTargets` only checks `thread.session?.status` for `"starting"` or `"running"`, but a thread is also considered active when `thread.latestTurn?.state === "running"` with a `null` or `"ready"` session. A shell in that state is omitted from the target list, so `background-root.ts` releases (or never acquires) its detail lease while the thread still has an active turn. That thread stops receiving background state updates until the session status transitions to `"starting"`/`"running"` or the turn finishes. The `"running"` latest-turn condition should be included alongside the session status check.
| }) | ||
| .catch((error) => { | ||
| console.error("[agent-notifications] failed to deliver Android notification", error); | ||
| }); |
There was a problem hiding this comment.
Notification persist skips on present failure
Medium Severity
The notification service marks an event as emitted in its in-memory state before successfully presenting it or persisting its ID. If presentation or persistence fails, the user won't see the notification, and it won't be re-attempted.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f05becb. Configure here.
|
|
||
| const connectDisabled = isSubmitting || hostInput.trim().length === 0; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
🟠 High connection/ConnectionsNewRouteScreen.tsx:48
When the route is opened with params.pairingUrl, the sync effect overrides any later user edits or QR scan results. Scanning a new QR code calls onChangeConnectionPairingUrl, which reruns the effect and restores hostInput/codeInput to the original route URL. Manually editing the host or code and submitting likewise triggers onChangeConnectionPairingUrl, rerunning the effect and resetting the form to the route-provided values. Users cannot replace or correct a route-provided pairing target. Consider syncing params.pairingUrl only on initial mount, or guarding the effect so it does not restore route values after the user has taken an action.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/mobile/src/features/connection/ConnectionsNewRouteScreen.tsx around line 48:
When the route is opened with `params.pairingUrl`, the sync effect overrides any later user edits or QR scan results. Scanning a new QR code calls `onChangeConnectionPairingUrl`, which reruns the effect and restores `hostInput`/`codeInput` to the original route URL. Manually editing the host or code and submitting likewise triggers `onChangeConnectionPairingUrl`, rerunning the effect and resetting the form to the route-provided values. Users cannot replace or correct a route-provided pairing target. Consider syncing `params.pairingUrl` only on initial mount, or guarding the effect so it does not restore route values after the user has taken an action.
| }, [connectionPairingUrl]); | ||
| if (routePairingUrl && routePairingUrl !== connectionPairingUrl) { | ||
| onChangeConnectionPairingUrl(routePairingUrl); | ||
| } |
There was a problem hiding this comment.
Route param overrides pairing atom
Medium Severity
The sync effect builds the pairing URL as params.pairingUrl || connectionPairingUrl, so a stale pairingUrl route param always wins over the global pairing atom. A QR scan or other atom update can be overwritten on the next effect run, resetting host and code to the route value instead of the scanned URL.
Reviewed by Cursor Bugbot for commit 8476ef8. Configure here.
| defaultEnvironmentHost, | ||
| onChangeConnectionPairingUrl, | ||
| params.pairingUrl, | ||
| ]); |
There was a problem hiding this comment.
Sync effect reapplies default host
Medium Severity
Whenever the sync effect runs with an empty parsed host, setHostInput(host || defaultEnvironmentHost) injects the preview default. That differs from the prior setHostInput(host) behavior and can replace a deliberately cleared host (or other local edits) whenever connectionPairingUrl or route params change while the screen stays mounted.
Reviewed by Cursor Bugbot for commit 8476ef8. Configure here.
| throw new Error(`Could not check preview updates (${response.status}).`); | ||
| } | ||
| const release = parsePersonalPreviewRelease(await response.json()); | ||
| return selectNewerPersonalPreviewUpdate(Constants.nativeBuildVersion, release); |
There was a problem hiding this comment.
Wrong latest release breaks updates
Medium Severity
Preview update checks call GitHub’s /releases/latest and then require a mark-mobile-preview-v tag. If the fork’s current “latest” release is anything else, parsing throws and the whole check fails instead of treating it as no update, so Settings and launch checks error until preview is latest again.
Reviewed by Cursor Bugbot for commit 8476ef8. Configure here.
| git -C source checkout --detach "$feature_sha" | ||
| git -C source rebase origin/main | ||
|
|
||
| source_key="${main_sha}:${feature_sha}" |
There was a problem hiding this comment.
🟡 Medium workflows/personal-android-preview.yml:91
source_key only includes main_sha and feature_sha, so changes to the preview automation (this workflow or prepare-personal-android-preview.mjs) are ignored by the unchanged-source check. When only the automation changes, scheduled runs keep skipping the build and continue serving an APK built by the old safeguards indefinitely — only a manual force_rebuild corrects it. Include the automation revision (e.g. $GITHUB_SHA) in source_key so automation changes trigger a new build.
- source_key="${main_sha}:${feature_sha}"
+ source_key="${main_sha}:${feature_sha}:${GITHUB_SHA}"🤖 Copy this AI Prompt to have your agent fix this:
In file @.github/workflows/personal-android-preview.yml around line 91:
`source_key` only includes `main_sha` and `feature_sha`, so changes to the preview automation (this workflow or `prepare-personal-android-preview.mjs`) are ignored by the unchanged-source check. When only the automation changes, scheduled runs keep skipping the build and continue serving an APK built by the old safeguards indefinitely — only a manual `force_rebuild` corrects it. Include the automation revision (e.g. `$GITHUB_SHA`) in `source_key` so automation changes trigger a new build.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4c95746. Configure here.
| const personalPreviewDefaultEnvironmentHost = ( | ||
| repoEnv.EXPO_PUBLIC_T3CODE_PERSONAL_PREVIEW_DEFAULT_ENVIRONMENT_HOST ?? | ||
| repoEnv.T3CODE_PERSONAL_PREVIEW_DEFAULT_ENVIRONMENT_HOST | ||
| )?.trim(); |
There was a problem hiding this comment.
Empty EXPO_PUBLIC blocks fallback
Medium Severity
The new personal preview configuration incorrectly treats empty or invalid EXPO_PUBLIC_* environment variables as valid. This prevents falling back to legacy T3CODE_* env vars or Constants.expoConfig?.extra for the update API URL and default connection host, disabling these features even when valid fallback configurations exist.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4c95746. Configure here.
2 participants
What Changed
Mobile archiving required a partial swipe plus an action tap, while recovery lived under Settings. This PR makes the archive lifecycle reversible from the thread list:
Archived (N)shelf sits at the bottom of the list, hidden when empty and collapsed by default.The collapsed, empty-hidden shelf provides point-of-use optionality without adding a persistent setting or permanent list clutter.
Closes #5204.
Why
Archiving should be one deliberate gesture, and mistakes should be recoverable without leaving the thread list. Keeping the complete lifecycle together makes Archive safe enough to be fast.
UI Changes
Watch the 33-second Android archive/restore interaction.
Verification
vp test run scripts/mobile-showcase.test.ts apps/mobile/src/features/threads/threadListV2.test.ts apps/mobile/src/features/archive/archivedThreadList.test.ts apps/mobile/src/features/home/homeThreadList.test.ts— 76 tests passedvp run --filter @t3tools/mobile typecheckvp fmt --checkvp lint --report-unused-disable-directiveson every changed TypeScript fileChecklist
Built with Codex (GPT-5.6) in T3 Code.
Note
High Risk
Large mobile surface area: foreground services, battery/notification permissions, relay auth across UI and background, and CI signing secrets for preview APKs—any regression affects connectivity, notifications, or update delivery.
Overview
Beyond the archive UX work, this PR adds a large Android background connection stack and personal preview delivery path that are unrelated to quick archive alone.
Archive lifecycle on the thread list — Active rows can full-swipe to archive; archived rows support full-swipe Restore, partial swipe, and long-press Restore/Delete. A collapsible Archived (N) shelf at the bottom of home/sidebar lists filtered archived threads (up to 10 inline) with project scoping in archive list building.
Android keep-connected — New Expo module
t3-background-connectionruns aremoteMessagingforeground service plus Headless JS task that holds connection supervisors, outbox drain, relay auth, and thread detail leases without a second sync protocol. Settings expose Keep connected in background and Agent notifications; foreground return can skip aggressive reconnect when the native runtime reports healthy protection.Agent notifications — A reducer watches thread shells for completion, failure, approval, and input edges; a service posts high-priority
expo-notificationswhen the app is backgrounded and the preference is on.Cloud relay ownership — Managed relay sessions split UI vs background owners so Clerk UI unmount does not tear down relay used by the headless task; background auth bootstraps Clerk with retry and invalidation on sign-out.
Personal Android preview — GitHub Actions rebases a pinned personal feature branch onto upstream, signs
com.t3tools.t3code.preview, disables official Expo OTA, and publishes immutable GitHub releases; preview builds read update API and default environment host from env/extra.Reviewed by Cursor Bugbot for commit 4c95746. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add Android background connection service and make thread archiving the primary one-swipe action
T3BackgroundConnection) that runs a headless JS task to maintain relay auth, mount thread atoms, drain the outbox, and send agent notifications while the app is backgrounded; the service survives device reboot and app replacement via aBroadcastReceiver.BackgroundConnectionSettingsSection) for enabling/disabling the background connection, prompting for battery optimization exemption, and managing Android notification permissions.managedRelaySessionOwnershiplayer that coordinates UI and background relay session ownership, preventing stale background auth from overwriting an active UI session.Macroscope summarized 4c95746.
Built with Codex (GPT-5.6) in T3 Code.