From bf13cdf74fb61e4736eed2043cc5487e86e9ce64 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 16:00:51 -0700 Subject: [PATCH 01/17] fix(mobile): recover from render errors in-place with scoped boundaries A render error anywhere in the mobile app previously reached the global fatal handler, and expo-updates' ErrorRecovery killed the process. Wrap every root navigation route (navigator screenLayout) and the thread feed in their own error boundary: the failed subtree is replaced by a recovery view (retry, copy diagnostics, go back) while the native header, navigation, and the rest of the stack stay alive. Caught errors are recorded in a session-scoped render-error log (deduped by error identity so nested boundaries report once) and surfaced on the Settings > Diagnostics screen next to the existing expo-updates startup-crash log, which keeps covering only process fatals. --- apps/mobile/src/Stack.tsx | 32 +++- .../src/components/RenderErrorBoundary.tsx | 144 ++++++++++++++++++ .../SettingsDiagnosticsRouteScreen.tsx | 53 +++++++ .../diagnostics/render-error-log.test.ts | 90 +++++++++++ .../features/diagnostics/render-error-log.ts | 87 +++++++++++ .../features/threads/ThreadDetailScreen.tsx | 77 ++++++---- 6 files changed, 449 insertions(+), 34 deletions(-) create mode 100644 apps/mobile/src/components/RenderErrorBoundary.tsx create mode 100644 apps/mobile/src/features/diagnostics/render-error-log.test.ts create mode 100644 apps/mobile/src/features/diagnostics/render-error-log.ts diff --git a/apps/mobile/src/Stack.tsx b/apps/mobile/src/Stack.tsx index 3de335cfd562..4d20a122f871 100644 --- a/apps/mobile/src/Stack.tsx +++ b/apps/mobile/src/Stack.tsx @@ -10,7 +10,7 @@ import { createNativeStackScreen, type NativeStackNavigationOptions, } from "@react-navigation/native-stack"; -import { useEffect, useRef } from "react"; +import { useEffect, useRef, type ReactNode } from "react"; import { Platform, Pressable, @@ -23,6 +23,7 @@ import { useResolveClassNames } from "uniwind"; import { AppText as Text } from "./components/AppText"; import { getCompactBrandHeaderOptions } from "./components/CompactBrandTitle"; +import { RenderErrorBoundary, RenderFailureView } from "./components/RenderErrorBoundary"; import { ArchivedThreadsRouteScreen } from "./features/archive/ArchivedThreadsRouteScreen"; import { useAgentNotificationNavigation } from "./features/agent-awareness/notificationNavigation"; import { ConnectOnboardingRouteScreen } from "./features/cloud/ConnectOnboardingRouteScreen"; @@ -777,6 +778,34 @@ const RootStackConfig = createNativeStackNavigator({ }, }); +// NAVIGATION SEAM: every root route renders inside its own error boundary. +// A crashing screen shows the recovery UI in place — the native header, back +// gesture, and the rest of the stack stay alive, so recovery works without +// killing the app. Each route renders this layout within its own screen slot +// (keyed by the navigator), so a popped route tears its boundary down. Screens +// nested inside a sheet/stack route share that route's boundary. +function GuardedScreenLayout(props: { + readonly children: ReactNode; + readonly route: { readonly name: string }; +}) { + return ( + + {props.children} + + ); +} + +function ScreenRenderFallback(props: { readonly error: unknown; readonly retry: () => void }) { + const navigation = useNavigation(); + return ( + navigation.goBack() : undefined} + /> + ); +} + export const RootStack = RootStackConfig.with(function AdaptiveRootStack({ Navigator }) { const { width, height } = useWindowDimensions(); const usesWorkspaceFlowScreens = @@ -784,6 +813,7 @@ export const RootStack = RootStackConfig.with(function AdaptiveRootStack({ Navig return ( { if (route.name !== "SettingsSheet" && route.name !== "NewTaskSheet") { return {}; diff --git a/apps/mobile/src/components/RenderErrorBoundary.tsx b/apps/mobile/src/components/RenderErrorBoundary.tsx new file mode 100644 index 000000000000..2818f87e9cba --- /dev/null +++ b/apps/mobile/src/components/RenderErrorBoundary.tsx @@ -0,0 +1,144 @@ +import { Component, type ComponentType, type ReactNode } from "react"; +import { View } from "react-native"; + +import { SymbolView } from "./AppSymbol"; +import { AppText as Text } from "./AppText"; +import { MaterialButton } from "./MaterialButton"; +import { tryCopyTextWithHaptic } from "../lib/copyTextWithHaptic"; +import { recordRenderError } from "../features/diagnostics/render-error-log"; + +interface RenderErrorBoundaryProps { + readonly children: ReactNode; + /** Where the error was caught, recorded into the diagnostics render-error log. */ + readonly scope: string; + /** Changed inputs reset a failed subtree without remounting the boundary. */ + readonly resetKeys?: ReadonlyArray | undefined; + /** Subject noun for the default fallback's headline, e.g. "The conversation". */ + readonly subject?: string; + /** + * Recovery UI override, rendered as its own component so it can use hooks + * (e.g. navigation) even though the boundary itself is a class. + */ + readonly fallback?: ComponentType | undefined; +} + +export interface RenderFallbackProps { + readonly error: unknown; + readonly retry: () => void; +} + +interface RenderErrorBoundaryState { + readonly failedWith: unknown | undefined; + readonly resetKeys?: ReadonlyArray | undefined; +} + +/** + * Catches render errors in one subtree, records them for diagnostics, and + * shows an in-session recovery UI instead of letting the app die. Retrying + * unmounts the failed subtree and mounts a fresh one; changed `resetKeys` + * (e.g. a thread switch) clear the failure on their own. + * + * A caught error never reaches the global fatal handler, so expo-updates' + * ErrorRecovery startup log stays exclusively for process-ending fatals — + * `recordRenderError` is the sole report path here, deduped by error identity + * so an inner and outer boundary catching the same throw record it once. + */ +export class RenderErrorBoundary extends Component< + RenderErrorBoundaryProps, + RenderErrorBoundaryState +> { + override state = { failedWith: undefined, resetKeys: this.props.resetKeys }; + + // A changed thread/environment underneath a persistent boundary is new input: + // retry without waiting for the user to press Try again. + static getDerivedStateFromProps( + { resetKeys }: RenderErrorBoundaryProps, + state: RenderErrorBoundaryState, + ) { + if ( + resetKeys?.length !== state.resetKeys?.length || + resetKeys?.some((key, index) => !Object.is(key, state.resetKeys?.[index])) + ) { + return { failedWith: undefined, resetKeys }; + } + return null; + } + + static getDerivedStateFromError(error: unknown) { + return { failedWith: error }; + } + + override componentDidCatch(error: unknown, info: { componentStack?: string }) { + recordRenderError(error, this.props.scope, { componentStack: info.componentStack }); + } + + private readonly retry = () => { + this.setState({ failedWith: undefined }); + }; + + override render() { + const { failedWith } = this.state; + if (failedWith !== undefined) { + if (this.props.fallback) { + const Fallback = this.props.fallback; + return ; + } + return ( + + ); + } + return this.props.children; + } +} + +/** + * The recovery UI itself: retry, copy diagnostics, and (where navigation gives + * a way out) go back. It renders without a connection — recovery must work the + * same locally and over a tunnel, where the crash itself may have arrived with + * remote data. + */ +export function RenderFailureView(props: { + readonly subject?: string; + readonly error: unknown; + readonly retry: () => void; + readonly onGoBack?: (() => void) | undefined; +}) { + const message = + props.error instanceof Error ? props.error.message || props.error.name : String(props.error); + const copy = async () => { + const detail = props.error instanceof Error ? (props.error.stack ?? message) : message; + await tryCopyTextWithHaptic(detail, { target: "render error details" }); + }; + return ( + + + + + {props.subject ?? "This screen"} couldn’t be displayed + + + Try again to re-render it. If it keeps happening, copy the details — they help us fix it. + + + {message.slice(0, 300)} + + + + + void copy()} fullWidth /> + {props.onGoBack ? ( + + ) : null} + + + ); +} diff --git a/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx b/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx index 3d4a79d85910..3868ed33fda6 100644 --- a/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx +++ b/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx @@ -16,6 +16,11 @@ import { parseStartupCrashRecords, type StartupCrashRecord, } from "./crash-log-model"; +import { + formatRenderErrorReport, + getRenderErrorRecords, + type RenderErrorRecord, +} from "./render-error-log"; // expo-updates keeps its persistent log this long. Reading any further back // returns nothing, so this is the whole available window. @@ -47,6 +52,10 @@ export function SettingsDiagnosticsRouteScreen() { Updates.isEnabled ? { status: "loading" } : { status: "unavailable" }, ); const [copied, setCopied] = useState(false); + const [renderCopied, setRenderCopied] = useState(false); + // Render errors live only for the session (in memory, no persistence), and + // this screen mounts fresh each time it opens. + const [renderRecords] = useState>(getRenderErrorRecords); useEffect(() => { if (!Updates.isEnabled) return; @@ -72,6 +81,12 @@ export function SettingsDiagnosticsRouteScreen() { }); if (ok) setCopied(true); }; + const copyRenderReport = async () => { + const ok = await tryCopyTextWithHaptic(formatRenderErrorReport(renderRecords, appIdentity()), { + target: "render error report", + }); + if (ok) setRenderCopied(true); + }; return ( @@ -107,6 +122,24 @@ export function SettingsDiagnosticsRouteScreen() { )} + + {renderRecords.length === 0 ? ( + + ) : ( + renderRecords.map((record, index) => ( + + )) + )} + + void copyReport()} /> + void copyRenderReport()} + /> Paste the report into a GitHub issue. It contains the app version, the JavaScript error @@ -147,6 +186,20 @@ function EmptyState(props: { ); } +function RenderErrorRow(props: { readonly record: RenderErrorRecord; readonly first: boolean }) { + const { record } = props; + return ( + + + {new Date(record.timestamp).toLocaleString()} · {record.scope} + + + {record.message} + + + ); +} + function CrashRow(props: { readonly record: StartupCrashRecord; readonly first: boolean }) { const { record } = props; return ( diff --git a/apps/mobile/src/features/diagnostics/render-error-log.test.ts b/apps/mobile/src/features/diagnostics/render-error-log.test.ts new file mode 100644 index 000000000000..4777d2ec70ee --- /dev/null +++ b/apps/mobile/src/features/diagnostics/render-error-log.test.ts @@ -0,0 +1,90 @@ +import { beforeEach, describe, expect, it } from "vite-plus/test"; + +import { + clearRenderErrorRecords, + describeRenderError, + formatRenderErrorReport, + getRenderErrorRecords, + recordRenderError, +} from "./render-error-log"; + +beforeEach(() => { + clearRenderErrorRecords(); +}); + +describe("recordRenderError", () => { + it("records the message, stack, scope, and timestamp, newest first", () => { + const first = new Error("first broke"); + const second = new Error("second broke"); + expect(recordRenderError(first, "thread-feed", { timestamp: 100 })).toBe(true); + expect(recordRenderError(second, "screen:Thread", { timestamp: 200 })).toBe(true); + + const records = getRenderErrorRecords(); + expect(records.map((record) => record.message)).toEqual(["second broke", "first broke"]); + expect(records[0]?.scope).toBe("screen:Thread"); + expect(records[1]?.detail).toContain("first broke"); + expect(records[1]?.detail).toContain("Error: first broke"); + }); + + it("records one throw once when several boundaries on the path catch it", () => { + const error = new Error("bubble through everything"); + expect(recordRenderError(error, "thread-feed")).toBe(true); + // The same object keeps bubbling; the outer screen boundary must not add + // a second report of the identical crash. + expect(recordRenderError(error, "screen:Thread")).toBe(false); + expect(getRenderErrorRecords()).toHaveLength(1); + + // A fresh throw with the same message (e.g. after a failed retry) is a + // distinct crash and is recorded. + expect(recordRenderError(new Error("bubble through everything"), "thread-feed")).toBe(true); + expect(getRenderErrorRecords()).toHaveLength(2); + }); + + it("keeps the newest records when the log overflows", () => { + for (let index = 0; index < 25; index += 1) { + recordRenderError(new Error(`error ${index}`), "screen:Home", { timestamp: index }); + } + const records = getRenderErrorRecords(); + expect(records).toHaveLength(20); + expect(records[0]?.message).toBe("error 24"); + expect(records[19]?.message).toBe("error 5"); + }); + + it("appends the component stack when the runtime provides one", () => { + recordRenderError(new Error("bad render"), "thread-feed", { + componentStack: "\n in ThreadFeed\n in View", + }); + expect(getRenderErrorRecords()[0]?.detail).toContain("Component stack:\n in ThreadFeed"); + }); +}); + +describe("describeRenderError", () => { + it("falls back to the error name when the message is empty", () => { + expect(describeRenderError(new TypeError(""))).toBe("TypeError"); + }); + + it("stringifies non-error throws", () => { + expect(describeRenderError("boom")).toBe("boom"); + const thrown = { toString: () => "custom" }; + expect(describeRenderError(thrown)).toBe("custom"); + }); +}); + +describe("formatRenderErrorReport", () => { + it("labels an empty session without pretending a crash happened", () => { + const report = formatRenderErrorReport([], { version: "1.2.3", build: "45" }); + expect(report).toContain("T3 Code 1.2.3 (45)"); + expect(report).toContain("No recovered render errors this session."); + }); + + it("renders one section per record with scope and detail", () => { + recordRenderError(new Error("feed exploded"), "thread-feed", { timestamp: 1789277752000 }); + const report = formatRenderErrorReport(getRenderErrorRecords(), { + version: "1.2.3", + build: "45", + }); + expect(report).toContain("recovered render errors"); + expect(report).toContain("2026-09-13T05:35:52.000Z [thread-feed]"); + expect(report).toContain("feed exploded"); + }); +}); diff --git a/apps/mobile/src/features/diagnostics/render-error-log.ts b/apps/mobile/src/features/diagnostics/render-error-log.ts new file mode 100644 index 000000000000..1fbfb9c8e045 --- /dev/null +++ b/apps/mobile/src/features/diagnostics/render-error-log.ts @@ -0,0 +1,87 @@ +/** + * In-memory log of render errors the app caught and recovered from during the + * current session. + * + * This deliberately does NOT feed the expo-updates crash log the Diagnostics + * screen reads: `crash-log-model.ts` captures only ErrorRecovery fatals that + * took the process down at startup. A caught render error never reaches the + * global fatal handler, so it could never appear there — recording it here is + * the only way to surface it, and routing it anywhere else would double-report + * what the startup log already owns. + */ +export interface RenderErrorRecord { + readonly timestamp: number; + /** Where it was caught, e.g. `screen:Thread` or `thread-feed`. */ + readonly scope: string; + readonly message: string; + /** Message + stack (+ component stack when the runtime provides one). */ + readonly detail: string; +} + +const MAX_RECORDS = 20; +const recordedErrors = new WeakSet(); +let records: RenderErrorRecord[] = []; + +export function describeRenderError(error: unknown): string { + if (error instanceof Error) { + return error.message.trim().length > 0 ? error.message : error.name; + } + return String(error); +} + +/** + * Record a caught render error. Returns false when this exact error object was + * already recorded (a boundary further up the tree saw the same throw), which + * is how ancestors stay silent while still resetting their subtree. + */ +export function recordRenderError( + error: unknown, + scope: string, + options: { readonly componentStack?: string | undefined; readonly timestamp?: number } = {}, +): boolean { + const message = describeRenderError(error); + if (typeof error === "object" && error !== null) { + if (recordedErrors.has(error)) return false; + recordedErrors.add(error); + } + const stack = error instanceof Error ? error.stack : undefined; + const detail = [ + message, + stack !== undefined && stack !== null ? stack : "", + options.componentStack !== undefined && options.componentStack !== null + ? `Component stack:${options.componentStack}` + : "", + ] + .filter((part) => part.length > 0) + .join("\n"); + records = [ + { timestamp: options.timestamp ?? Date.now(), scope, message, detail }, + ...records, + ].slice(0, MAX_RECORDS); + return true; +} + +/** Newest first, as stored. */ +export function getRenderErrorRecords(): ReadonlyArray { + return records; +} + +export function clearRenderErrorRecords(): void { + records = []; +} + +/** The report a user pastes into an issue, mirroring the startup crash report. */ +export function formatRenderErrorReport( + input: ReadonlyArray, + app: { readonly version: string; readonly build: string }, +): string { + const header = `T3 Code ${app.version} (${app.build}) — recovered render errors`; + if (input.length === 0) return `${header}\nNo recovered render errors this session.`; + return [ + header, + ...input.map( + (record) => + `\n--- ${new Date(record.timestamp).toISOString()} [${record.scope}] ---\n${record.detail}`, + ), + ].join("\n"); +} diff --git a/apps/mobile/src/features/threads/ThreadDetailScreen.tsx b/apps/mobile/src/features/threads/ThreadDetailScreen.tsx index ad0872ea9116..c2e880fe697d 100644 --- a/apps/mobile/src/features/threads/ThreadDetailScreen.tsx +++ b/apps/mobile/src/features/threads/ThreadDetailScreen.tsx @@ -72,6 +72,7 @@ import type { StatusTone } from "../../components/StatusPill"; import type { DraftComposerAttachment } from "../../lib/composerImages"; import { CHAT_CONTENT_MAX_WIDTH, type LayoutVariant } from "../../lib/layout"; import { IOS_NAV_BAR_HEIGHT } from "../../lib/layoutMetrics"; +import { RenderErrorBoundary } from "../../components/RenderErrorBoundary"; import { editPendingThreadMessage } from "../../state/edit-pending-thread-message"; import { deviceEnvironment } from "../../state/device"; import { useEnvironmentQuery } from "../../state/query"; @@ -892,39 +893,49 @@ export const ThreadDetailScreen = memo(function ThreadDetailScreen(props: Thread : "absolute inset-0 bg-screen" } /> - + {/* A crash while rendering feed entries is scoped here: the composer, + header, and navigation survive, and switching threads (new + resetKeys) clears the failure without any user action. */} + + + ) : ( From d330f4f1d826881f2c761e485337977f6aea9ea2 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 16:12:41 -0700 Subject: [PATCH 02/17] fix(mobile): track render failure by flag and exit cold-launch crashes Two audit findings on the boundary: - The thrown value doubled as the failure sentinel, so `throw undefined`/`null`/`""` rendered no fallback. Failure now lives in a dedicated flag in a pure, testable state model; falsy throws fail the boundary. - A cold-launch crash on the only route had no previous route to pop to and Settings lived inside the failed subtree. The screen fallback now offers Open settings (outside the boundary, leading to the Diagnostics tab) when navigation cannot go back. --- apps/mobile/src/Stack.tsx | 16 ++++- .../src/components/RenderErrorBoundary.tsx | 49 +++++++++------ .../render-error-boundary-model.test.ts | 61 +++++++++++++++++++ .../components/render-error-boundary-model.ts | 54 ++++++++++++++++ 4 files changed, 161 insertions(+), 19 deletions(-) create mode 100644 apps/mobile/src/components/render-error-boundary-model.test.ts create mode 100644 apps/mobile/src/components/render-error-boundary-model.ts diff --git a/apps/mobile/src/Stack.tsx b/apps/mobile/src/Stack.tsx index 4d20a122f871..ecfeed76fb77 100644 --- a/apps/mobile/src/Stack.tsx +++ b/apps/mobile/src/Stack.tsx @@ -24,6 +24,7 @@ import { useResolveClassNames } from "uniwind"; import { AppText as Text } from "./components/AppText"; import { getCompactBrandHeaderOptions } from "./components/CompactBrandTitle"; import { RenderErrorBoundary, RenderFailureView } from "./components/RenderErrorBoundary"; +import { screenFallbackExit } from "./components/render-error-boundary-model"; import { ArchivedThreadsRouteScreen } from "./features/archive/ArchivedThreadsRouteScreen"; import { useAgentNotificationNavigation } from "./features/agent-awareness/notificationNavigation"; import { ConnectOnboardingRouteScreen } from "./features/cloud/ConnectOnboardingRouteScreen"; @@ -797,11 +798,24 @@ function GuardedScreenLayout(props: { function ScreenRenderFallback(props: { readonly error: unknown; readonly retry: () => void }) { const navigation = useNavigation(); + // The navigation container is outside the failed subtree, so navigating out + // still works even when the screen content cannot render. On a cold-launch + // crash there is no previous route to go back to; Settings (and its + // Diagnostics tab, where the caught errors are listed) is always reachable. + if (screenFallbackExit(navigation.canGoBack()) === "open-settings") { + return ( + navigation.navigate("SettingsSheet")} + /> + ); + } return ( navigation.goBack() : undefined} + onGoBack={() => navigation.goBack()} /> ); } diff --git a/apps/mobile/src/components/RenderErrorBoundary.tsx b/apps/mobile/src/components/RenderErrorBoundary.tsx index 2818f87e9cba..ee079d3e419d 100644 --- a/apps/mobile/src/components/RenderErrorBoundary.tsx +++ b/apps/mobile/src/components/RenderErrorBoundary.tsx @@ -6,6 +6,12 @@ import { AppText as Text } from "./AppText"; import { MaterialButton } from "./MaterialButton"; import { tryCopyTextWithHaptic } from "../lib/copyTextWithHaptic"; import { recordRenderError } from "../features/diagnostics/render-error-log"; +import { + boundaryResetFromProps, + failedBoundaryState, + healthyBoundaryState, + type BoundaryState, +} from "./render-error-boundary-model"; interface RenderErrorBoundaryProps { readonly children: ReactNode; @@ -27,10 +33,7 @@ export interface RenderFallbackProps { readonly retry: () => void; } -interface RenderErrorBoundaryState { - readonly failedWith: unknown | undefined; - readonly resetKeys?: ReadonlyArray | undefined; -} +type RenderErrorBoundaryState = BoundaryState; /** * Catches render errors in one subtree, records them for diagnostics, and @@ -38,6 +41,9 @@ interface RenderErrorBoundaryState { * unmounts the failed subtree and mounts a fresh one; changed `resetKeys` * (e.g. a thread switch) clear the failure on their own. * + * Failure is tracked by a dedicated flag, not the thrown value, so + * `throw undefined`/`null`/`""` still render the fallback. + * * A caught error never reaches the global fatal handler, so expo-updates' * ErrorRecovery startup log stays exclusively for process-ending fatals — * `recordRenderError` is the sole report path here, deduped by error identity @@ -47,7 +53,7 @@ export class RenderErrorBoundary extends Component< RenderErrorBoundaryProps, RenderErrorBoundaryState > { - override state = { failedWith: undefined, resetKeys: this.props.resetKeys }; + override state = healthyBoundaryState(this.props.resetKeys); // A changed thread/environment underneath a persistent boundary is new input: // retry without waiting for the user to press Try again. @@ -55,17 +61,11 @@ export class RenderErrorBoundary extends Component< { resetKeys }: RenderErrorBoundaryProps, state: RenderErrorBoundaryState, ) { - if ( - resetKeys?.length !== state.resetKeys?.length || - resetKeys?.some((key, index) => !Object.is(key, state.resetKeys?.[index])) - ) { - return { failedWith: undefined, resetKeys }; - } - return null; + return boundaryResetFromProps(resetKeys, state); } static getDerivedStateFromError(error: unknown) { - return { failedWith: error }; + return failedBoundaryState(error); } override componentDidCatch(error: unknown, info: { componentStack?: string }) { @@ -73,18 +73,21 @@ export class RenderErrorBoundary extends Component< } private readonly retry = () => { - this.setState({ failedWith: undefined }); + this.setState(healthyBoundaryState(this.state.resetKeys)); }; override render() { - const { failedWith } = this.state; - if (failedWith !== undefined) { + if (this.state.failed) { if (this.props.fallback) { const Fallback = this.props.fallback; - return ; + return ; } return ( - + ); } return this.props.children; @@ -102,6 +105,8 @@ export function RenderFailureView(props: { readonly error: unknown; readonly retry: () => void; readonly onGoBack?: (() => void) | undefined; + /** Escape exit for a cold-launch crash where there is no route to go back to. */ + readonly onOpenSettings?: (() => void) | undefined; }) { const message = props.error instanceof Error ? props.error.message || props.error.name : String(props.error); @@ -138,6 +143,14 @@ export function RenderFailureView(props: { {props.onGoBack ? ( ) : null} + {props.onOpenSettings ? ( + + ) : null} ); diff --git a/apps/mobile/src/components/render-error-boundary-model.test.ts b/apps/mobile/src/components/render-error-boundary-model.test.ts new file mode 100644 index 000000000000..45282a7f1914 --- /dev/null +++ b/apps/mobile/src/components/render-error-boundary-model.test.ts @@ -0,0 +1,61 @@ +import { describe, expect, it } from "vite-plus/test"; + +import { + boundaryResetFromProps, + failedBoundaryState, + healthyBoundaryState, + screenFallbackExit, +} from "./render-error-boundary-model"; + +describe("failedBoundaryState", () => { + it("marks a failure for falsy throws, which the thrown value alone could not signal", () => { + for (const thrown of [undefined, null, "", 0, false]) { + const state = failedBoundaryState(thrown); + expect(state.failed).toBe(true); + expect(state.error).toBe(thrown); + } + }); + + it("omits resetKeys so the setState merge keeps the tracked keys", () => { + // Carrying an explicit `resetKeys: undefined` through would look like + // changed props on the next render and instantly auto-retry a crash loop. + expect("resetKeys" in failedBoundaryState(new Error("boom"))).toBe(false); + }); +}); + +describe("boundaryResetFromProps", () => { + it("clears a failure only when the tracked inputs actually changed", () => { + const failed = { ...healthyBoundaryState(["thread:1"]), ...failedBoundaryState("boom") }; + expect(boundaryResetFromProps(["thread:1"], failed)).toBeNull(); + expect(boundaryResetFromProps(["thread:2"], failed)).toEqual({ + failed: false, + error: undefined, + resetKeys: ["thread:2"], + }); + expect(boundaryResetFromProps(["thread:1", "extra"], failed)).not.toBeNull(); + }); + + it("treats unchanged absence of resetKeys as stable, not as a change", () => { + const failed = { ...healthyBoundaryState(undefined), ...failedBoundaryState("boom") }; + expect(boundaryResetFromProps(undefined, failed)).toBeNull(); + }); + + it("retrying a boundary returns to healthy while keeping the tracked keys", () => { + const failed = { ...healthyBoundaryState(["thread:1"]), ...failedBoundaryState("boom") }; + const retried = healthyBoundaryState(failed.resetKeys); + expect(retried.failed).toBe(false); + expect(boundaryResetFromProps(["thread:1"], retried)).toBeNull(); + }); +}); + +describe("screenFallbackExit", () => { + it("offers Go back when a previous route exists", () => { + expect(screenFallbackExit(true)).toBe("go-back"); + }); + + it("offers Settings when the crashing route is the only route", () => { + // Cold launch straight into a broken Home: no back gesture, and the + // Settings sheet is outside the failed subtree and always reachable. + expect(screenFallbackExit(false)).toBe("open-settings"); + }); +}); diff --git a/apps/mobile/src/components/render-error-boundary-model.ts b/apps/mobile/src/components/render-error-boundary-model.ts new file mode 100644 index 000000000000..fa8689f953d2 --- /dev/null +++ b/apps/mobile/src/components/render-error-boundary-model.ts @@ -0,0 +1,54 @@ +/** + * Pure state model for `RenderErrorBoundary`, kept separate from the RN view + * so the failure bookkeeping is directly testable. + * + * The failure signal is a dedicated `failed` flag, never the thrown value: + * `throw undefined` / `throw null` / `throw ""` must still fail the boundary, + * so the value itself can't double as the sentinel. + */ +export interface BoundaryState { + readonly failed: boolean; + readonly error: unknown; + readonly resetKeys?: ReadonlyArray | undefined; +} + +/** Returned by getDerivedStateFromError; omitting `resetKeys` keeps the tracked ones through the merge. */ +export function failedBoundaryState(error: unknown): Pick { + return { failed: true, error }; +} + +export function healthyBoundaryState( + resetKeys?: ReadonlyArray | undefined, +): BoundaryState { + return { failed: false, error: undefined, resetKeys }; +} + +/** + * `getDerivedStateFromProps` logic: changed `resetKeys` (e.g. a thread switch + * under a persistent boundary) clear a failure without waiting for user + * action; unchanged keys leave the current state untouched. + */ +export function boundaryResetFromProps( + resetKeys: ReadonlyArray | undefined, + state: BoundaryState, +): BoundaryState | null { + if ( + resetKeys?.length !== state.resetKeys?.length || + resetKeys?.some((key, index) => !Object.is(key, state.resetKeys?.[index])) + ) { + return healthyBoundaryState(resetKeys); + } + return null; +} + +/** + * Which exit the screen-level fallback offers: normally Go back, but when the + * crashing route is the only route (cold launch on Home), there is no previous + * route and no back gesture — the fallback must still lead somewhere that can + * show the recorded diagnostics, so it offers Settings instead. + */ +export type ScreenFallbackExit = "go-back" | "open-settings"; + +export function screenFallbackExit(canGoBack: boolean): ScreenFallbackExit { + return canGoBack ? "go-back" : "open-settings"; +} From 6babe9a634968a4937ad090580fad759aee4a25c Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 16:16:08 -0700 Subject: [PATCH 03/17] fix(mobile): close boundary coverage gaps found in review - NewTaskSheet declares its own screen `layout`, and React Navigation resolves `screen.layout ?? group ?? navigator screenLayout`, so the route silently bypassed the navigator-level seam. It now renders GuardedScreenLayout inside its own layout, wrapping the whole flow (and NewTaskFlowProvider) with a comment noting the override rule. - The split-view workspace sidebar and inspector render in RootStackLayout, outside every screen slot. Give ThreadNavigationSidebar and WorkspaceInspectorPane their own scoped boundaries so a pane failure spares the open thread and the rest of the workspace. - In split view the Thread route stays mounted across sidebar selections; screen boundaries now take resetKeys from the route params so a new thread does not inherit the previous route's failure state. - Drop the WeakSet identity dedupe from the render-error log: a cached error re-thrown on retry is a fresh incident, and identity dedupe would swallow it. Once a boundary recovers, the throw stops bubbling, so nested double-recording was not happening anyway. --- apps/mobile/src/Stack.tsx | 29 ++++++++-- .../src/components/RenderErrorBoundary.tsx | 4 +- .../diagnostics/render-error-log.test.ts | 26 ++++----- .../features/diagnostics/render-error-log.ts | 19 +++---- .../layout/AdaptiveWorkspaceLayout.tsx | 57 +++++++++++-------- 5 files changed, 80 insertions(+), 55 deletions(-) diff --git a/apps/mobile/src/Stack.tsx b/apps/mobile/src/Stack.tsx index ecfeed76fb77..5bf6de918f88 100644 --- a/apps/mobile/src/Stack.tsx +++ b/apps/mobile/src/Stack.tsx @@ -762,10 +762,15 @@ const RootStackConfig = createNativeStackNavigator({ // The whole new-task flow (choose project → draft → add project) shares // draft state via NewTaskFlowProvider. The expo-router era mounted it in // app/new/_layout.tsx; this layout wrapper is the native-stack equivalent. - layout: ({ children }) => ( - - {children} - + // A screen `layout` replaces the navigator's default screenLayout, so + // this route's boundary lives HERE, wrapping the whole flow (outside the + // provider: a provider crash is also caught, and retry remounts it). + layout: ({ children, route }) => ( + + + {children} + + ), options: { gestureEnabled: true, @@ -785,12 +790,24 @@ const RootStackConfig = createNativeStackNavigator({ // killing the app. Each route renders this layout within its own screen slot // (keyed by the navigator), so a popped route tears its boundary down. Screens // nested inside a sheet/stack route share that route's boundary. +// +// NOTE: React Navigation resolves the wrapper as `screen.layout ?? group +// layout ?? navigator screenLayout` (useDescriptors), so any screen that +// declares its own `layout` BYPASSES this default and must render +// GuardedScreenLayout inside its own layout (see NewTaskSheet below). function GuardedScreenLayout(props: { readonly children: ReactNode; - readonly route: { readonly name: string }; + readonly route: { readonly name: string; readonly params?: object | undefined }; }) { return ( - + {props.children} ); diff --git a/apps/mobile/src/components/RenderErrorBoundary.tsx b/apps/mobile/src/components/RenderErrorBoundary.tsx index ee079d3e419d..b4f997df9f8b 100644 --- a/apps/mobile/src/components/RenderErrorBoundary.tsx +++ b/apps/mobile/src/components/RenderErrorBoundary.tsx @@ -46,8 +46,8 @@ type RenderErrorBoundaryState = BoundaryState; * * A caught error never reaches the global fatal handler, so expo-updates' * ErrorRecovery startup log stays exclusively for process-ending fatals — - * `recordRenderError` is the sole report path here, deduped by error identity - * so an inner and outer boundary catching the same throw record it once. + * `recordRenderError` is the sole report path here. Once a boundary recovers, + * the throw stops bubbling, so only the innermost boundary records it. */ export class RenderErrorBoundary extends Component< RenderErrorBoundaryProps, diff --git a/apps/mobile/src/features/diagnostics/render-error-log.test.ts b/apps/mobile/src/features/diagnostics/render-error-log.test.ts index 4777d2ec70ee..03799fb5450a 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.test.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.test.ts @@ -16,8 +16,8 @@ describe("recordRenderError", () => { it("records the message, stack, scope, and timestamp, newest first", () => { const first = new Error("first broke"); const second = new Error("second broke"); - expect(recordRenderError(first, "thread-feed", { timestamp: 100 })).toBe(true); - expect(recordRenderError(second, "screen:Thread", { timestamp: 200 })).toBe(true); + recordRenderError(first, "thread-feed", { timestamp: 100 }); + recordRenderError(second, "screen:Thread", { timestamp: 200 }); const records = getRenderErrorRecords(); expect(records.map((record) => record.message)).toEqual(["second broke", "first broke"]); @@ -26,18 +26,16 @@ describe("recordRenderError", () => { expect(records[1]?.detail).toContain("Error: first broke"); }); - it("records one throw once when several boundaries on the path catch it", () => { - const error = new Error("bubble through everything"); - expect(recordRenderError(error, "thread-feed")).toBe(true); - // The same object keeps bubbling; the outer screen boundary must not add - // a second report of the identical crash. - expect(recordRenderError(error, "screen:Thread")).toBe(false); - expect(getRenderErrorRecords()).toHaveLength(1); - - // A fresh throw with the same message (e.g. after a failed retry) is a - // distinct crash and is recorded. - expect(recordRenderError(new Error("bubble through everything"), "thread-feed")).toBe(true); - expect(getRenderErrorRecords()).toHaveLength(2); + it("records a cached error again when a retry re-throws the same object", () => { + // A module-level or memoized Error keeps its identity across re-throws. + // Identity-based dedupe would silently swallow every crash after the + // first, which is exactly the report the retry most needs. + const cached = new Error("deterministically broken"); + recordRenderError(cached, "thread-feed", { timestamp: 100 }); + recordRenderError(cached, "thread-feed", { timestamp: 200 }); + const records = getRenderErrorRecords(); + expect(records).toHaveLength(2); + expect(records[0]?.timestamp).toBe(200); }); it("keeps the newest records when the log overflows", () => { diff --git a/apps/mobile/src/features/diagnostics/render-error-log.ts b/apps/mobile/src/features/diagnostics/render-error-log.ts index 1fbfb9c8e045..d52e8640deb1 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.ts @@ -8,6 +8,10 @@ * global fatal handler, so it could never appear there — recording it here is * the only way to surface it, and routing it anywhere else would double-report * what the startup log already owns. + * + * There is no dedupe by error identity on purpose: a cached error re-thrown on + * retry is a fresh incident worth recording again, and the same throw seen by + * nested boundaries leaves one row per scope, which reads as the bubble path. */ export interface RenderErrorRecord { readonly timestamp: number; @@ -19,7 +23,6 @@ export interface RenderErrorRecord { } const MAX_RECORDS = 20; -const recordedErrors = new WeakSet(); let records: RenderErrorRecord[] = []; export function describeRenderError(error: unknown): string { @@ -30,20 +33,17 @@ export function describeRenderError(error: unknown): string { } /** - * Record a caught render error. Returns false when this exact error object was - * already recorded (a boundary further up the tree saw the same throw), which - * is how ancestors stay silent while still resetting their subtree. + * Record a caught render error. Every boundary that catches a throw records it + * against its own scope, so one crash can produce one row per scope it bubbled + * through — a useful trail, and the opposite of suppressing a cached error + * that legitimately throws again after a retry. */ export function recordRenderError( error: unknown, scope: string, options: { readonly componentStack?: string | undefined; readonly timestamp?: number } = {}, -): boolean { +): void { const message = describeRenderError(error); - if (typeof error === "object" && error !== null) { - if (recordedErrors.has(error)) return false; - recordedErrors.add(error); - } const stack = error instanceof Error ? error.stack : undefined; const detail = [ message, @@ -58,7 +58,6 @@ export function recordRenderError( { timestamp: options.timestamp ?? Date.now(), scope, message, detail }, ...records, ].slice(0, MAX_RECORDS); - return true; } /** Newest first, as stored. */ diff --git a/apps/mobile/src/features/layout/AdaptiveWorkspaceLayout.tsx b/apps/mobile/src/features/layout/AdaptiveWorkspaceLayout.tsx index 6c6133c7ef13..52cb95f4cc70 100644 --- a/apps/mobile/src/features/layout/AdaptiveWorkspaceLayout.tsx +++ b/apps/mobile/src/features/layout/AdaptiveWorkspaceLayout.tsx @@ -31,6 +31,8 @@ import Animated, { } from "react-native-reanimated"; import { AsyncResult } from "effect/unstable/reactivity"; +import { RenderErrorBoundary } from "../../components/RenderErrorBoundary"; + import { deriveFileInspectorPaneLayout, deriveLayout, @@ -580,21 +582,28 @@ function AdaptiveWorkspaceLayoutContent( style={sidebarAnimatedStyle} > - - - + {/* The sidebar and inspector render OUTSIDE the navigator's + screen slots (the workspace layout wraps the whole stack), + so no screenLayout boundary covers them. Scope their own + failures here: a broken sidebar must not take the open + thread (or the app) down, and vice versa. */} + + + + + ) : null} @@ -624,14 +633,16 @@ function AdaptiveWorkspaceLayoutContent( - + + + From e5bdb01eebde71872c0f7fd369625d27c274bfac Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 16:18:05 -0700 Subject: [PATCH 04/17] fix(mobile): harden recovery view and cold-launch exit per review - `describeRenderError` now stringifies defensively: a thrown object whose `toString`/`Symbol.toPrimitive` rethrows can no longer turn the recovery view (or the log) into a second crash. - "Copy details" includes React's component stack when the runtime supplied one, so the copy from the fallback matches what Diagnostics records. - The cold-launch exit dead-end: when the broken route IS the SettingsSheet, the fallback offers Return home (StackActions.replace) instead of Open settings, which would navigate back onto the crashed, focused route. --- apps/mobile/src/Stack.tsx | 31 ++++++++++++--- .../src/components/RenderErrorBoundary.tsx | 39 ++++++++++++++++--- .../render-error-boundary-model.test.ts | 11 +++++- .../components/render-error-boundary-model.ts | 21 +++++++--- .../diagnostics/render-error-log.test.ts | 11 ++++++ .../features/diagnostics/render-error-log.ts | 14 ++++++- 6 files changed, 106 insertions(+), 21 deletions(-) diff --git a/apps/mobile/src/Stack.tsx b/apps/mobile/src/Stack.tsx index 5bf6de918f88..d4495d66b714 100644 --- a/apps/mobile/src/Stack.tsx +++ b/apps/mobile/src/Stack.tsx @@ -802,6 +802,7 @@ function GuardedScreenLayout(props: { return ( void }) { +function ScreenRenderFallback(props: { + readonly error: unknown; + readonly retry: () => void; + readonly routeName?: string | undefined; +}) { + // The seam renders OUTSIDE SceneView, so this hook resolves to the root + // navigation container — exactly the stack-level goBack/navigate/popToTop + // the recovery exits need, and it is unaffected by the failed subtree. const navigation = useNavigation(); - // The navigation container is outside the failed subtree, so navigating out - // still works even when the screen content cannot render. On a cold-launch - // crash there is no previous route to go back to; Settings (and its - // Diagnostics tab, where the caught errors are listed) is always reachable. - if (screenFallbackExit(navigation.canGoBack()) === "open-settings") { + const exit = screenFallbackExit({ + canGoBack: navigation.canGoBack(), + routeName: props.routeName ?? "", + }); + if (exit === "go-home") { + // Replace, not pop: on a single-route cold launch the broken sheet must + // unmount, or its fallback would stay on screen behind Home. + return ( + navigation.dispatch(StackActions.replace("Home"))} + /> + ); + } + if (exit === "open-settings") { return ( | undefined; /** Subject noun for the default fallback's headline, e.g. "The conversation". */ readonly subject?: string; + /** Forwarded to a custom `fallback` so it can adapt per route (screen seam). */ + readonly routeName?: string | undefined; /** * Recovery UI override, rendered as its own component so it can use hooks * (e.g. navigation) even though the boundary itself is a class. @@ -31,6 +33,10 @@ interface RenderErrorBoundaryProps { export interface RenderFallbackProps { readonly error: unknown; readonly retry: () => void; + /** React's component stack when the runtime captured one; feeds "Copy details". */ + readonly componentStack?: string | undefined; + /** Route name for screen-seam fallbacks that pick their exit per route. */ + readonly routeName?: string | undefined; } type RenderErrorBoundaryState = BoundaryState; @@ -70,6 +76,11 @@ export class RenderErrorBoundary extends Component< override componentDidCatch(error: unknown, info: { componentStack?: string }) { recordRenderError(error, this.props.scope, { componentStack: info.componentStack }); + // Keep the component path for the recovery view's "Copy details" too — + // in release builds it may be the only component stack anyone ever sees. + if (info.componentStack !== undefined) { + this.setState({ componentStack: info.componentStack }); + } } private readonly retry = () => { @@ -80,13 +91,21 @@ export class RenderErrorBoundary extends Component< if (this.state.failed) { if (this.props.fallback) { const Fallback = this.props.fallback; - return ; + return ( + + ); } return ( ); } @@ -104,14 +123,21 @@ export function RenderFailureView(props: { readonly subject?: string; readonly error: unknown; readonly retry: () => void; + readonly componentStack?: string | undefined; readonly onGoBack?: (() => void) | undefined; /** Escape exit for a cold-launch crash where there is no route to go back to. */ readonly onOpenSettings?: (() => void) | undefined; + /** Escape exit when even Settings is the broken route (replace stack with Home). */ + readonly onGoHome?: (() => void) | undefined; }) { - const message = - props.error instanceof Error ? props.error.message || props.error.name : String(props.error); + // Safe even for hostile throws (throwing `toString`, primitives, symbols). + const message = describeRenderError(props.error); const copy = async () => { - const detail = props.error instanceof Error ? (props.error.stack ?? message) : message; + const stack = props.error instanceof Error ? (props.error.stack ?? message) : message; + const detail = + props.componentStack !== undefined + ? `${stack}\nComponent stack:\n${props.componentStack}` + : stack; await tryCopyTextWithHaptic(detail, { target: "render error details" }); }; return ( @@ -151,6 +177,9 @@ export function RenderFailureView(props: { fullWidth /> ) : null} + {props.onGoHome ? ( + + ) : null} ); diff --git a/apps/mobile/src/components/render-error-boundary-model.test.ts b/apps/mobile/src/components/render-error-boundary-model.test.ts index 45282a7f1914..964e4c977c55 100644 --- a/apps/mobile/src/components/render-error-boundary-model.test.ts +++ b/apps/mobile/src/components/render-error-boundary-model.test.ts @@ -50,12 +50,19 @@ describe("boundaryResetFromProps", () => { describe("screenFallbackExit", () => { it("offers Go back when a previous route exists", () => { - expect(screenFallbackExit(true)).toBe("go-back"); + expect(screenFallbackExit({ canGoBack: true, routeName: "SettingsSheet" })).toBe("go-back"); + expect(screenFallbackExit({ canGoBack: true, routeName: "Home" })).toBe("go-back"); }); it("offers Settings when the crashing route is the only route", () => { // Cold launch straight into a broken Home: no back gesture, and the // Settings sheet is outside the failed subtree and always reachable. - expect(screenFallbackExit(false)).toBe("open-settings"); + expect(screenFallbackExit({ canGoBack: false, routeName: "Home" })).toBe("open-settings"); + }); + + it("offers Home when the Settings sheet itself is the cold-launch crash", () => { + // "Open settings" on a broken, already-focused SettingsSheet navigates to + // the broken route — the exit must replace the stack with Home instead. + expect(screenFallbackExit({ canGoBack: false, routeName: "SettingsSheet" })).toBe("go-home"); }); }); diff --git a/apps/mobile/src/components/render-error-boundary-model.ts b/apps/mobile/src/components/render-error-boundary-model.ts index fa8689f953d2..917f45f4b29b 100644 --- a/apps/mobile/src/components/render-error-boundary-model.ts +++ b/apps/mobile/src/components/render-error-boundary-model.ts @@ -9,10 +9,12 @@ export interface BoundaryState { readonly failed: boolean; readonly error: unknown; + /** React's component stack, when the runtime provides one; shown on copy. */ + readonly componentStack?: string | undefined; readonly resetKeys?: ReadonlyArray | undefined; } -/** Returned by getDerivedStateFromError; omitting `resetKeys` keeps the tracked ones through the merge. */ +/** Returned by getDerivedStateFromError; omitting other keys keeps them through the setState merge. */ export function failedBoundaryState(error: unknown): Pick { return { failed: true, error }; } @@ -20,7 +22,7 @@ export function failedBoundaryState(error: unknown): Pick | undefined, ): BoundaryState { - return { failed: false, error: undefined, resetKeys }; + return { failed: false, error: undefined, componentStack: undefined, resetKeys }; } /** @@ -45,10 +47,17 @@ export function boundaryResetFromProps( * Which exit the screen-level fallback offers: normally Go back, but when the * crashing route is the only route (cold launch on Home), there is no previous * route and no back gesture — the fallback must still lead somewhere that can - * show the recorded diagnostics, so it offers Settings instead. + * show the recorded diagnostics, so it offers Settings instead. When the + * broken route IS the Settings sheet itself, navigating to it would land back + * on the crash, so the only safe exit is replacing the stack with Home. */ -export type ScreenFallbackExit = "go-back" | "open-settings"; +export type ScreenFallbackExit = "go-back" | "open-settings" | "go-home"; -export function screenFallbackExit(canGoBack: boolean): ScreenFallbackExit { - return canGoBack ? "go-back" : "open-settings"; +export function screenFallbackExit(args: { + readonly canGoBack: boolean; + readonly routeName: string; +}): ScreenFallbackExit { + if (args.canGoBack) return "go-back"; + if (args.routeName === "SettingsSheet") return "go-home"; + return "open-settings"; } diff --git a/apps/mobile/src/features/diagnostics/render-error-log.test.ts b/apps/mobile/src/features/diagnostics/render-error-log.test.ts index 03799fb5450a..f45e7012bcba 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.test.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.test.ts @@ -66,6 +66,17 @@ describe("describeRenderError", () => { const thrown = { toString: () => "custom" }; expect(describeRenderError(thrown)).toBe("custom"); }); + + it("survives hostile throws whose toString rethrows", () => { + // Reporting and the recovery view must never become the second crash. + const thrown = { + toString() { + throw new Error("nope"); + }, + }; + expect(() => describeRenderError(thrown)).not.toThrow(); + expect(describeRenderError(thrown)).toBe("[object Object]"); + }); }); describe("formatRenderErrorReport", () => { diff --git a/apps/mobile/src/features/diagnostics/render-error-log.ts b/apps/mobile/src/features/diagnostics/render-error-log.ts index d52e8640deb1..c38e0d41055f 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.ts @@ -27,9 +27,19 @@ let records: RenderErrorRecord[] = []; export function describeRenderError(error: unknown): string { if (error instanceof Error) { - return error.message.trim().length > 0 ? error.message : error.name; + return safeString(error.message) || error.name; + } + return safeString(error); +} + +// A hostile `toString`/`Symbol.toPrimitive` must not turn error reporting — or +// the recovery view that renders the message — into a second crash. +function safeString(value: unknown): string { + try { + return String(value); + } catch { + return Object.prototype.toString.call(value); } - return String(error); } /** From 21c3d34d24b397af85fd60df6e2f87d28dd1a46f Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 16:19:03 -0700 Subject: [PATCH 05/17] fix(mobile): live-subscribe the Diagnostics render-error list The screen snapshotted the in-memory log at mount, so a crash recorded on another root route while Diagnostics stayed mounted (split view) left the list and the copied report stale. The log now replaces its snapshot array on every write and notifies subscribers; the screen reads it through useSyncExternalStore. --- .../SettingsDiagnosticsRouteScreen.tsx | 12 ++++--- .../diagnostics/render-error-log.test.ts | 33 +++++++++++++++++++ .../features/diagnostics/render-error-log.ts | 15 +++++++++ 3 files changed, 56 insertions(+), 4 deletions(-) diff --git a/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx b/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx index 3868ed33fda6..6cc5f6083f93 100644 --- a/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx +++ b/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx @@ -1,7 +1,7 @@ import { ScreenScrollView as ScrollView } from "../../components/ScreenScrollView"; import Constants from "expo-constants"; import * as Updates from "expo-updates"; -import { useEffect, useState } from "react"; +import { useEffect, useState, useSyncExternalStore } from "react"; import { ActivityIndicator, Platform, View } from "react-native"; import { useSafeAreaInsets } from "react-native-safe-area-context"; @@ -19,6 +19,7 @@ import { import { formatRenderErrorReport, getRenderErrorRecords, + subscribeToRenderErrors, type RenderErrorRecord, } from "./render-error-log"; @@ -53,9 +54,12 @@ export function SettingsDiagnosticsRouteScreen() { ); const [copied, setCopied] = useState(false); const [renderCopied, setRenderCopied] = useState(false); - // Render errors live only for the session (in memory, no persistence), and - // this screen mounts fresh each time it opens. - const [renderRecords] = useState>(getRenderErrorRecords); + // Session memory, not persisted. Subscribed because another root route can + // crash (and record) while this screen stays mounted, e.g. in split view. + const renderRecords = useSyncExternalStore>( + subscribeToRenderErrors, + getRenderErrorRecords, + ); useEffect(() => { if (!Updates.isEnabled) return; diff --git a/apps/mobile/src/features/diagnostics/render-error-log.test.ts b/apps/mobile/src/features/diagnostics/render-error-log.test.ts index f45e7012bcba..ecc68d61ad38 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.test.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.test.ts @@ -6,6 +6,7 @@ import { formatRenderErrorReport, getRenderErrorRecords, recordRenderError, + subscribeToRenderErrors, } from "./render-error-log"; beforeEach(() => { @@ -79,6 +80,38 @@ describe("describeRenderError", () => { }); }); +describe("subscribeToRenderErrors", () => { + it("notifies listeners and hands them a new snapshot on every write", () => { + const before = getRenderErrorRecords(); + let notified = 0; + const unsubscribe = subscribeToRenderErrors(() => { + notified += 1; + }); + + recordRenderError(new Error("crash while diagnostics is open"), "screen:Thread"); + expect(notified).toBe(1); + const after = getRenderErrorRecords(); + expect(after).not.toBe(before); + expect(after[0]?.message).toBe("crash while diagnostics is open"); + + unsubscribe(); + recordRenderError(new Error("after unsubscribe"), "screen:Thread"); + expect(notified).toBe(1); + }); + + it("notifies on clear so a mounted view cannot show stale rows", () => { + recordRenderError(new Error("something"), "screen:Thread"); + let notified = 0; + const unsubscribe = subscribeToRenderErrors(() => { + notified += 1; + }); + clearRenderErrorRecords(); + expect(notified).toBe(1); + expect(getRenderErrorRecords()).toHaveLength(0); + unsubscribe(); + }); +}); + describe("formatRenderErrorReport", () => { it("labels an empty session without pretending a crash happened", () => { const report = formatRenderErrorReport([], { version: "1.2.3", build: "45" }); diff --git a/apps/mobile/src/features/diagnostics/render-error-log.ts b/apps/mobile/src/features/diagnostics/render-error-log.ts index c38e0d41055f..cd4c1c0d8ff3 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.ts @@ -24,6 +24,19 @@ export interface RenderErrorRecord { const MAX_RECORDS = 20; let records: RenderErrorRecord[] = []; +const listeners = new Set<() => void>(); + +/** + * `useSyncExternalStore` pair. The records array is replaced (never mutated) + * on every write, so `getRenderErrorRecords` is a stable snapshot getter: the + * Diagnostics screen sees a crash recorded by another route while it is open. + */ +export function subscribeToRenderErrors(listener: () => void): () => void { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +} export function describeRenderError(error: unknown): string { if (error instanceof Error) { @@ -68,6 +81,7 @@ export function recordRenderError( { timestamp: options.timestamp ?? Date.now(), scope, message, detail }, ...records, ].slice(0, MAX_RECORDS); + for (const listener of listeners) listener(); } /** Newest first, as stored. */ @@ -77,6 +91,7 @@ export function getRenderErrorRecords(): ReadonlyArray { export function clearRenderErrorRecords(): void { records = []; + for (const listener of listeners) listener(); } /** The report a user pastes into an issue, mirroring the startup crash report. */ From a4e849c2183526c2bbf2da3f366529a160efa57a Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 16:25:25 -0700 Subject: [PATCH 06/17] fix(mobile): scope the inspector boundary inside the pane and harden safeString Review findings on head 21c3d34: - The inspector boundary wrapped the whole WorkspaceInspectorPane, so a content crash deleted the fixed-width column and resize divider and left the flex-1 fallback as a layout sibling. Move it inside the pane around the rendered content only; the column chrome survives. - It also had no resetKeys, so a healthy renderer after a route change kept showing the previous renderer's fallback. resetKeys now tracks the renderer identity. - safeString's fallback (Object.prototype.toString) could itself throw via a Symbol.toStringTag getter; guard it and return a constant, with a test. --- .../diagnostics/render-error-log.test.ts | 13 +++++++++++++ .../features/diagnostics/render-error-log.ts | 11 ++++++++--- .../layout/AdaptiveWorkspaceLayout.tsx | 18 ++++++++---------- .../layout/workspace-inspector-pane.tsx | 15 ++++++++++++++- 4 files changed, 43 insertions(+), 14 deletions(-) diff --git a/apps/mobile/src/features/diagnostics/render-error-log.test.ts b/apps/mobile/src/features/diagnostics/render-error-log.test.ts index ecc68d61ad38..afaffd2ef791 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.test.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.test.ts @@ -78,6 +78,19 @@ describe("describeRenderError", () => { expect(() => describeRenderError(thrown)).not.toThrow(); expect(describeRenderError(thrown)).toBe("[object Object]"); }); + + it("falls back to a constant when even the fallback stringify rethrows", () => { + const hostile = { + toString() { + throw new Error("nope"); + }, + get [Symbol.toStringTag]() { + throw new Error("also nope"); + }, + }; + expect(() => describeRenderError(hostile)).not.toThrow(); + expect(describeRenderError(hostile)).toBe("[unstringifiable value]"); + }); }); describe("subscribeToRenderErrors", () => { diff --git a/apps/mobile/src/features/diagnostics/render-error-log.ts b/apps/mobile/src/features/diagnostics/render-error-log.ts index cd4c1c0d8ff3..ab1953925b46 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.ts @@ -45,13 +45,18 @@ export function describeRenderError(error: unknown): string { return safeString(error); } -// A hostile `toString`/`Symbol.toPrimitive` must not turn error reporting — or -// the recovery view that renders the message — into a second crash. +// A hostile `toString`/`Symbol.toPrimitive`/`Symbol.toStringTag` must not turn +// error reporting — or the recovery view that renders the message — into a +// second crash, so even the fallback stringify is guarded. function safeString(value: unknown): string { try { return String(value); } catch { - return Object.prototype.toString.call(value); + try { + return Object.prototype.toString.call(value); + } catch { + return "[unstringifiable value]"; + } } } diff --git a/apps/mobile/src/features/layout/AdaptiveWorkspaceLayout.tsx b/apps/mobile/src/features/layout/AdaptiveWorkspaceLayout.tsx index 52cb95f4cc70..101d6ed31ceb 100644 --- a/apps/mobile/src/features/layout/AdaptiveWorkspaceLayout.tsx +++ b/apps/mobile/src/features/layout/AdaptiveWorkspaceLayout.tsx @@ -633,16 +633,14 @@ function AdaptiveWorkspaceLayoutContent( - - - + diff --git a/apps/mobile/src/features/layout/workspace-inspector-pane.tsx b/apps/mobile/src/features/layout/workspace-inspector-pane.tsx index 7c6574e22e6f..4fa8ea939f68 100644 --- a/apps/mobile/src/features/layout/workspace-inspector-pane.tsx +++ b/apps/mobile/src/features/layout/workspace-inspector-pane.tsx @@ -8,6 +8,7 @@ import Animated, { } from "react-native-reanimated"; import { constrainAuxiliaryPaneWidth, type WorkspacePaneLayout } from "../../lib/layout"; +import { RenderErrorBoundary } from "../../components/RenderErrorBoundary"; import { WORKSPACE_PANE_TIMING } from "./workspace-pane-animation"; import { WorkspacePaneDivider } from "./workspace-pane-divider"; @@ -139,7 +140,19 @@ export function WorkspaceInspectorPane(props: { style={inspectorStyle} > - {props.renderInspector?.()} + {/* INSIDE the pane so a content crash swaps only the content: the + fixed-width column, reveal animation, and resize divider stay + mounted and a fallback never becomes a flex sibling of the + column. The renderer identity is the content input — a new + inspector (e.g. after a route change) must not inherit the + previous renderer's failure. */} + + {props.renderInspector?.()} + ) : null} From aeb3582e793fbf6cdabe3d118c1fdf323637a024 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 16:33:44 -0700 Subject: [PATCH 07/17] fix(mobile): guard every hostile read path and catch inspector-callback throws - describeRenderError/recordRenderError/readErrorStack now read message, name, and stack through guarded access: an Error subclass with throwing getters can no longer explode inside componentDidCatch and defeat the recovery it is part of. The recovery view's Copy details uses the same guarded stack read. Tests cover hostile getters and ordinary errors. - The inspector invokes the route-supplied renderer in a dedicated child (InspectorRenderer) inside the boundary. Calling it as a children expression ran the callback during the pane's own render, above the boundary, where it could not be caught. - ScreenRenderFallback forwards the captured component stack to all three exits, so Copy details keeps the component path on every screen fallback. --- apps/mobile/src/Stack.tsx | 4 +++ .../src/components/RenderErrorBoundary.tsx | 10 +++++-- .../diagnostics/render-error-log.test.ts | 26 +++++++++++++++++ .../features/diagnostics/render-error-log.ts | 28 +++++++++++++++++-- .../layout/workspace-inspector-pane.tsx | 13 ++++++++- 5 files changed, 75 insertions(+), 6 deletions(-) diff --git a/apps/mobile/src/Stack.tsx b/apps/mobile/src/Stack.tsx index d4495d66b714..797af87a363d 100644 --- a/apps/mobile/src/Stack.tsx +++ b/apps/mobile/src/Stack.tsx @@ -817,6 +817,7 @@ function GuardedScreenLayout(props: { function ScreenRenderFallback(props: { readonly error: unknown; readonly retry: () => void; + readonly componentStack?: string | undefined; readonly routeName?: string | undefined; }) { // The seam renders OUTSIDE SceneView, so this hook resolves to the root @@ -834,6 +835,7 @@ function ScreenRenderFallback(props: { navigation.dispatch(StackActions.replace("Home"))} /> ); @@ -843,6 +845,7 @@ function ScreenRenderFallback(props: { navigation.navigate("SettingsSheet")} /> ); @@ -851,6 +854,7 @@ function ScreenRenderFallback(props: { navigation.goBack()} /> ); diff --git a/apps/mobile/src/components/RenderErrorBoundary.tsx b/apps/mobile/src/components/RenderErrorBoundary.tsx index fada95799e90..40bcaecc7a84 100644 --- a/apps/mobile/src/components/RenderErrorBoundary.tsx +++ b/apps/mobile/src/components/RenderErrorBoundary.tsx @@ -5,7 +5,11 @@ import { SymbolView } from "./AppSymbol"; import { AppText as Text } from "./AppText"; import { MaterialButton } from "./MaterialButton"; import { tryCopyTextWithHaptic } from "../lib/copyTextWithHaptic"; -import { describeRenderError, recordRenderError } from "../features/diagnostics/render-error-log"; +import { + describeRenderError, + readErrorStack, + recordRenderError, +} from "../features/diagnostics/render-error-log"; import { boundaryResetFromProps, failedBoundaryState, @@ -133,7 +137,9 @@ export function RenderFailureView(props: { // Safe even for hostile throws (throwing `toString`, primitives, symbols). const message = describeRenderError(props.error); const copy = async () => { - const stack = props.error instanceof Error ? (props.error.stack ?? message) : message; + // readErrorStack guards hostile stack getters too — copying must never + // itself crash the recovery view. + const stack = readErrorStack(props.error) ?? message; const detail = props.componentStack !== undefined ? `${stack}\nComponent stack:\n${props.componentStack}` diff --git a/apps/mobile/src/features/diagnostics/render-error-log.test.ts b/apps/mobile/src/features/diagnostics/render-error-log.test.ts index afaffd2ef791..60ef564572be 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.test.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.test.ts @@ -5,6 +5,7 @@ import { describeRenderError, formatRenderErrorReport, getRenderErrorRecords, + readErrorStack, recordRenderError, subscribeToRenderErrors, } from "./render-error-log"; @@ -91,6 +92,31 @@ describe("describeRenderError", () => { expect(() => describeRenderError(hostile)).not.toThrow(); expect(describeRenderError(hostile)).toBe("[unstringifiable value]"); }); + + it("survives Error objects whose message/name/stack getters throw", () => { + // All three are read inside componentDidCatch's record path; a throw from + // any of them would defeat the recovery it is part of. + const hostile = new Error("base"); + const explode = () => { + throw new Error("getter"); + }; + Object.defineProperty(hostile, "message", { get: explode }); + Object.defineProperty(hostile, "name", { get: explode }); + Object.defineProperty(hostile, "stack", { get: explode }); + + expect(() => recordRenderError(hostile, "thread-feed")).not.toThrow(); + expect(() => readErrorStack(hostile)).not.toThrow(); + expect(readErrorStack(hostile)).toBeUndefined(); + const record = getRenderErrorRecords()[0]; + expect(record?.message).toBe("Error"); + expect(record?.detail).toBe("Error"); + }); + + it("keeps ordinary Error fields when the getters behave", () => { + const error = new Error("plain failure"); + expect(describeRenderError(error)).toBe("plain failure"); + expect(readErrorStack(error)).toContain("plain failure"); + }); }); describe("subscribeToRenderErrors", () => { diff --git a/apps/mobile/src/features/diagnostics/render-error-log.ts b/apps/mobile/src/features/diagnostics/render-error-log.ts index ab1953925b46..8943e97b913f 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.ts @@ -40,11 +40,33 @@ export function subscribeToRenderErrors(listener: () => void): () => void { export function describeRenderError(error: unknown): string { if (error instanceof Error) { - return safeString(error.message) || error.name; + // message/name/stack can be throwing getters on hostile or exotic errors, + // and this runs inside componentDidCatch — a throw here would defeat the + // recovery it is part of, so every property read is guarded. + const message = readSafely(() => error.message); + if (typeof message === "string" && message.length > 0) return message; + const name = readSafely(() => error.name); + if (typeof name === "string" && name.length > 0) return name; + return "Error"; } return safeString(error); } +/** The error's own stack, or undefined when absent or unreadable. */ +export function readErrorStack(error: unknown): string | undefined { + if (!(error instanceof Error)) return undefined; + const stack = readSafely(() => error.stack); + return typeof stack === "string" ? stack : undefined; +} + +function readSafely(read: () => T): T | undefined { + try { + return read(); + } catch { + return undefined; + } +} + // A hostile `toString`/`Symbol.toPrimitive`/`Symbol.toStringTag` must not turn // error reporting — or the recovery view that renders the message — into a // second crash, so even the fallback stringify is guarded. @@ -72,10 +94,10 @@ export function recordRenderError( options: { readonly componentStack?: string | undefined; readonly timestamp?: number } = {}, ): void { const message = describeRenderError(error); - const stack = error instanceof Error ? error.stack : undefined; + const stack = readErrorStack(error); const detail = [ message, - stack !== undefined && stack !== null ? stack : "", + stack !== undefined ? stack : "", options.componentStack !== undefined && options.componentStack !== null ? `Component stack:${options.componentStack}` : "", diff --git a/apps/mobile/src/features/layout/workspace-inspector-pane.tsx b/apps/mobile/src/features/layout/workspace-inspector-pane.tsx index 4fa8ea939f68..3aecbd20c3f5 100644 --- a/apps/mobile/src/features/layout/workspace-inspector-pane.tsx +++ b/apps/mobile/src/features/layout/workspace-inspector-pane.tsx @@ -23,6 +23,17 @@ import { WorkspacePaneDivider } from "./workspace-pane-divider"; * Receives the pane layout via props (not the workspace context hook) so this * module stays import-cycle-free with AdaptiveWorkspaceLayout. */ +/** + * Invokes the route's inspector renderer INSIDE the boundary subtree. Calling + * `props.renderInspector()` as a children expression would run the callback + * during THIS component's render — a throw in the route-supplied callback + * escapes any boundary placed at the call site. Rendered as a child, the + * callback's frame sits under the boundary where it can be caught. + */ +function InspectorRenderer(props: { readonly render?: (() => ReactNode) | undefined }) { + return props.render?.() ?? null; +} + export function WorkspaceInspectorPane(props: { readonly renderedInspectorWidth: SharedValue; /** @@ -151,7 +162,7 @@ export function WorkspaceInspectorPane(props: { subject="The inspector" resetKeys={[props.renderInspector]} > - {props.renderInspector?.()} + From 3e01a27d8b5f5ee8e9a02d233a686bacc0e35189 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 16:41:06 -0700 Subject: [PATCH 08/17] fix(mobile): stabilize inspector reset identity and finish hostile-throw hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The inspector boundary keyed resets on the registered render callback, but registrants (ThreadRouteScreen) rebuild it on every active-turn update: a persistently crashing inspector reset, re-threw, and re-recorded on each update, burning through the bounded diagnostics log. Registrations now carry a stable content identity (thread key + inspector mode, review pane, files path), and the boundary resets only when that changes. inspectorResetKeys() encodes the rule with tests: same-owner callback rebuilds do not reset, content changes do. - describeRenderError/readErrorStack now also guard the `instanceof Error` check itself (Symbol.hasInstance / throwing getPrototypeOf traps), with a Proxy test — everything on the componentDidCatch path is throw-proof. - Diagnostics rows now key off a unique per-record id instead of timestamp+scope (which collided for two catches in the same millisecond) and without an array index. --- .../render-error-boundary-model.test.ts | 29 +++++++++++ .../components/render-error-boundary-model.ts | 17 ++++++ .../SettingsDiagnosticsRouteScreen.tsx | 6 +-- .../diagnostics/render-error-log.test.ts | 25 +++++++++ .../features/diagnostics/render-error-log.ts | 16 ++++-- .../features/files/ThreadFilesRouteScreen.tsx | 5 +- .../layout/AdaptiveWorkspaceLayout.tsx | 52 ++++++++++++------- .../layout/workspace-inspector-pane.tsx | 5 +- .../src/features/review/ReviewSheet.tsx | 5 +- .../features/threads/ThreadRouteScreen.tsx | 14 ++++- 10 files changed, 144 insertions(+), 30 deletions(-) diff --git a/apps/mobile/src/components/render-error-boundary-model.test.ts b/apps/mobile/src/components/render-error-boundary-model.test.ts index 964e4c977c55..08f0b3757c17 100644 --- a/apps/mobile/src/components/render-error-boundary-model.test.ts +++ b/apps/mobile/src/components/render-error-boundary-model.test.ts @@ -4,6 +4,7 @@ import { boundaryResetFromProps, failedBoundaryState, healthyBoundaryState, + inspectorResetKeys, screenFallbackExit, } from "./render-error-boundary-model"; @@ -48,6 +49,34 @@ describe("boundaryResetFromProps", () => { }); }); +describe("inspectorResetKeys", () => { + it("does not reset when the same owner rebuilds its render callback", () => { + // ThreadRouteScreen rebuilds the inspector callback on unrelated updates + // (active-turn churn). Keyed on the callback, a persistently crashing + // inspector would reset, re-throw, and re-record on every such update. + const renderOne = () => null; + const renderTwo = () => null; + const [firstKey] = inspectorResetKeys("thread:env1:t1:files", renderOne); + const [secondKey] = inspectorResetKeys("thread:env1:t1:files", renderTwo); + expect(Object.is(firstKey, secondKey)).toBe(true); + }); + + it("resets when the inspected content identity changes", () => { + const render = () => null; + expect(inspectorResetKeys("thread:env1:t1:files", render)).not.toEqual( + inspectorResetKeys("thread:env1:t2:files", render), + ); + expect(inspectorResetKeys("thread:env1:t1:git", render)).not.toEqual( + inspectorResetKeys("thread:env1:t1:files", render), + ); + }); + + it("falls back to the callback when no identity is registered", () => { + const render = () => null; + expect(inspectorResetKeys(undefined, render)).toEqual([render]); + }); +}); + describe("screenFallbackExit", () => { it("offers Go back when a previous route exists", () => { expect(screenFallbackExit({ canGoBack: true, routeName: "SettingsSheet" })).toBe("go-back"); diff --git a/apps/mobile/src/components/render-error-boundary-model.ts b/apps/mobile/src/components/render-error-boundary-model.ts index 917f45f4b29b..426f1c91a9ca 100644 --- a/apps/mobile/src/components/render-error-boundary-model.ts +++ b/apps/mobile/src/components/render-error-boundary-model.ts @@ -61,3 +61,20 @@ export function screenFallbackExit(args: { if (args.routeName === "SettingsSheet") return "go-home"; return "open-settings"; } + +/** + * Reset signature for the inspector boundary. The registrant's stable + * content identity (owner + content the user perceives, e.g. route thread + + * inspector mode) wins over the render callback: registrants like + * ThreadRouteScreen rebuild the callback on unrelated updates (active-turn + * churn), so keying on it would reset — re-throw, and re-record — a + * persistently crashing inspector on every such update, spamming the bounded + * diagnostics log. Without an identity there is nothing better than the + * callback itself. + */ +export function inspectorResetKeys( + contentIdentity: string | undefined, + render: (() => unknown) | undefined, +): ReadonlyArray { + return contentIdentity !== undefined ? [contentIdentity] : [render]; +} diff --git a/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx b/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx index 6cc5f6083f93..8ebf10f18bec 100644 --- a/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx +++ b/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx @@ -135,11 +135,7 @@ export function SettingsDiagnosticsRouteScreen() { /> ) : ( renderRecords.map((record, index) => ( - + )) )} diff --git a/apps/mobile/src/features/diagnostics/render-error-log.test.ts b/apps/mobile/src/features/diagnostics/render-error-log.test.ts index 60ef564572be..0c910008fb9f 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.test.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.test.ts @@ -40,6 +40,13 @@ describe("recordRenderError", () => { expect(records[0]?.timestamp).toBe(200); }); + it("keeps same-millisecond catches distinct for list identity", () => { + recordRenderError(new Error("a"), "screen:Thread", { timestamp: 100 }); + recordRenderError(new Error("b"), "screen:Thread", { timestamp: 100 }); + const [newest, oldest] = getRenderErrorRecords(); + expect(newest?.id).not.toBe(oldest?.id); + }); + it("keeps the newest records when the log overflows", () => { for (let index = 0; index < 25; index += 1) { recordRenderError(new Error(`error ${index}`), "screen:Home", { timestamp: index }); @@ -112,6 +119,24 @@ describe("describeRenderError", () => { expect(record?.detail).toBe("Error"); }); + it("survives throws where even instanceof Error rethrows", () => { + // instanceof walks the prototype chain; a Proxy with a throwing + // getPrototypeOf trap must not break the record path either. + const hostile = new Proxy( + {}, + { + getPrototypeOf() { + throw new TypeError("no proto"); + }, + }, + ); + expect(() => describeRenderError(hostile)).not.toThrow(); + expect(() => recordRenderError(hostile, "screen:Thread")).not.toThrow(); + expect(() => readErrorStack(hostile)).not.toThrow(); + expect(readErrorStack(hostile)).toBeUndefined(); + expect(getRenderErrorRecords()[0]?.message).toBe("[object Object]"); + }); + it("keeps ordinary Error fields when the getters behave", () => { const error = new Error("plain failure"); expect(describeRenderError(error)).toBe("plain failure"); diff --git a/apps/mobile/src/features/diagnostics/render-error-log.ts b/apps/mobile/src/features/diagnostics/render-error-log.ts index 8943e97b913f..5b2f255d7f5e 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.ts @@ -14,6 +14,8 @@ * nested boundaries leaves one row per scope, which reads as the bubble path. */ export interface RenderErrorRecord { + /** Unique per recorded catch; two catches in the same millisecond stay distinct. */ + readonly id: number; readonly timestamp: number; /** Where it was caught, e.g. `screen:Thread` or `thread-feed`. */ readonly scope: string; @@ -24,6 +26,7 @@ export interface RenderErrorRecord { const MAX_RECORDS = 20; let records: RenderErrorRecord[] = []; +let nextRecordId = 1; const listeners = new Set<() => void>(); /** @@ -39,7 +42,7 @@ export function subscribeToRenderErrors(listener: () => void): () => void { } export function describeRenderError(error: unknown): string { - if (error instanceof Error) { + if (isErrorLike(error)) { // message/name/stack can be throwing getters on hostile or exotic errors, // and this runs inside componentDidCatch — a throw here would defeat the // recovery it is part of, so every property read is guarded. @@ -54,11 +57,18 @@ export function describeRenderError(error: unknown): string { /** The error's own stack, or undefined when absent or unreadable. */ export function readErrorStack(error: unknown): string | undefined { - if (!(error instanceof Error)) return undefined; + if (!isErrorLike(error)) return undefined; const stack = readSafely(() => error.stack); return typeof stack === "string" ? stack : undefined; } +// instanceof consults the prototype chain (and Symbol.hasInstance), so even +// the type check must be guarded against exotic throws (e.g. a Proxy with a +// throwing getPrototypeOf trap) — this runs inside componentDidCatch. +function isErrorLike(error: unknown): error is Error { + return readSafely(() => error instanceof Error) ?? false; +} + function readSafely(read: () => T): T | undefined { try { return read(); @@ -105,7 +115,7 @@ export function recordRenderError( .filter((part) => part.length > 0) .join("\n"); records = [ - { timestamp: options.timestamp ?? Date.now(), scope, message, detail }, + { id: nextRecordId++, timestamp: options.timestamp ?? Date.now(), scope, message, detail }, ...records, ].slice(0, MAX_RECORDS); for (const listener of listeners) listener(); diff --git a/apps/mobile/src/features/files/ThreadFilesRouteScreen.tsx b/apps/mobile/src/features/files/ThreadFilesRouteScreen.tsx index 06ef8f6ba4d9..f6147fb7fb16 100644 --- a/apps/mobile/src/features/files/ThreadFilesRouteScreen.tsx +++ b/apps/mobile/src/features/files/ThreadFilesRouteScreen.tsx @@ -723,7 +723,10 @@ export function ThreadFileScreen(props: ThreadFileRouteScreenProps) { () => renderInspector(inspectorHeaderInset), [inspectorHeaderInset, renderInspector], ); - useRegisterWorkspaceInspector(fileInspector.supported ? renderWorkspaceInspector : undefined); + useRegisterWorkspaceInspector( + fileInspector.supported ? renderWorkspaceInspector : undefined, + fileInspector.supported ? `files:${relativePath ?? "tree"}` : undefined, + ); const fileMenuActions = useMemo(() => { if (relativePath === null) return []; diff --git a/apps/mobile/src/features/layout/AdaptiveWorkspaceLayout.tsx b/apps/mobile/src/features/layout/AdaptiveWorkspaceLayout.tsx index 101d6ed31ceb..934a2a4635b8 100644 --- a/apps/mobile/src/features/layout/AdaptiveWorkspaceLayout.tsx +++ b/apps/mobile/src/features/layout/AdaptiveWorkspaceLayout.tsx @@ -78,7 +78,10 @@ interface AdaptiveWorkspaceContextValue { * registration already took over — stale deactivates never clobber it. * Prefer useRegisterWorkspaceInspector over calling this directly. */ - readonly registerWorkspaceInspector: (render: () => ReactNode) => () => void; + readonly registerWorkspaceInspector: ( + render: () => ReactNode, + identity?: string | undefined, + ) => () => void; readonly setPrimarySidebarSearchQuery: (query: string) => void; readonly showAuxiliaryPane: (role: WorkspaceAuxiliaryPaneRole) => void; readonly toggleAuxiliaryPane: () => void; @@ -140,7 +143,10 @@ export function useAdaptiveWorkspacePaneRole(role: WorkspaceAuxiliaryPaneRole) { * animates closed, or is replaced seamlessly when the next route registers in * the same commit); focus re-registers it. */ -export function useRegisterWorkspaceInspector(render: (() => ReactNode) | undefined) { +export function useRegisterWorkspaceInspector( + render: (() => ReactNode) | undefined, + identity?: string | undefined, +) { const { registerWorkspaceInspector } = useAdaptiveWorkspaceLayout(); // Raw context values (not the useNavigation/useRoute wrappers) so the // portal re-provides exactly what this screen sees. @@ -168,8 +174,8 @@ export function useRegisterWorkspaceInspector(render: (() => ReactNode) | undefi deactivateRef.current?.(); return; } - deactivateRef.current = registerWorkspaceInspector(wrappedRenderRef.current); - }, [registerWorkspaceInspector]); + deactivateRef.current = registerWorkspaceInspector(wrappedRenderRef.current, identity); + }, [identity, registerWorkspaceInspector]); // Focus lifecycle. Blur/focus events fire even when the blurred subtree is // frozen (events are navigation-driven, renders are not). @@ -184,7 +190,9 @@ export function useRegisterWorkspaceInspector(render: (() => ReactNode) | undefi }, [syncRegistration]), ); - // Content changes while focused re-register in place. + // Content changes while focused re-register in place; identity rides the + // same syncRegistration change so the workspace's stored identity stays + // current even if a registrant changes it without changing the callback. useEffect(() => { if (focusedRef.current) { syncRegistration(); @@ -317,23 +325,30 @@ function AdaptiveWorkspaceLayoutContent( // seamlessly by the next route's registration in the same commit). const [workspaceInspector, setWorkspaceInspector] = useState<{ readonly render: () => ReactNode; + /** Registrant-provided stable content identity (see inspectorResetKeys). */ + readonly identity: string | undefined; readonly active: boolean; } | null>(null); const workspaceInspectorOwner = useRef(null); - const registerWorkspaceInspector = useCallback((render: () => ReactNode) => { - const owner = Symbol("workspace-inspector"); - workspaceInspectorOwner.current = owner; - setWorkspaceInspector({ render, active: true }); + const registerWorkspaceInspector = useCallback( + (render: () => ReactNode, identity?: string | undefined) => { + const owner = Symbol("workspace-inspector"); + workspaceInspectorOwner.current = owner; + setWorkspaceInspector({ render, identity, active: true }); - return () => { - // During a push/replace the outgoing screen deactivates AFTER the - // incoming screen registered — only the current owner may deactivate. - if (workspaceInspectorOwner.current !== owner) { - return; - } - setWorkspaceInspector((current) => (current === null ? null : { ...current, active: false })); - }; - }, []); + return () => { + // During a push/replace the outgoing screen deactivates AFTER the + // incoming screen registered — only the current owner may deactivate. + if (workspaceInspectorOwner.current !== owner) { + return; + } + setWorkspaceInspector((current) => + current === null ? null : { ...current, active: false }, + ); + }; + }, + [], + ); // Once the close animation settles, drop the stale content entirely. const handleWorkspaceInspectorClosed = useCallback(() => { setWorkspaceInspector((current) => (current !== null && !current.active ? null : current)); @@ -636,6 +651,7 @@ function AdaptiveWorkspaceLayoutContent( void; readonly panes: WorkspacePaneLayout; readonly renderInspector?: () => ReactNode; + /** Stable content identity from the registrant; resets the boundary only when the inspected content really changes. */ + readonly inspectorIdentity?: string | undefined; readonly setAuxiliaryPaneWidth: (width: number) => void; }) { const { panes, setAuxiliaryPaneWidth } = props; @@ -160,7 +163,7 @@ export function WorkspaceInspectorPane(props: { diff --git a/apps/mobile/src/features/review/ReviewSheet.tsx b/apps/mobile/src/features/review/ReviewSheet.tsx index 2fac5376e5b1..ef68594cc309 100644 --- a/apps/mobile/src/features/review/ReviewSheet.tsx +++ b/apps/mobile/src/features/review/ReviewSheet.tsx @@ -672,7 +672,10 @@ export function ReviewSheet(props: ReviewSheetProps) { selectedSection !== null && parsedDiff.kind === "files" && NativeReviewDiffView !== null; - useRegisterWorkspaceInspector(showChangedFilesPane ? renderInspector : undefined); + useRegisterWorkspaceInspector( + showChangedFilesPane ? renderInspector : undefined, + showChangedFilesPane ? "review:changed-files" : undefined, + ); // A toggle needs registered content; loading, errors and raw patches have no navigator pane. const showChangedFilesToggle = panes.supportsAuxiliaryPane && showChangedFilesPane; diff --git a/apps/mobile/src/features/threads/ThreadRouteScreen.tsx b/apps/mobile/src/features/threads/ThreadRouteScreen.tsx index e5bfc1590cf0..afb12c8081a0 100644 --- a/apps/mobile/src/features/threads/ThreadRouteScreen.tsx +++ b/apps/mobile/src/features/threads/ThreadRouteScreen.tsx @@ -615,7 +615,19 @@ function ThreadRouteContent( // Hand the inspector to the workspace so it renders beside the navigator, // outside this screen's native header — the terminal/git/files toolbar // stays anchored to the chat pane instead of floating above the inspector. - useRegisterWorkspaceInspector(activeInspectorRenderer); + // Stable content identity for the inspector boundary: the thread the pane + // actually shows plus its mode. Callback identity churns with active-turn + // updates (see renderInspectorStack) and must not drive the reset. + useRegisterWorkspaceInspector( + activeInspectorRenderer, + activeInspectorRenderer === undefined + ? undefined + : `thread:${ + selectedThread === null + ? "pending" + : scopedThreadKey(selectedThread.environmentId, selectedThread.id) + }:${inspectorMode ?? "none"}`, + ); const handleOpenConnectionEditor = useCallback(() => { void navigation.navigate("Connections"); From 401d3c70b6b6090d5a80c5eb408b2e3cf607bffc Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 16:47:33 -0700 Subject: [PATCH 09/17] fix(mobile): content-accurate inspector identities and isolated subscriber errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Review registrations all used `review:changed-files` although the pane shows different content per selected section, and files registrations keyed only on the relative path, which collides across environments and worktrees. Identity builders (reviewInspectorIdentity, filesInspectorIdentity) now encode what the user actually sees — section, environment, thread-or-cwd, path — so selecting a healthy section out of a crashed inspector resets the boundary, while unrelated rebuilds still do not. Tests cover both directions. - recordRenderError/clearRenderErrorRecords isolate each subscriber call: a throwing useSyncExternalStore listener can no longer propagate through componentDidCatch or starve the subscribers registered after it. --- .../render-error-boundary-model.test.ts | 28 +++++++++++++++++++ .../components/render-error-boundary-model.ts | 23 +++++++++++++++ .../diagnostics/render-error-log.test.ts | 21 ++++++++++++++ .../features/diagnostics/render-error-log.ts | 18 ++++++++++-- .../features/files/ThreadFilesRouteScreen.tsx | 8 +++++- .../src/features/review/ReviewSheet.tsx | 5 +++- 6 files changed, 99 insertions(+), 4 deletions(-) diff --git a/apps/mobile/src/components/render-error-boundary-model.test.ts b/apps/mobile/src/components/render-error-boundary-model.test.ts index 08f0b3757c17..0976ea31ed76 100644 --- a/apps/mobile/src/components/render-error-boundary-model.test.ts +++ b/apps/mobile/src/components/render-error-boundary-model.test.ts @@ -3,8 +3,10 @@ import { describe, expect, it } from "vite-plus/test"; import { boundaryResetFromProps, failedBoundaryState, + filesInspectorIdentity, healthyBoundaryState, inspectorResetKeys, + reviewInspectorIdentity, screenFallbackExit, } from "./render-error-boundary-model"; @@ -75,6 +77,32 @@ describe("inspectorResetKeys", () => { const render = () => null; expect(inspectorResetKeys(undefined, render)).toEqual([render]); }); + + it("resets a crashed review inspector when a healthy section is selected", () => { + const render = () => null; + const crashed = inspectorResetKeys(reviewInspectorIdentity("section-a"), render); + // Same section, rebuilt callback: still no reset (covered above). + expect(inspectorResetKeys(reviewInspectorIdentity("section-a"), () => null)).toEqual(crashed); + // Selecting a different, healthy section is new content: the boundary + // must not stay failed showing the crashed section's fallback. + expect(inspectorResetKeys(reviewInspectorIdentity("section-b"), render)).not.toEqual(crashed); + expect(inspectorResetKeys(reviewInspectorIdentity(undefined), render)).not.toEqual(crashed); + }); + + it("keeps the same relative path in another workspace distinct", () => { + const render = () => null; + const base = { environmentId: "env1", threadOrWorkspace: "t1", relativePath: "src/a.ts" }; + expect(inspectorResetKeys(filesInspectorIdentity(base), render)).not.toEqual( + inspectorResetKeys(filesInspectorIdentity({ ...base, environmentId: "env2" }), render), + ); + expect(inspectorResetKeys(filesInspectorIdentity(base), render)).not.toEqual( + inspectorResetKeys(filesInspectorIdentity({ ...base, threadOrWorkspace: "t2" }), render), + ); + // Same content from any registrant → same identity, no spurious resets. + expect(inspectorResetKeys(filesInspectorIdentity(base), render)).toEqual( + inspectorResetKeys(filesInspectorIdentity({ ...base }), render), + ); + }); }); describe("screenFallbackExit", () => { diff --git a/apps/mobile/src/components/render-error-boundary-model.ts b/apps/mobile/src/components/render-error-boundary-model.ts index 426f1c91a9ca..a22875301533 100644 --- a/apps/mobile/src/components/render-error-boundary-model.ts +++ b/apps/mobile/src/components/render-error-boundary-model.ts @@ -78,3 +78,26 @@ export function inspectorResetKeys( ): ReadonlyArray { return contentIdentity !== undefined ? [contentIdentity] : [render]; } + +/** + * Identity builders for the known registrants. The rule each encodes: the + * identity changes exactly when the content the user perceives changes — + * selecting a healthy section out of a crashed inspector must reset, while + * unrelated route state churn must not. + */ +export function reviewInspectorIdentity(sectionId: string | undefined): string { + return `review:changed-files:${sectionId ?? "none"}`; +} + +/** + * Files content is workspace-scoped: the same relative path in another + * environment or another thread/worktree is different content, so both the + * environment and the thread-or-cwd are part of the identity. + */ +export function filesInspectorIdentity(args: { + readonly environmentId: string | null | undefined; + readonly threadOrWorkspace: string | null | undefined; + readonly relativePath: string | null; +}): string { + return `files:${args.environmentId ?? "none"}:${args.threadOrWorkspace ?? "none"}:${args.relativePath ?? "tree"}`; +} diff --git a/apps/mobile/src/features/diagnostics/render-error-log.test.ts b/apps/mobile/src/features/diagnostics/render-error-log.test.ts index 0c910008fb9f..004c46fa1638 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.test.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.test.ts @@ -174,6 +174,27 @@ describe("subscribeToRenderErrors", () => { expect(getRenderErrorRecords()).toHaveLength(0); unsubscribe(); }); + + it("isolates a throwing subscriber on both notify paths", () => { + // recordRenderError runs inside componentDidCatch; a throwing subscriber + // must not propagate through the catch, and must not starve the + // subscribers registered after it. + let healthyNotified = 0; + const unsubscribeBad = subscribeToRenderErrors(() => { + throw new Error("subscriber exploded"); + }); + const unsubscribeGood = subscribeToRenderErrors(() => { + healthyNotified += 1; + }); + + expect(() => recordRenderError(new Error("crash"), "screen:Thread")).not.toThrow(); + expect(healthyNotified).toBe(1); + expect(() => clearRenderErrorRecords()).not.toThrow(); + expect(healthyNotified).toBe(2); + + unsubscribeBad(); + unsubscribeGood(); + }); }); describe("formatRenderErrorReport", () => { diff --git a/apps/mobile/src/features/diagnostics/render-error-log.ts b/apps/mobile/src/features/diagnostics/render-error-log.ts index 5b2f255d7f5e..051801b06576 100644 --- a/apps/mobile/src/features/diagnostics/render-error-log.ts +++ b/apps/mobile/src/features/diagnostics/render-error-log.ts @@ -29,6 +29,20 @@ let records: RenderErrorRecord[] = []; let nextRecordId = 1; const listeners = new Set<() => void>(); +// recordRenderError runs inside componentDidCatch: one broken subscriber must +// not replace the recovery path mid-catch, nor starve the subscribers after +// it (useSyncExternalStore listeners must all see the change). +function notifyRenderErrorListeners(): void { + for (const listener of listeners) { + try { + listener(); + } catch { + // The log's own write already succeeded; a subscriber's render error + // will surface through its own boundary, not through this stack. + } + } +} + /** * `useSyncExternalStore` pair. The records array is replaced (never mutated) * on every write, so `getRenderErrorRecords` is a stable snapshot getter: the @@ -118,7 +132,7 @@ export function recordRenderError( { id: nextRecordId++, timestamp: options.timestamp ?? Date.now(), scope, message, detail }, ...records, ].slice(0, MAX_RECORDS); - for (const listener of listeners) listener(); + notifyRenderErrorListeners(); } /** Newest first, as stored. */ @@ -128,7 +142,7 @@ export function getRenderErrorRecords(): ReadonlyArray { export function clearRenderErrorRecords(): void { records = []; - for (const listener of listeners) listener(); + notifyRenderErrorListeners(); } /** The report a user pastes into an issue, mirroring the startup crash report. */ diff --git a/apps/mobile/src/features/files/ThreadFilesRouteScreen.tsx b/apps/mobile/src/features/files/ThreadFilesRouteScreen.tsx index f6147fb7fb16..56353d43221e 100644 --- a/apps/mobile/src/features/files/ThreadFilesRouteScreen.tsx +++ b/apps/mobile/src/features/files/ThreadFilesRouteScreen.tsx @@ -1,5 +1,6 @@ import { NativeStackScreenOptions } from "../../native/StackHeader"; import { StackActions, useNavigation, type StaticScreenProps } from "@react-navigation/native"; +import { filesInspectorIdentity } from "../../components/render-error-boundary-model"; import { useCallback, useEffect, useId, useMemo, useRef, useState } from "react"; import { Platform, View } from "react-native"; import { useSafeAreaInsets } from "react-native-safe-area-context"; @@ -725,7 +726,12 @@ export function ThreadFileScreen(props: ThreadFileRouteScreenProps) { ); useRegisterWorkspaceInspector( fileInspector.supported ? renderWorkspaceInspector : undefined, - fileInspector.supported ? `files:${relativePath ?? "tree"}` : undefined, + filesInspectorIdentity({ + environmentId, + // Draft-mode screens carry cwd instead of a thread. + threadOrWorkspace: threadId ?? cwd, + relativePath, + }), ); const fileMenuActions = useMemo(() => { diff --git a/apps/mobile/src/features/review/ReviewSheet.tsx b/apps/mobile/src/features/review/ReviewSheet.tsx index ef68594cc309..6fa962a3ed52 100644 --- a/apps/mobile/src/features/review/ReviewSheet.tsx +++ b/apps/mobile/src/features/review/ReviewSheet.tsx @@ -1,5 +1,6 @@ import type { EnvironmentId, ThreadId } from "@t3tools/contracts"; import { useNavigation, type StaticScreenProps } from "@react-navigation/native"; +import { reviewInspectorIdentity } from "../../components/render-error-boundary-model"; import { nativeHeaderScrollEdgeEffects } from "../../native/StackHeader"; import { ScreenHeader } from "../../components/ScreenHeader"; import type { ScreenHeaderMenuItem } from "../../components/ScreenHeader.types"; @@ -674,7 +675,9 @@ export function ReviewSheet(props: ReviewSheetProps) { NativeReviewDiffView !== null; useRegisterWorkspaceInspector( showChangedFilesPane ? renderInspector : undefined, - showChangedFilesPane ? "review:changed-files" : undefined, + // Per-section identity: selecting a healthy section out of a crashed + // changed-files inspector must reset the boundary (see tests). + showChangedFilesPane ? reviewInspectorIdentity(selectedSection?.id) : undefined, ); // A toggle needs registered content; loading, errors and raw patches have no navigator pane. const showChangedFilesToggle = panes.supportsAuxiliaryPane && showChangedFilesPane; From d2c8618187e3d7a1f249bfce393c6cb7de7f426f Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 16:53:42 -0700 Subject: [PATCH 10/17] fix(mobile): make every inspector identity fully workspace-bound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three audited collisions where a crashed inspector fallback could persist over new healthy content: review keyed on section id only (ids recur across threads/worktrees), files used threadId ?? cwd (a worktree move under a stable thread id left the key unchanged), and the thread screen's workspace-bound Files/Git modes keyed on thread + mode without the cwd they actually render. One builder — source, workspace key, cwd, content selection — replaces both helpers; tests cover failed-fallback -> healthy content switching for each registrant path. --- .../render-error-boundary-model.test.ts | 90 ++++++++++++++----- .../components/render-error-boundary-model.ts | 29 +++--- .../features/files/ThreadFilesRouteScreen.tsx | 18 ++-- .../src/features/review/ReviewSheet.tsx | 17 +++- .../features/threads/ThreadRouteScreen.tsx | 18 ++-- 5 files changed, 121 insertions(+), 51 deletions(-) diff --git a/apps/mobile/src/components/render-error-boundary-model.test.ts b/apps/mobile/src/components/render-error-boundary-model.test.ts index 0976ea31ed76..f9722d9c385e 100644 --- a/apps/mobile/src/components/render-error-boundary-model.test.ts +++ b/apps/mobile/src/components/render-error-boundary-model.test.ts @@ -3,11 +3,10 @@ import { describe, expect, it } from "vite-plus/test"; import { boundaryResetFromProps, failedBoundaryState, - filesInspectorIdentity, healthyBoundaryState, inspectorResetKeys, - reviewInspectorIdentity, screenFallbackExit, + workspaceInspectorContentIdentity, } from "./render-error-boundary-model"; describe("failedBoundaryState", () => { @@ -78,31 +77,80 @@ describe("inspectorResetKeys", () => { expect(inspectorResetKeys(undefined, render)).toEqual([render]); }); - it("resets a crashed review inspector when a healthy section is selected", () => { + it("resets a crashed review inspector on new section, thread, or worktree", () => { const render = () => null; - const crashed = inspectorResetKeys(reviewInspectorIdentity("section-a"), render); - // Same section, rebuilt callback: still no reset (covered above). - expect(inspectorResetKeys(reviewInspectorIdentity("section-a"), () => null)).toEqual(crashed); - // Selecting a different, healthy section is new content: the boundary - // must not stay failed showing the crashed section's fallback. - expect(inspectorResetKeys(reviewInspectorIdentity("section-b"), render)).not.toEqual(crashed); - expect(inspectorResetKeys(reviewInspectorIdentity(undefined), render)).not.toEqual(crashed); + const crashed = inspectorResetKeys( + workspaceInspectorContentIdentity({ + source: "review", + workspaceKey: "env1|t1", + cwd: "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/wt/a", + contentId: "section-a", + }), + render, + ); + // Same content, rebuilt callback: still no reset (covered above). + // New section, same section id in another thread, or a moved worktree + // are all new content and must clear the crashed fallback. + const switched = [{ contentId: "section-b" }, { workspaceKey: "env1|t2" }, { cwd: "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/wt/b" }]; + for (const change of switched) { + expect( + inspectorResetKeys( + workspaceInspectorContentIdentity({ + source: "review", + workspaceKey: "env1|t1", + cwd: "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/wt/a", + contentId: "section-a", + ...change, + }), + render, + ), + ).not.toEqual(crashed); + } }); - it("keeps the same relative path in another workspace distinct", () => { + it("resets a crashed files inspector when the worktree moves under a stable thread", () => { const render = () => null; - const base = { environmentId: "env1", threadOrWorkspace: "t1", relativePath: "src/a.ts" }; - expect(inspectorResetKeys(filesInspectorIdentity(base), render)).not.toEqual( - inspectorResetKeys(filesInspectorIdentity({ ...base, environmentId: "env2" }), render), - ); - expect(inspectorResetKeys(filesInspectorIdentity(base), render)).not.toEqual( - inspectorResetKeys(filesInspectorIdentity({ ...base, threadOrWorkspace: "t2" }), render), - ); - // Same content from any registrant → same identity, no spurious resets. - expect(inspectorResetKeys(filesInspectorIdentity(base), render)).toEqual( - inspectorResetKeys(filesInspectorIdentity({ ...base }), render), + const base = { + source: "files" as const, + workspaceKey: "env1|t1", + cwd: "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/wt/a", + contentId: "src/a.ts", + }; + const crashed = inspectorResetKeys(workspaceInspectorContentIdentity(base), render); + // threadId present but cwd changed: the audited collision. + expect( + inspectorResetKeys(workspaceInspectorContentIdentity({ ...base, cwd: "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/wt/b" }), render), + ).not.toEqual(crashed); + expect( + inspectorResetKeys( + workspaceInspectorContentIdentity({ ...base, workspaceKey: "env1|t2" }), + render, + ), + ).not.toEqual(crashed); + // Same content → same identity: unrelated callback rebuilds still do not reset. + expect(inspectorResetKeys(workspaceInspectorContentIdentity({ ...base }), render)).toEqual( + crashed, ); }); + + it("resets a crashed thread inspector when the inspected cwd changes", () => { + const render = () => null; + const base = { + source: "thread" as const, + workspaceKey: "env1|t1", + cwd: "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/wt/a", + contentId: "files", + }; + const crashed = inspectorResetKeys(workspaceInspectorContentIdentity(base), render); + // Same thread and mode, worktree moved: workspace-bound content changed. + expect( + inspectorResetKeys(workspaceInspectorContentIdentity({ ...base, cwd: "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/wt/b" }), render), + ).not.toEqual(crashed); + // Sources stay distinct even with otherwise identical parts. + expect( + inspectorResetKeys(workspaceInspectorContentIdentity({ ...base, source: "files" }), render), + ).not.toEqual(crashed); + }); }); describe("screenFallbackExit", () => { diff --git a/apps/mobile/src/components/render-error-boundary-model.ts b/apps/mobile/src/components/render-error-boundary-model.ts index a22875301533..c4966e01cb79 100644 --- a/apps/mobile/src/components/render-error-boundary-model.ts +++ b/apps/mobile/src/components/render-error-boundary-model.ts @@ -80,24 +80,23 @@ export function inspectorResetKeys( } /** - * Identity builders for the known registrants. The rule each encodes: the + * Identity builder for the known registrants. The rule each encodes: the * identity changes exactly when the content the user perceives changes — * selecting a healthy section out of a crashed inspector must reset, while * unrelated route state churn must not. + * + * Every inspector's content is workspace-bound (a diff, a file tree, a thread + * view), so all three parts ride the key: the route/thread the content + * belongs to, the cwd it renders (a thread's worktree can move, and + * same-id content recurs across workspaces), and the content selection + * itself. Keying on any subset lets a crashed fallback persist over new, + * healthy content. */ -export function reviewInspectorIdentity(sectionId: string | undefined): string { - return `review:changed-files:${sectionId ?? "none"}`; -} - -/** - * Files content is workspace-scoped: the same relative path in another - * environment or another thread/worktree is different content, so both the - * environment and the thread-or-cwd are part of the identity. - */ -export function filesInspectorIdentity(args: { - readonly environmentId: string | null | undefined; - readonly threadOrWorkspace: string | null | undefined; - readonly relativePath: string | null; +export function workspaceInspectorContentIdentity(args: { + readonly source: "thread" | "review" | "files"; + readonly workspaceKey: string | null | undefined; + readonly cwd: string | null | undefined; + readonly contentId: string | null | undefined; }): string { - return `files:${args.environmentId ?? "none"}:${args.threadOrWorkspace ?? "none"}:${args.relativePath ?? "tree"}`; + return `${args.source}:${args.workspaceKey ?? "none"}:${args.cwd ?? "none"}:${args.contentId ?? "none"}`; } diff --git a/apps/mobile/src/features/files/ThreadFilesRouteScreen.tsx b/apps/mobile/src/features/files/ThreadFilesRouteScreen.tsx index 56353d43221e..cac3ab14a690 100644 --- a/apps/mobile/src/features/files/ThreadFilesRouteScreen.tsx +++ b/apps/mobile/src/features/files/ThreadFilesRouteScreen.tsx @@ -1,6 +1,7 @@ import { NativeStackScreenOptions } from "../../native/StackHeader"; import { StackActions, useNavigation, type StaticScreenProps } from "@react-navigation/native"; -import { filesInspectorIdentity } from "../../components/render-error-boundary-model"; +import { workspaceInspectorContentIdentity } from "../../components/render-error-boundary-model"; +import { scopedThreadKey } from "../../lib/scopedEntities"; import { useCallback, useEffect, useId, useMemo, useRef, useState } from "react"; import { Platform, View } from "react-native"; import { useSafeAreaInsets } from "react-native-safe-area-context"; @@ -726,11 +727,16 @@ export function ThreadFileScreen(props: ThreadFileRouteScreenProps) { ); useRegisterWorkspaceInspector( fileInspector.supported ? renderWorkspaceInspector : undefined, - filesInspectorIdentity({ - environmentId, - // Draft-mode screens carry cwd instead of a thread. - threadOrWorkspace: threadId ?? cwd, - relativePath, + // Thread and cwd are BOTH part of the key: a thread's inspected + // worktree can move (cwd change) while the thread id stays the same. + workspaceInspectorContentIdentity({ + source: "files", + workspaceKey: + environmentId !== null && threadId !== null + ? scopedThreadKey(environmentId, threadId) + : null, + cwd, + contentId: relativePath, }), ); diff --git a/apps/mobile/src/features/review/ReviewSheet.tsx b/apps/mobile/src/features/review/ReviewSheet.tsx index 6fa962a3ed52..85acdf65db37 100644 --- a/apps/mobile/src/features/review/ReviewSheet.tsx +++ b/apps/mobile/src/features/review/ReviewSheet.tsx @@ -1,6 +1,6 @@ import type { EnvironmentId, ThreadId } from "@t3tools/contracts"; import { useNavigation, type StaticScreenProps } from "@react-navigation/native"; -import { reviewInspectorIdentity } from "../../components/render-error-boundary-model"; +import { workspaceInspectorContentIdentity } from "../../components/render-error-boundary-model"; import { nativeHeaderScrollEdgeEffects } from "../../native/StackHeader"; import { ScreenHeader } from "../../components/ScreenHeader"; import type { ScreenHeaderMenuItem } from "../../components/ScreenHeader.types"; @@ -675,9 +675,18 @@ export function ReviewSheet(props: ReviewSheetProps) { NativeReviewDiffView !== null; useRegisterWorkspaceInspector( showChangedFilesPane ? renderInspector : undefined, - // Per-section identity: selecting a healthy section out of a crashed - // changed-files inspector must reset the boundary (see tests). - showChangedFilesPane ? reviewInspectorIdentity(selectedSection?.id) : undefined, + // Workspace-scoped per-section identity: the same section id can recur + // across threads/worktrees, and a thread's worktree cwd can move, so the + // thread key and cwd ride the key. Selecting new healthy content (new + // section, new cwd) resets; unrelated rebuilds do not. + showChangedFilesPane + ? workspaceInspectorContentIdentity({ + source: "review", + workspaceKey: reviewCache.threadKey, + cwd: selectedThreadCwd, + contentId: selectedSection?.id, + }) + : undefined, ); // A toggle needs registered content; loading, errors and raw patches have no navigator pane. const showChangedFilesToggle = panes.supportsAuxiliaryPane && showChangedFilesPane; diff --git a/apps/mobile/src/features/threads/ThreadRouteScreen.tsx b/apps/mobile/src/features/threads/ThreadRouteScreen.tsx index afb12c8081a0..4848571e36d0 100644 --- a/apps/mobile/src/features/threads/ThreadRouteScreen.tsx +++ b/apps/mobile/src/features/threads/ThreadRouteScreen.tsx @@ -48,6 +48,7 @@ import { vcsEnvironment } from "../../state/vcs"; import { EmptyState } from "../../components/EmptyState"; import { LoadingScreen } from "../../components/LoadingScreen"; import { scopedThreadKey } from "../../lib/scopedEntities"; +import { workspaceInspectorContentIdentity } from "../../components/render-error-boundary-model"; import { NATIVE_LIQUID_GLASS_SUPPORTED } from "../../native/native-glass"; import { connectionTone } from "../connection/connectionTone"; import { @@ -620,13 +621,20 @@ function ThreadRouteContent( // updates (see renderInspectorStack) and must not drive the reset. useRegisterWorkspaceInspector( activeInspectorRenderer, + // Thread key + cwd + mode: the Files/Git inspectors render the thread's + // current worktree, and the cwd can move under a stable thread id, so a + // crashed inspector must reset when the workspace it shows changes. activeInspectorRenderer === undefined ? undefined - : `thread:${ - selectedThread === null - ? "pending" - : scopedThreadKey(selectedThread.environmentId, selectedThread.id) - }:${inspectorMode ?? "none"}`, + : workspaceInspectorContentIdentity({ + source: "thread", + workspaceKey: + selectedThread === null + ? null + : scopedThreadKey(selectedThread.environmentId, selectedThread.id), + cwd: selectedThreadCwd, + contentId: inspectorMode, + }), ); const handleOpenConnectionEditor = useCallback(() => { From 2232d2d72b476fea6c4bbee88aa92a73b3efd39d Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 16:59:19 -0700 Subject: [PATCH 11/17] docs(mobile): correct the seam fallback's navigation comment The fallback resolves through Screen's per-route context, so it holds the guarded route's own root-stack navigation, not the navigation container's. --- apps/mobile/src/Stack.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/apps/mobile/src/Stack.tsx b/apps/mobile/src/Stack.tsx index 797af87a363d..1aacc8fbecf8 100644 --- a/apps/mobile/src/Stack.tsx +++ b/apps/mobile/src/Stack.tsx @@ -820,9 +820,10 @@ function ScreenRenderFallback(props: { readonly componentStack?: string | undefined; readonly routeName?: string | undefined; }) { - // The seam renders OUTSIDE SceneView, so this hook resolves to the root - // navigation container — exactly the stack-level goBack/navigate/popToTop - // the recovery exits need, and it is unaffected by the failed subtree. + // Screen's per-route context wraps the layout, so this hook resolves to + // the guarded root-stack route's own navigation — the same stack the exit + // actions (goBack/navigate/replace) need — and it lives outside the + // failed subtree, so recovery keeps working when the screen cannot render. const navigation = useNavigation(); const exit = screenFallbackExit({ canGoBack: navigation.canGoBack(), From 333b8feae9c3dbaaf98935e8e01533f877724e6a Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 17:06:13 -0700 Subject: [PATCH 12/17] fix(mobile): keep never-painted Home crashes fatal and de-collide reset identities MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Cold-launch OTA lockout: checkForAppUpdateOnLaunch lives in HomeRouteScreen's effect, so a bad OTA crashing Home before first paint would strand the user on a fallback whose update check never ran — and ErrorRecovery's rollback only fires for fatals. The Home seam now rethrows when the guarded subtree has never committed healthy children, restoring the pre-boundary fatal path (rollback + its startup crash log, deliberately not also recorded in the render-error log). Any failure after the first successful paint still recovers in-session on every screen, Home included. - workspaceInspectorContentIdentity is a JSON tuple, not a ':'-join: paths and ids contain colons, and null previously mapped to the same string a real content id of "none" would produce. - The thread feed boundary resets on cwd changes too: the feed renders the worktree setup surface, so a same-thread worktree move is new content. threadFeedResetKeys covers it with a test. --- apps/mobile/src/Stack.tsx | 6 ++ .../src/components/RenderErrorBoundary.tsx | 35 ++++++++++ .../render-error-boundary-model.test.ts | 70 +++++++++++++++++++ .../components/render-error-boundary-model.ts | 42 ++++++++++- .../features/threads/ThreadDetailScreen.tsx | 3 +- 5 files changed, 154 insertions(+), 2 deletions(-) diff --git a/apps/mobile/src/Stack.tsx b/apps/mobile/src/Stack.tsx index 1aacc8fbecf8..7d1835b10a1d 100644 --- a/apps/mobile/src/Stack.tsx +++ b/apps/mobile/src/Stack.tsx @@ -803,6 +803,12 @@ function GuardedScreenLayout(props: { | undefined; /** Subject noun for the default fallback's headline, e.g. "The conversation". */ readonly subject?: string; + /** + * Cold-launch safety valve (Home seam only): a failure before the guarded + * subtree has ever committed rethrows to the global handler so expo-updates' + * ErrorRecovery rollback and its startup crash log behave exactly as before + * boundaries existed. See shouldRethrowAsFatal. + */ + readonly fatalIfFirstPaintFails?: boolean | undefined; /** Forwarded to a custom `fallback` so it can adapt per route (screen seam). */ readonly routeName?: string | undefined; /** @@ -79,6 +87,22 @@ export class RenderErrorBoundary extends Component< } override componentDidCatch(error: unknown, info: { componentStack?: string }) { + if ( + shouldRethrowAsFatal({ + fatalIfFirstPaintFails: this.props.fatalIfFirstPaintFails === true, + childCommitted: this.childCommitted, + }) + ) { + // Rethrowing inside the error phase would re-enter the boundary; the + // macrotask throw reaches the global handler (dev redbox in dev, + // expo-updates ErrorRecovery fatal + rollback in store builds). Not + // recorded here: ErrorRecovery logs the fatal itself, and Diagnostics + // already surfaces that log — recording it too would double-report. + setTimeout(() => { + throw error; + }, 0); + return; + } recordRenderError(error, this.props.scope, { componentStack: info.componentStack }); // Keep the component path for the recovery view's "Copy details" too — // in release builds it may be the only component stack anyone ever sees. @@ -87,6 +111,17 @@ export class RenderErrorBoundary extends Component< } } + // A healthy commit of the guarded subtree ends the "first paint" window. + private childCommitted = false; + + override componentDidMount() { + if (!this.state.failed) this.childCommitted = true; + } + + override componentDidUpdate() { + if (!this.state.failed) this.childCommitted = true; + } + private readonly retry = () => { this.setState(healthyBoundaryState(this.state.resetKeys)); }; diff --git a/apps/mobile/src/components/render-error-boundary-model.test.ts b/apps/mobile/src/components/render-error-boundary-model.test.ts index f9722d9c385e..fc24d0614ebc 100644 --- a/apps/mobile/src/components/render-error-boundary-model.test.ts +++ b/apps/mobile/src/components/render-error-boundary-model.test.ts @@ -6,6 +6,8 @@ import { healthyBoundaryState, inspectorResetKeys, screenFallbackExit, + shouldRethrowAsFatal, + threadFeedResetKeys, workspaceInspectorContentIdentity, } from "./render-error-boundary-model"; @@ -151,6 +153,74 @@ describe("inspectorResetKeys", () => { inspectorResetKeys(workspaceInspectorContentIdentity({ ...base, source: "files" }), render), ).not.toEqual(crashed); }); + it("cannot collide on delimiter-joined parts", () => { + const render = () => null; + const a = workspaceInspectorContentIdentity({ + source: "files", + workspaceKey: "env1|t1", + cwd: "/repo:a", + contentId: "b", + }); + const b = workspaceInspectorContentIdentity({ + source: "files", + workspaceKey: "env1|t1", + cwd: "/repo", + contentId: "a:b", + }); + expect(a).not.toBe(b); + // Absent and the literal string "none" are different facts. + expect( + workspaceInspectorContentIdentity({ + source: "review", + workspaceKey: "w", + cwd: null, + contentId: null, + }), + ).not.toBe( + workspaceInspectorContentIdentity({ + source: "review", + workspaceKey: "w", + cwd: null, + contentId: "none", + }), + ); + expect(inspectorResetKeys(a, render)).not.toEqual(inspectorResetKeys(b, render)); + }); +}); + +describe("threadFeedResetKeys", () => { + it("treats a same-thread worktree move as new content", () => { + expect(threadFeedResetKeys("env1|t1", "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/wt/a")).not.toEqual( + threadFeedResetKeys("env1|t1", "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/wt/b"), + ); + expect(threadFeedResetKeys("env1|t1", "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/wt/a")).toEqual( + threadFeedResetKeys("env1|t1", "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/wt/a"), + ); + expect(threadFeedResetKeys("env1|t1", null)).toEqual(threadFeedResetKeys("env1|t1", undefined)); + }); +}); + +describe("shouldRethrowAsFatal", () => { + it("keeps a never-painted Home crash fatal so OTA rollback can run", () => { + // Bad-OTA cold launch: Home throws before ever committing healthy + // children — that is exactly the failure expo-updates' rollback exists + // for, so it must not be swallowed into a fallback. + expect(shouldRethrowAsFatal({ fatalIfFirstPaintFails: true, childCommitted: false })).toBe( + true, + ); + }); + + it("recovers in-session failures after the first successful paint", () => { + expect(shouldRethrowAsFatal({ fatalIfFirstPaintFails: true, childCommitted: true })).toBe( + false, + ); + }); + + it("never rethrows for screens without the cold-launch valve", () => { + expect(shouldRethrowAsFatal({ fatalIfFirstPaintFails: false, childCommitted: false })).toBe( + false, + ); + }); }); describe("screenFallbackExit", () => { diff --git a/apps/mobile/src/components/render-error-boundary-model.ts b/apps/mobile/src/components/render-error-boundary-model.ts index c4966e01cb79..b7266173cd10 100644 --- a/apps/mobile/src/components/render-error-boundary-model.ts +++ b/apps/mobile/src/components/render-error-boundary-model.ts @@ -43,6 +43,37 @@ export function boundaryResetFromProps( return null; } +/** + * Reset keys for the thread feed boundary. The feed renders entries AND the + * worktree setup card for a cwd, so a same-thread worktree move (cwd change + * under a stable thread key) is new content and must clear a stale failure. + */ +export function threadFeedResetKeys( + threadKey: string, + cwd: string | null | undefined, +): ReadonlyArray { + return [threadKey, cwd ?? null]; +} + +/** + * Cold-launch safety valve for the Home seam. A bad OTA that crashes Home + * before it has ever painted would otherwise strand the user on the fallback + * forever: the launch update check runs inside HomeRouteScreen's effect + * (which never ran) and expo-updates' ErrorRecovery rollback only fires for + * fatals. So a Home boundary that has never committed healthy children + * rethrows — restoring the pre-boundary fatal path (ErrorRecovery rollback + + * its startup crash log) — while any failure after the first successful + * paint keeps in-session recovery. A fatal is deliberately NOT recorded in + * the render-error log: ErrorRecovery already logs it and Diagnostics reads + * that log, and recording both would double-report the same crash. + */ +export function shouldRethrowAsFatal(args: { + readonly fatalIfFirstPaintFails: boolean; + readonly childCommitted: boolean; +}): boolean { + return args.fatalIfFirstPaintFails && !args.childCommitted; +} + /** * Which exit the screen-level fallback offers: normally Go back, but when the * crashing route is the only route (cold launch on Home), there is no previous @@ -98,5 +129,14 @@ export function workspaceInspectorContentIdentity(args: { readonly cwd: string | null | undefined; readonly contentId: string | null | undefined; }): string { - return `${args.source}:${args.workspaceKey ?? "none"}:${args.cwd ?? "none"}:${args.contentId ?? "none"}`; + // JSON tuple, not ':'-joined: paths and ids contain ':' and users can + // legitimately have a content id of "none", so delimiter joining (and + // null-mapping to "none") can collide — and a collision re-arms the + // failed-over-inspector bug this identity exists to prevent. + return JSON.stringify([ + args.source, + args.workspaceKey ?? null, + args.cwd ?? null, + args.contentId ?? null, + ]); } diff --git a/apps/mobile/src/features/threads/ThreadDetailScreen.tsx b/apps/mobile/src/features/threads/ThreadDetailScreen.tsx index c2e880fe697d..01757977b790 100644 --- a/apps/mobile/src/features/threads/ThreadDetailScreen.tsx +++ b/apps/mobile/src/features/threads/ThreadDetailScreen.tsx @@ -73,6 +73,7 @@ import type { DraftComposerAttachment } from "../../lib/composerImages"; import { CHAT_CONTENT_MAX_WIDTH, type LayoutVariant } from "../../lib/layout"; import { IOS_NAV_BAR_HEIGHT } from "../../lib/layoutMetrics"; import { RenderErrorBoundary } from "../../components/RenderErrorBoundary"; +import { threadFeedResetKeys } from "../../components/render-error-boundary-model"; import { editPendingThreadMessage } from "../../state/edit-pending-thread-message"; import { deviceEnvironment } from "../../state/device"; import { useEnvironmentQuery } from "../../state/query"; @@ -899,7 +900,7 @@ export const ThreadDetailScreen = memo(function ThreadDetailScreen(props: Thread Date: Tue, 22 Sep 2026 17:13:23 -0700 Subject: [PATCH 13/17] fix(mobile): throw the cold-launch Home failure inside the failed render pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The macrotask rethrow let React commit the fallback first: that paints a frame, expo-updates records first content / launch success, and the cached- update rollback is dropped before the delayed fatal ever runs — so a bad OTA's Home crash could persist. Rethrowing from the boundary's own render while the fatal-first-paint policy applies means nothing above the seam catches it, React discards the whole in-progress commit, and no frame is shown. That is the same unwinding a boundaryless render throw took before this PR existed, so ErrorRecovery's startup failure path (rollback + its crash log) is untouched; componentDidCatch never runs for the discarded pass, so nothing double-reports. --- .../src/components/RenderErrorBoundary.tsx | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/apps/mobile/src/components/RenderErrorBoundary.tsx b/apps/mobile/src/components/RenderErrorBoundary.tsx index a3d278ca4710..7f5fbf17968e 100644 --- a/apps/mobile/src/components/RenderErrorBoundary.tsx +++ b/apps/mobile/src/components/RenderErrorBoundary.tsx @@ -87,22 +87,6 @@ export class RenderErrorBoundary extends Component< } override componentDidCatch(error: unknown, info: { componentStack?: string }) { - if ( - shouldRethrowAsFatal({ - fatalIfFirstPaintFails: this.props.fatalIfFirstPaintFails === true, - childCommitted: this.childCommitted, - }) - ) { - // Rethrowing inside the error phase would re-enter the boundary; the - // macrotask throw reaches the global handler (dev redbox in dev, - // expo-updates ErrorRecovery fatal + rollback in store builds). Not - // recorded here: ErrorRecovery logs the fatal itself, and Diagnostics - // already surfaces that log — recording it too would double-report. - setTimeout(() => { - throw error; - }, 0); - return; - } recordRenderError(error, this.props.scope, { componentStack: info.componentStack }); // Keep the component path for the recovery view's "Copy details" too — // in release builds it may be the only component stack anyone ever sees. @@ -128,6 +112,24 @@ export class RenderErrorBoundary extends Component< override render() { if (this.state.failed) { + if ( + shouldRethrowAsFatal({ + fatalIfFirstPaintFails: this.props.fatalIfFirstPaintFails === true, + childCommitted: this.childCommitted, + }) + ) { + // Cold-launch Home failure: rethrow inside this failed render pass. + // Nothing above the seam catches, so React unwinds the whole + // in-progress commit and nothing ever paints — no fallback frame, no + // first-content signal for expo-updates to treat the launch as + // successful. This is deliberately the exact same code path a render + // throw took before boundaries existed, so ErrorRecovery's startup + // failure handling (cached-update rollback + its crash log) behaves + // identically to pre-PR. componentDidCatch never runs for this pass + // (the commit is discarded), so the fatal is not also recorded in the + // render-error log — Diagnostics reads the ErrorRecovery log instead. + throw this.state.error; + } if (this.props.fallback) { const Fallback = this.props.fallback; return ( From cb480a0f54748c8bf1dc0dd624c5b400b808529e Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 17:22:19 -0700 Subject: [PATCH 14/17] docs(mobile): state the expo-updates startup-error pipeline exactly Verified against expo-updates 57.0.19 source: ErrorRecoveryHandler runs wait-for-remote-update -> launch new update -> relaunch cached older update -> crash, and CONTENT_APPEARED (Android ReactRootView.onViewAdded, the first root view render; ExpoUpdatesKit is symmetric) removes the two recovery tasks at that point. So a render throw that discards the first commit keeps the whole pipeline available, while any post-first-paint crash never had cached fallback even before this PR. Comments now say the pipeline, not 'rollback', and note expo's existing successful-launch-count rule. --- .../src/components/RenderErrorBoundary.tsx | 23 +++++++++++-------- .../components/render-error-boundary-model.ts | 17 +++++++++----- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/apps/mobile/src/components/RenderErrorBoundary.tsx b/apps/mobile/src/components/RenderErrorBoundary.tsx index 7f5fbf17968e..b7cfd9959d9c 100644 --- a/apps/mobile/src/components/RenderErrorBoundary.tsx +++ b/apps/mobile/src/components/RenderErrorBoundary.tsx @@ -29,8 +29,9 @@ interface RenderErrorBoundaryProps { /** * Cold-launch safety valve (Home seam only): a failure before the guarded * subtree has ever committed rethrows to the global handler so expo-updates' - * ErrorRecovery rollback and its startup crash log behave exactly as before - * boundaries existed. See shouldRethrowAsFatal. + * ErrorRecovery's startup error pipeline (wait briefly for a remote fix, + * else fall back to the cached older update, else crash) stays available + * exactly as before boundaries existed. See shouldRethrowAsFatal. */ readonly fatalIfFirstPaintFails?: boolean | undefined; /** Forwarded to a custom `fallback` so it can adapt per route (screen seam). */ @@ -120,14 +121,16 @@ export class RenderErrorBoundary extends Component< ) { // Cold-launch Home failure: rethrow inside this failed render pass. // Nothing above the seam catches, so React unwinds the whole - // in-progress commit and nothing ever paints — no fallback frame, no - // first-content signal for expo-updates to treat the launch as - // successful. This is deliberately the exact same code path a render - // throw took before boundaries existed, so ErrorRecovery's startup - // failure handling (cached-update rollback + its crash log) behaves - // identically to pre-PR. componentDidCatch never runs for this pass - // (the commit is discarded), so the fatal is not also recorded in the - // render-error log — Diagnostics reads the ErrorRecovery log instead. + // in-progress commit and nothing ever paints. That matters because + // expo-updates 57 disarms its recovery tasks at RN's CONTENT_APPEARED + // marker (Android ReactRootView.onViewAdded — the first root view + // render; ExpoUpdatesKit is symmetric), and a discarded commit never + // reaches it. So the full startup error pipeline (brief wait for a + // remote fix, else cached-older-update fallback, else crash) runs, + // exactly as for any pre-boundary render throw. componentDidCatch + // never runs for this pass (the commit is discarded), so the fatal is + // not also recorded in the render-error log — Diagnostics reads the + // ErrorRecovery log instead. throw this.state.error; } if (this.props.fallback) { diff --git a/apps/mobile/src/components/render-error-boundary-model.ts b/apps/mobile/src/components/render-error-boundary-model.ts index b7266173cd10..8d6ee498620b 100644 --- a/apps/mobile/src/components/render-error-boundary-model.ts +++ b/apps/mobile/src/components/render-error-boundary-model.ts @@ -59,12 +59,17 @@ export function threadFeedResetKeys( * Cold-launch safety valve for the Home seam. A bad OTA that crashes Home * before it has ever painted would otherwise strand the user on the fallback * forever: the launch update check runs inside HomeRouteScreen's effect - * (which never ran) and expo-updates' ErrorRecovery rollback only fires for - * fatals. So a Home boundary that has never committed healthy children - * rethrows — restoring the pre-boundary fatal path (ErrorRecovery rollback + - * its startup crash log) — while any failure after the first successful - * paint keeps in-session recovery. A fatal is deliberately NOT recorded in - * the render-error log: ErrorRecovery already logs it and Diagnostics reads + * (which never ran), and expo-updates 57's startup error pipeline (wait for + * a remote fix, else relaunch the cached older update, else crash — see its + * ErrorRecoveryHandler) removes the two recovery tasks at RN's CONTENT_APPEARED + * marker, the first root view render. A first-paint Home failure therefore + * rethrows from the boundary's own render pass, so React discards the commit, + * content never appears, and the pipeline stays armed exactly as it was + * before boundaries existed. (It also skips cached-older-update fallback for + * an update that has launched successfully before — a pre-existing expo rule, + * unaffected by this PR.) Any failure after the first successful paint keeps + * in-session recovery. A fatal is deliberately NOT recorded in the + * render-error log: ErrorRecovery already logs it and Diagnostics reads * that log, and recording both would double-report the same crash. */ export function shouldRethrowAsFatal(args: { From fd62e5edff976f1cb2e0bfc931a556f43c142a45 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 17:29:15 -0700 Subject: [PATCH 15/17] fix(mobile): disarm the first-paint valve once any frame has painted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit expo-updates disarms its OTA recovery tasks at RN's first-native-view marker, which rides the first commit ANYWHERE in the root tree — providers above the navigation seam mount native views too. If a shell frame ever paints before Home's first commit, a Home crash afterwards is already 'after content appeared' for expo: a rethrow there would be a plain crash with the recovery tasks removed — strictly worse than the fallback. So the valve now also requires that the app has never completed a commit, latched by a sentinel layout effect that flips inside that very first commit and therefore stays false exactly when a render-phase throw discards it. In the all-at-once launch (the pre-PR world for bad OTAs) the valve fires and the startup-error pipeline runs; in an early-shell world it self-disarms and the fallback wins, matching the fact that cached-update fallback was never available in that world even on main. --- apps/mobile/src/App.tsx | 19 +++++++- apps/mobile/src/Stack.tsx | 11 +++-- .../src/components/RenderErrorBoundary.tsx | 2 + .../mobile/src/components/app-first-commit.ts | 27 ++++++++++++ .../render-error-boundary-model.test.ts | 44 +++++++++++++++---- .../components/render-error-boundary-model.ts | 10 ++++- 6 files changed, 98 insertions(+), 15 deletions(-) create mode 100644 apps/mobile/src/components/app-first-commit.ts diff --git a/apps/mobile/src/App.tsx b/apps/mobile/src/App.tsx index 34a742f8f11b..1bc9c7e74636 100644 --- a/apps/mobile/src/App.tsx +++ b/apps/mobile/src/App.tsx @@ -1,6 +1,6 @@ import * as Linking from "expo-linking"; import * as SplashScreen from "expo-splash-screen"; -import { useEffect } from "react"; +import { useEffect, useLayoutEffect } from "react"; import { StatusBar, View } from "react-native"; import { GestureHandlerRootView } from "react-native-gesture-handler"; import { KeyboardProvider } from "react-native-keyboard-controller"; @@ -20,6 +20,7 @@ import { import { RootStack } from "./Stack"; import { appAtomRegistry } from "./state/atom-registry"; import { OverlayPortalHost } from "./components/OverlayPortal"; +import { markAppRootCommitted } from "./components/app-first-commit"; import { shouldHandleAppLink } from "./lib/appLinking"; import { useMobileNavigationTheme } from "./lib/useMobileNavigationTheme"; import { SubscriptionUsageCoordinator } from "./widgets/SubscriptionUsageCoordinator"; @@ -53,6 +54,21 @@ function SplashScreenCoordinator() { return null; } +/** + * Records the first completed commit of the app tree. expo-updates disarms + * OTA startup-error recovery at RN's first-native-view marker, which rides + * ANY commit — so the Home boundary's first-paint fatal valve must know + * whether a frame has ever painted anywhere, not just inside Home. A layout + * effect runs inside the commit itself: if a render-phase throw discards the + * first commit, this never fires and the valve stays armed. + */ +function FirstCommitSentinel() { + useLayoutEffect(() => { + markAppRootCommitted(); + }, []); + return null; +} + export default function App() { return ( @@ -71,6 +87,7 @@ function AppContent() { return ( <> + diff --git a/apps/mobile/src/Stack.tsx b/apps/mobile/src/Stack.tsx index 7d1835b10a1d..a7b1c1d0dee4 100644 --- a/apps/mobile/src/Stack.tsx +++ b/apps/mobile/src/Stack.tsx @@ -804,10 +804,13 @@ function GuardedScreenLayout(props: { scope={`screen:${props.route.name}`} routeName={props.route.name} // Cold-launch OTA lockout guard: Home hosts checkForAppUpdateOnLaunch, - // so if Home crashes before ever painting, let it stay fatal (rethrow → - // ErrorRecovery rollback + its startup log) instead of stranding the - // user on a fallback whose update check can never run. Once Home has - // painted, failures recover in-session like every other screen. + // so if Home crashes before ANY frame has ever painted, let it stay + // fatal (rethrow from the failed render pass → expo-updates' full + // startup-error pipeline) instead of stranding the user on a fallback + // whose update check can never run. FirstCommitSentinel in App.tsx + // disarms this once any commit has painted, because expo disarms its + // recovery tasks at that same first-view marker. Any later failure + // recovers in-session like every other screen. fatalIfFirstPaintFails={props.route.name === "Home"} // In split view the Thread route stays mounted while a sidebar selection // swaps its params; new params are new input and must not inherit a diff --git a/apps/mobile/src/components/RenderErrorBoundary.tsx b/apps/mobile/src/components/RenderErrorBoundary.tsx index b7cfd9959d9c..b5da5734ce2c 100644 --- a/apps/mobile/src/components/RenderErrorBoundary.tsx +++ b/apps/mobile/src/components/RenderErrorBoundary.tsx @@ -5,6 +5,7 @@ import { SymbolView } from "./AppSymbol"; import { AppText as Text } from "./AppText"; import { MaterialButton } from "./MaterialButton"; import { tryCopyTextWithHaptic } from "../lib/copyTextWithHaptic"; +import { hasAppRootCommitted } from "./app-first-commit"; import { describeRenderError, readErrorStack, @@ -117,6 +118,7 @@ export class RenderErrorBoundary extends Component< shouldRethrowAsFatal({ fatalIfFirstPaintFails: this.props.fatalIfFirstPaintFails === true, childCommitted: this.childCommitted, + appRootCommitted: hasAppRootCommitted(), }) ) { // Cold-launch Home failure: rethrow inside this failed render pass. diff --git a/apps/mobile/src/components/app-first-commit.ts b/apps/mobile/src/components/app-first-commit.ts new file mode 100644 index 000000000000..16e6d8a2ed99 --- /dev/null +++ b/apps/mobile/src/components/app-first-commit.ts @@ -0,0 +1,27 @@ +/** + * Whether the app's root tree has ever completed a React commit. + * + * Why this exists: expo-updates 57 disarms its OTA startup-error recovery at + * RN's CONTENT_APPEARED marker, which fires when the first native view is + * added to the root view — i.e. when ANY commit has painted, not when the + * Home screen specifically has. The Home first-paint fatal valve is only + * equivalent to the pre-boundary behavior while nothing has painted; once + * any frame exists, a rethrow is just a crash with no recovery tasks left, + * so the valve must disarm itself. This flag flips false→true exactly once, + * inside the first successful commit (the sentinel's layout effect is part + * of that commit), which is the same commit CONTENT_APPEARED rides on. + */ +let appRootCommitted = false; + +export function markAppRootCommitted(): void { + appRootCommitted = true; +} + +export function hasAppRootCommitted(): boolean { + return appRootCommitted; +} + +/** Test-only: reset the latch between cases. */ +export function resetAppRootCommitted(): void { + appRootCommitted = false; +} diff --git a/apps/mobile/src/components/render-error-boundary-model.test.ts b/apps/mobile/src/components/render-error-boundary-model.test.ts index fc24d0614ebc..2b2d98d5ecc9 100644 --- a/apps/mobile/src/components/render-error-boundary-model.test.ts +++ b/apps/mobile/src/components/render-error-boundary-model.test.ts @@ -205,21 +205,47 @@ describe("shouldRethrowAsFatal", () => { // Bad-OTA cold launch: Home throws before ever committing healthy // children — that is exactly the failure expo-updates' rollback exists // for, so it must not be swallowed into a fallback. - expect(shouldRethrowAsFatal({ fatalIfFirstPaintFails: true, childCommitted: false })).toBe( - true, - ); + expect( + shouldRethrowAsFatal({ + fatalIfFirstPaintFails: true, + childCommitted: false, + appRootCommitted: false, + }), + ).toBe(true); }); it("recovers in-session failures after the first successful paint", () => { - expect(shouldRethrowAsFatal({ fatalIfFirstPaintFails: true, childCommitted: true })).toBe( - false, - ); + expect( + shouldRethrowAsFatal({ + fatalIfFirstPaintFails: true, + childCommitted: true, + appRootCommitted: true, + }), + ).toBe(false); + }); + + it("disarms once any frame has painted elsewhere in the root tree", () => { + // CONTENT_APPEARED rides the first commit ANYWHERE (providers above the + // seam mount native views too). After that a rethrow is a plain crash + // with expo's recovery tasks already removed — strictly worse than the + // fallback — so a Home-specific non-commit must not re-fire the valve. + expect( + shouldRethrowAsFatal({ + fatalIfFirstPaintFails: true, + childCommitted: false, + appRootCommitted: true, + }), + ).toBe(false); }); it("never rethrows for screens without the cold-launch valve", () => { - expect(shouldRethrowAsFatal({ fatalIfFirstPaintFails: false, childCommitted: false })).toBe( - false, - ); + expect( + shouldRethrowAsFatal({ + fatalIfFirstPaintFails: false, + childCommitted: false, + appRootCommitted: false, + }), + ).toBe(false); }); }); diff --git a/apps/mobile/src/components/render-error-boundary-model.ts b/apps/mobile/src/components/render-error-boundary-model.ts index 8d6ee498620b..e5e4059cdd8d 100644 --- a/apps/mobile/src/components/render-error-boundary-model.ts +++ b/apps/mobile/src/components/render-error-boundary-model.ts @@ -71,12 +71,20 @@ export function threadFeedResetKeys( * in-session recovery. A fatal is deliberately NOT recorded in the * render-error log: ErrorRecovery already logs it and Diagnostics reads * that log, and recording both would double-report the same crash. + * + * `appRootCommitted` closes a timing hole: CONTENT_APPEARED rides the first + * commit ANYWHERE in the root tree (providers above the seam mount native + * views too), not the first Home commit. If any frame has already painted, + * a rethrow would arrive after expo disarmed its recovery tasks — strictly + * worse than the fallback — so the valve only fires while nothing has ever + * committed. */ export function shouldRethrowAsFatal(args: { readonly fatalIfFirstPaintFails: boolean; readonly childCommitted: boolean; + readonly appRootCommitted: boolean; }): boolean { - return args.fatalIfFirstPaintFails && !args.childCommitted; + return args.fatalIfFirstPaintFails && !args.childCommitted && !args.appRootCommitted; } /** From 796bbd7fc433d36b2785ac6577753663a5f045bc Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 17:41:09 -0700 Subject: [PATCH 16/17] =?UTF-8?q?refactor(mobile):=20drop=20the=20Home=20f?= =?UTF-8?q?irst-paint=20fatal=20valve=20=E2=80=94=20it=20could=20never=20f?= =?UTF-8?q?ire?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified in the installed sources: @react-navigation/native 7.3.4 NavigationContainer renders only its (null) fallback until the async linking getInitialState thenable resolves, while the providers above it already commit native views. So the first frame — RN's first-native-view marker, which is also what disarms expo-updates 57's startup-error recovery tasks — always precedes Home's first render, on every launch, pre-PR included. A cached-older-update fallback was therefore never available for a Home render crash even on main (expo logged the crash and waited for a remote update), and a valve that rethrows only before the first frame can never fire in the real app. Keeping it would only trade a working fallback for a crash to match main's already-poor behavior. Home cold-launch render crashes now take the same path as every other screen: fallback with Open settings -> Diagnostics. OTA recovery is unchanged from main and handled by expo's native checkAutomatically ON_LOAD (next launch downloads and applies the fix), independent of HomeRouteScreen's effect. Removes fatalIfFirstPaintFails, FirstCommitSentinel, app-first-commit, childCommitted tracking, shouldRethrowAsFatal, and their tests; valve-unit tests asserted flags, not launch order, and missed exactly this. --- apps/mobile/src/App.tsx | 19 +------ apps/mobile/src/Stack.tsx | 9 ---- .../src/components/RenderErrorBoundary.tsx | 42 ---------------- .../mobile/src/components/app-first-commit.ts | 27 ---------- .../render-error-boundary-model.test.ts | 50 ------------------- .../components/render-error-boundary-model.ts | 32 ------------ 6 files changed, 1 insertion(+), 178 deletions(-) delete mode 100644 apps/mobile/src/components/app-first-commit.ts diff --git a/apps/mobile/src/App.tsx b/apps/mobile/src/App.tsx index 1bc9c7e74636..34a742f8f11b 100644 --- a/apps/mobile/src/App.tsx +++ b/apps/mobile/src/App.tsx @@ -1,6 +1,6 @@ import * as Linking from "expo-linking"; import * as SplashScreen from "expo-splash-screen"; -import { useEffect, useLayoutEffect } from "react"; +import { useEffect } from "react"; import { StatusBar, View } from "react-native"; import { GestureHandlerRootView } from "react-native-gesture-handler"; import { KeyboardProvider } from "react-native-keyboard-controller"; @@ -20,7 +20,6 @@ import { import { RootStack } from "./Stack"; import { appAtomRegistry } from "./state/atom-registry"; import { OverlayPortalHost } from "./components/OverlayPortal"; -import { markAppRootCommitted } from "./components/app-first-commit"; import { shouldHandleAppLink } from "./lib/appLinking"; import { useMobileNavigationTheme } from "./lib/useMobileNavigationTheme"; import { SubscriptionUsageCoordinator } from "./widgets/SubscriptionUsageCoordinator"; @@ -54,21 +53,6 @@ function SplashScreenCoordinator() { return null; } -/** - * Records the first completed commit of the app tree. expo-updates disarms - * OTA startup-error recovery at RN's first-native-view marker, which rides - * ANY commit — so the Home boundary's first-paint fatal valve must know - * whether a frame has ever painted anywhere, not just inside Home. A layout - * effect runs inside the commit itself: if a render-phase throw discards the - * first commit, this never fires and the valve stays armed. - */ -function FirstCommitSentinel() { - useLayoutEffect(() => { - markAppRootCommitted(); - }, []); - return null; -} - export default function App() { return ( @@ -87,7 +71,6 @@ function AppContent() { return ( <> - diff --git a/apps/mobile/src/Stack.tsx b/apps/mobile/src/Stack.tsx index a7b1c1d0dee4..1aacc8fbecf8 100644 --- a/apps/mobile/src/Stack.tsx +++ b/apps/mobile/src/Stack.tsx @@ -803,15 +803,6 @@ function GuardedScreenLayout(props: { | undefined; /** Subject noun for the default fallback's headline, e.g. "The conversation". */ readonly subject?: string; - /** - * Cold-launch safety valve (Home seam only): a failure before the guarded - * subtree has ever committed rethrows to the global handler so expo-updates' - * ErrorRecovery's startup error pipeline (wait briefly for a remote fix, - * else fall back to the cached older update, else crash) stays available - * exactly as before boundaries existed. See shouldRethrowAsFatal. - */ - readonly fatalIfFirstPaintFails?: boolean | undefined; /** Forwarded to a custom `fallback` so it can adapt per route (screen seam). */ readonly routeName?: string | undefined; /** @@ -97,44 +87,12 @@ export class RenderErrorBoundary extends Component< } } - // A healthy commit of the guarded subtree ends the "first paint" window. - private childCommitted = false; - - override componentDidMount() { - if (!this.state.failed) this.childCommitted = true; - } - - override componentDidUpdate() { - if (!this.state.failed) this.childCommitted = true; - } - private readonly retry = () => { this.setState(healthyBoundaryState(this.state.resetKeys)); }; override render() { if (this.state.failed) { - if ( - shouldRethrowAsFatal({ - fatalIfFirstPaintFails: this.props.fatalIfFirstPaintFails === true, - childCommitted: this.childCommitted, - appRootCommitted: hasAppRootCommitted(), - }) - ) { - // Cold-launch Home failure: rethrow inside this failed render pass. - // Nothing above the seam catches, so React unwinds the whole - // in-progress commit and nothing ever paints. That matters because - // expo-updates 57 disarms its recovery tasks at RN's CONTENT_APPEARED - // marker (Android ReactRootView.onViewAdded — the first root view - // render; ExpoUpdatesKit is symmetric), and a discarded commit never - // reaches it. So the full startup error pipeline (brief wait for a - // remote fix, else cached-older-update fallback, else crash) runs, - // exactly as for any pre-boundary render throw. componentDidCatch - // never runs for this pass (the commit is discarded), so the fatal is - // not also recorded in the render-error log — Diagnostics reads the - // ErrorRecovery log instead. - throw this.state.error; - } if (this.props.fallback) { const Fallback = this.props.fallback; return ( diff --git a/apps/mobile/src/components/app-first-commit.ts b/apps/mobile/src/components/app-first-commit.ts deleted file mode 100644 index 16e6d8a2ed99..000000000000 --- a/apps/mobile/src/components/app-first-commit.ts +++ /dev/null @@ -1,27 +0,0 @@ -/** - * Whether the app's root tree has ever completed a React commit. - * - * Why this exists: expo-updates 57 disarms its OTA startup-error recovery at - * RN's CONTENT_APPEARED marker, which fires when the first native view is - * added to the root view — i.e. when ANY commit has painted, not when the - * Home screen specifically has. The Home first-paint fatal valve is only - * equivalent to the pre-boundary behavior while nothing has painted; once - * any frame exists, a rethrow is just a crash with no recovery tasks left, - * so the valve must disarm itself. This flag flips false→true exactly once, - * inside the first successful commit (the sentinel's layout effect is part - * of that commit), which is the same commit CONTENT_APPEARED rides on. - */ -let appRootCommitted = false; - -export function markAppRootCommitted(): void { - appRootCommitted = true; -} - -export function hasAppRootCommitted(): boolean { - return appRootCommitted; -} - -/** Test-only: reset the latch between cases. */ -export function resetAppRootCommitted(): void { - appRootCommitted = false; -} diff --git a/apps/mobile/src/components/render-error-boundary-model.test.ts b/apps/mobile/src/components/render-error-boundary-model.test.ts index 2b2d98d5ecc9..d75746b9e458 100644 --- a/apps/mobile/src/components/render-error-boundary-model.test.ts +++ b/apps/mobile/src/components/render-error-boundary-model.test.ts @@ -6,7 +6,6 @@ import { healthyBoundaryState, inspectorResetKeys, screenFallbackExit, - shouldRethrowAsFatal, threadFeedResetKeys, workspaceInspectorContentIdentity, } from "./render-error-boundary-model"; @@ -200,55 +199,6 @@ describe("threadFeedResetKeys", () => { }); }); -describe("shouldRethrowAsFatal", () => { - it("keeps a never-painted Home crash fatal so OTA rollback can run", () => { - // Bad-OTA cold launch: Home throws before ever committing healthy - // children — that is exactly the failure expo-updates' rollback exists - // for, so it must not be swallowed into a fallback. - expect( - shouldRethrowAsFatal({ - fatalIfFirstPaintFails: true, - childCommitted: false, - appRootCommitted: false, - }), - ).toBe(true); - }); - - it("recovers in-session failures after the first successful paint", () => { - expect( - shouldRethrowAsFatal({ - fatalIfFirstPaintFails: true, - childCommitted: true, - appRootCommitted: true, - }), - ).toBe(false); - }); - - it("disarms once any frame has painted elsewhere in the root tree", () => { - // CONTENT_APPEARED rides the first commit ANYWHERE (providers above the - // seam mount native views too). After that a rethrow is a plain crash - // with expo's recovery tasks already removed — strictly worse than the - // fallback — so a Home-specific non-commit must not re-fire the valve. - expect( - shouldRethrowAsFatal({ - fatalIfFirstPaintFails: true, - childCommitted: false, - appRootCommitted: true, - }), - ).toBe(false); - }); - - it("never rethrows for screens without the cold-launch valve", () => { - expect( - shouldRethrowAsFatal({ - fatalIfFirstPaintFails: false, - childCommitted: false, - appRootCommitted: false, - }), - ).toBe(false); - }); -}); - describe("screenFallbackExit", () => { it("offers Go back when a previous route exists", () => { expect(screenFallbackExit({ canGoBack: true, routeName: "SettingsSheet" })).toBe("go-back"); diff --git a/apps/mobile/src/components/render-error-boundary-model.ts b/apps/mobile/src/components/render-error-boundary-model.ts index e5e4059cdd8d..0957f3b0e713 100644 --- a/apps/mobile/src/components/render-error-boundary-model.ts +++ b/apps/mobile/src/components/render-error-boundary-model.ts @@ -55,38 +55,6 @@ export function threadFeedResetKeys( return [threadKey, cwd ?? null]; } -/** - * Cold-launch safety valve for the Home seam. A bad OTA that crashes Home - * before it has ever painted would otherwise strand the user on the fallback - * forever: the launch update check runs inside HomeRouteScreen's effect - * (which never ran), and expo-updates 57's startup error pipeline (wait for - * a remote fix, else relaunch the cached older update, else crash — see its - * ErrorRecoveryHandler) removes the two recovery tasks at RN's CONTENT_APPEARED - * marker, the first root view render. A first-paint Home failure therefore - * rethrows from the boundary's own render pass, so React discards the commit, - * content never appears, and the pipeline stays armed exactly as it was - * before boundaries existed. (It also skips cached-older-update fallback for - * an update that has launched successfully before — a pre-existing expo rule, - * unaffected by this PR.) Any failure after the first successful paint keeps - * in-session recovery. A fatal is deliberately NOT recorded in the - * render-error log: ErrorRecovery already logs it and Diagnostics reads - * that log, and recording both would double-report the same crash. - * - * `appRootCommitted` closes a timing hole: CONTENT_APPEARED rides the first - * commit ANYWHERE in the root tree (providers above the seam mount native - * views too), not the first Home commit. If any frame has already painted, - * a rethrow would arrive after expo disarmed its recovery tasks — strictly - * worse than the fallback — so the valve only fires while nothing has ever - * committed. - */ -export function shouldRethrowAsFatal(args: { - readonly fatalIfFirstPaintFails: boolean; - readonly childCommitted: boolean; - readonly appRootCommitted: boolean; -}): boolean { - return args.fatalIfFirstPaintFails && !args.childCommitted && !args.appRootCommitted; -} - /** * Which exit the screen-level fallback offers: normally Go back, but when the * crashing route is the only route (cold launch on Home), there is no previous From f213db037673cf264fe594256848a6d87fd0ed85 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Tue, 22 Sep 2026 17:46:50 -0700 Subject: [PATCH 17/17] refactor(mobile): move the render-error log from features/diagnostics to lib MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Main's dependency-graph guard ceilings components->features imports at 33 and this PR's RenderErrorBoundary -> features/diagnostics/render-error-log was a new upward edge (34) — the one P1 in CI. The log is generic session infrastructure, not a feature: it lives in lib/ now, so the merge against main nets zero new upward edges, and the Diagnostics screen keeps reading it downward like any other feature. --- apps/mobile/src/components/RenderErrorBoundary.tsx | 6 +----- .../features/diagnostics/SettingsDiagnosticsRouteScreen.tsx | 2 +- .../{features/diagnostics => lib}/render-error-log.test.ts | 0 .../src/{features/diagnostics => lib}/render-error-log.ts | 0 4 files changed, 2 insertions(+), 6 deletions(-) rename apps/mobile/src/{features/diagnostics => lib}/render-error-log.test.ts (100%) rename apps/mobile/src/{features/diagnostics => lib}/render-error-log.ts (100%) diff --git a/apps/mobile/src/components/RenderErrorBoundary.tsx b/apps/mobile/src/components/RenderErrorBoundary.tsx index 40bcaecc7a84..fc2f6cb838f7 100644 --- a/apps/mobile/src/components/RenderErrorBoundary.tsx +++ b/apps/mobile/src/components/RenderErrorBoundary.tsx @@ -5,11 +5,7 @@ import { SymbolView } from "./AppSymbol"; import { AppText as Text } from "./AppText"; import { MaterialButton } from "./MaterialButton"; import { tryCopyTextWithHaptic } from "../lib/copyTextWithHaptic"; -import { - describeRenderError, - readErrorStack, - recordRenderError, -} from "../features/diagnostics/render-error-log"; +import { describeRenderError, readErrorStack, recordRenderError } from "../lib/render-error-log"; import { boundaryResetFromProps, failedBoundaryState, diff --git a/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx b/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx index 8ebf10f18bec..adc8a3bbe375 100644 --- a/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx +++ b/apps/mobile/src/features/diagnostics/SettingsDiagnosticsRouteScreen.tsx @@ -21,7 +21,7 @@ import { getRenderErrorRecords, subscribeToRenderErrors, type RenderErrorRecord, -} from "./render-error-log"; +} from "../../lib/render-error-log"; // expo-updates keeps its persistent log this long. Reading any further back // returns nothing, so this is the whole available window. diff --git a/apps/mobile/src/features/diagnostics/render-error-log.test.ts b/apps/mobile/src/lib/render-error-log.test.ts similarity index 100% rename from apps/mobile/src/features/diagnostics/render-error-log.test.ts rename to apps/mobile/src/lib/render-error-log.test.ts diff --git a/apps/mobile/src/features/diagnostics/render-error-log.ts b/apps/mobile/src/lib/render-error-log.ts similarity index 100% rename from apps/mobile/src/features/diagnostics/render-error-log.ts rename to apps/mobile/src/lib/render-error-log.ts