From cf104df90c35eecdcbaf6e8ce1b85bb3a28e8779 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 26 Aug 2026 16:00:46 -0700 Subject: [PATCH] fix(mobile): harden crash recovery diagnostics --- mobile/app/troubleshoot.tsx | 18 +++++- .../components/MobileRootErrorBoundary.tsx | 15 ++--- .../components/PreviousCrashSessionBanner.tsx | 45 +++++++++++++ .../mobile-root-error-boundary.test.tsx | 11 +++- .../components/troubleshoot-screen.test.tsx | 64 +++++++++++++++++++ .../diagnostics/mobile-crash-diagnostics.ts | 4 ++ .../src/diagnostics/mobile-crash-session.ts | 43 ++----------- .../crash-reporting/crash-breadcrumb-store.ts | 8 +-- src/shared/crash-error-description.test.ts | 27 ++++++++ src/shared/crash-error-description.ts | 41 ++++++++++++ src/shared/crash-reporting.ts | 4 +- 11 files changed, 226 insertions(+), 54 deletions(-) create mode 100644 mobile/src/components/PreviousCrashSessionBanner.tsx create mode 100644 mobile/src/components/troubleshoot-screen.test.tsx create mode 100644 src/shared/crash-error-description.test.ts create mode 100644 src/shared/crash-error-description.ts diff --git a/mobile/app/troubleshoot.tsx b/mobile/app/troubleshoot.tsx index 9d468d697a1..67f7b655844 100644 --- a/mobile/app/troubleshoot.tsx +++ b/mobile/app/troubleshoot.tsx @@ -22,6 +22,7 @@ import { AlertTriangle } from 'lucide-react-native' import { colors, spacing, typography } from '../src/theme/mobile-theme' +import { PreviousCrashSessionBanner } from '../src/components/PreviousCrashSessionBanner' import { loadHosts } from '../src/transport/host-store' import { startDiagnosticFetchTimeout, @@ -33,7 +34,10 @@ import { unreachableHostDetail } from '../src/diagnostics/host-reachability' import { troubleshootCommonIssues } from '../src/diagnostics/troubleshoot-common-issues' -import { buildMobileCrashDiagnosticsReport } from '../src/diagnostics/mobile-crash-diagnostics' +import { + buildMobileCrashDiagnosticsReport, + getPreviousMobileCrashSession +} from '../src/diagnostics/mobile-crash-diagnostics' type DiagnosticStatus = 'idle' | 'running' | 'done' @@ -64,9 +68,19 @@ export default function TroubleshootScreen() { const diagnosticRunRef = useRef(0) const activeInternetCheckRef = useRef(null) const [crashDiagnosticsCopied, setCrashDiagnosticsCopied] = useState(false) + const [hasPreviousCrashSession, setHasPreviousCrashSession] = useState(false) + const crashSessionLoadedRef = useRef(false) const setTroubleshootRootRef = useCallback((node: View | null): void => { if (node !== null) { + if (!crashSessionLoadedRef.current) { + crashSessionLoadedRef.current = true + void getPreviousMobileCrashSession().then((session) => { + if (!abortRef.current) { + setHasPreviousCrashSession(session !== null) + } + }) + } return } // Why: diagnostics can outlive the screen; cancel the active run when the @@ -199,6 +213,8 @@ export default function TroubleshootScreen() { contentContainerStyle={styles.scrollContent} showsVerticalScrollIndicator={false} > + {hasPreviousCrashSession && } + [ styles.diagnosticButton, diff --git a/mobile/src/components/MobileRootErrorBoundary.tsx b/mobile/src/components/MobileRootErrorBoundary.tsx index 175bc1be4f5..32228b034b2 100644 --- a/mobile/src/components/MobileRootErrorBoundary.tsx +++ b/mobile/src/components/MobileRootErrorBoundary.tsx @@ -38,10 +38,8 @@ export class MobileRootErrorBoundary extends Component { } handleReturnHome = (): void => { - this.setState( - ({ resetKey }) => ({ error: null, resetKey: resetKey + 1 }), - this.props.onReturnHome - ) + this.props.onReturnHome() + this.setState(({ resetKey }) => ({ error: null, resetKey: resetKey + 1 })) } handleReport = (): void => { @@ -101,7 +99,7 @@ function MobileRootErrorFallback({ style={styles.secondaryButton} onPress={onReturnHome} > - + Return home - + Report error @@ -181,10 +179,11 @@ const styles = StyleSheet.create({ justifyContent: 'center', gap: spacing.sm, paddingHorizontal: spacing.xl, - borderRadius: radii.button + borderRadius: radii.button, + backgroundColor: colors.bgRaised }, secondaryButtonText: { - color: colors.textSecondary, + color: colors.textPrimary, fontSize: typography.bodySize, fontWeight: '500' } diff --git a/mobile/src/components/PreviousCrashSessionBanner.tsx b/mobile/src/components/PreviousCrashSessionBanner.tsx new file mode 100644 index 00000000000..277d406367c --- /dev/null +++ b/mobile/src/components/PreviousCrashSessionBanner.tsx @@ -0,0 +1,45 @@ +import { AlertTriangle } from 'lucide-react-native' +import { StyleSheet, Text, View } from 'react-native' +import { colors, radii, spacing, typography } from '../theme/mobile-theme' + +export function PreviousCrashSessionBanner() { + return ( + + + + Previous session ended abnormally + + Crash diagnostics are available to copy and share with support. + + + + ) +} + +const styles = StyleSheet.create({ + banner: { + flexDirection: 'row', + alignItems: 'flex-start', + gap: spacing.md, + padding: spacing.md, + marginBottom: spacing.lg, + borderWidth: StyleSheet.hairlineWidth, + borderColor: colors.borderSubtle, + borderRadius: radii.card, + backgroundColor: colors.bgPanel + }, + copy: { + flex: 1, + gap: spacing.xs + }, + title: { + color: colors.textPrimary, + fontSize: typography.bodySize, + fontWeight: '600' + }, + description: { + color: colors.textSecondary, + fontSize: typography.metaSize, + lineHeight: 18 + } +}) diff --git a/mobile/src/components/mobile-root-error-boundary.test.tsx b/mobile/src/components/mobile-root-error-boundary.test.tsx index 594ced32f20..bd2be3f7e77 100644 --- a/mobile/src/components/mobile-root-error-boundary.test.tsx +++ b/mobile/src/components/mobile-root-error-boundary.test.tsx @@ -84,6 +84,7 @@ vi.mock('../diagnostics/mobile-crash-diagnostics', () => ({ })) import RootLayout from '../../app/_layout' +import { colors } from '../theme/mobile-theme' describe('mobile root error boundary', () => { let renderer!: ReactTestRenderer @@ -121,6 +122,10 @@ describe('mobile root error boundary', () => { expect.objectContaining({ message: 'route render exploded' }), expect.any(String) ) + expect(buttons[1]?.props.style).toEqual( + expect.objectContaining({ backgroundColor: colors.bgRaised }) + ) + expect(buttons[1]?.findByType('House').props.color).toBe(colors.textPrimary) act(() => buttons[2]?.props.onPress()) expect(mocks.shareDiagnostics).toHaveBeenCalledOnce() @@ -148,19 +153,19 @@ describe('mobile root error boundary', () => { expect(mocks.recordRoute).toHaveBeenCalledWith(['h', '[hostId]', 'session', '[worktreeId]']) }) - it('returns home after remounting the failed route', () => { + it('navigates home before remounting the failed route', () => { vi.spyOn(console, 'error').mockImplementation(() => undefined) act(() => { renderer = create(createElement(RootLayout)) }) - mocks.routeShouldThrow = false mocks.routerReplace.mockImplementationOnce(() => { - expect(renderer.root.findAllByProps({ testID: 'mobile-root-error-boundary' })).toHaveLength(0) + mocks.routeShouldThrow = false }) const returnHome = renderer.root.findByProps({ accessibilityLabel: 'Return home' }) act(() => returnHome.props.onPress()) expect(mocks.routerReplace).toHaveBeenCalledWith('/') + expect(renderer.root.findAllByProps({ testID: 'mobile-root-error-boundary' })).toHaveLength(0) }) }) diff --git a/mobile/src/components/troubleshoot-screen.test.tsx b/mobile/src/components/troubleshoot-screen.test.tsx new file mode 100644 index 00000000000..0f5d7aeee33 --- /dev/null +++ b/mobile/src/components/troubleshoot-screen.test.tsx @@ -0,0 +1,64 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + getPreviousCrashSession: vi.fn().mockResolvedValue({ + openedAt: '2026-08-24T18:00:00.000Z', + breadcrumbs: [] + }) +})) + +vi.mock('react-native', () => ({ + ActivityIndicator: 'ActivityIndicator', + Platform: { OS: 'ios', Version: '26.5' }, + Pressable: 'Pressable', + ScrollView: 'ScrollView', + StyleSheet: { create: (styles: T) => styles, hairlineWidth: 1 }, + Text: 'Text', + View: 'View' +})) +vi.mock('react-native-safe-area-context', () => ({ useSafeAreaInsets: () => ({ top: 0 }) })) +vi.mock('expo-router', () => ({ useRouter: () => ({ back: vi.fn(), push: vi.fn() }) })) +vi.mock('expo-clipboard', () => ({ setStringAsync: vi.fn() })) +vi.mock('lucide-react-native', () => ({ + Activity: 'Activity', + AlertTriangle: 'AlertTriangle', + CheckCircle2: 'CheckCircle2', + ChevronDown: 'ChevronDown', + ChevronLeft: 'ChevronLeft', + ChevronUp: 'ChevronUp', + ScrollText: 'ScrollText', + XCircle: 'XCircle' +})) +vi.mock('../transport/host-store', () => ({ loadHosts: vi.fn().mockResolvedValue([]) })) +vi.mock('../diagnostics/diagnostic-fetch-timeout', () => ({ + startDiagnosticFetchTimeout: () => ({ signal: undefined, dispose: vi.fn() }) +})) +vi.mock('../diagnostics/host-reachability', () => ({ + formatEndpoint: vi.fn(), + testHostReachability: vi.fn(), + unreachableHostDetail: vi.fn() +})) +vi.mock('../diagnostics/troubleshoot-common-issues', () => ({ + troubleshootCommonIssues: [] +})) +vi.mock('../diagnostics/mobile-crash-diagnostics', () => ({ + buildMobileCrashDiagnosticsReport: vi.fn().mockResolvedValue('report'), + getPreviousMobileCrashSession: mocks.getPreviousCrashSession +})) + +import TroubleshootScreen from '../../app/troubleshoot' + +describe('TroubleshootScreen crash diagnostics', () => { + it('shows when the previous session ended abnormally', async () => { + let renderer!: ReactTestRenderer + await act(async () => { + renderer = create(createElement(TroubleshootScreen), { createNodeMock: () => ({}) }) + await Promise.resolve() + }) + + expect(mocks.getPreviousCrashSession).toHaveBeenCalledOnce() + expect(renderer.root.findByProps({ testID: 'previous-crash-session-banner' })).toBeDefined() + }) +}) diff --git a/mobile/src/diagnostics/mobile-crash-diagnostics.ts b/mobile/src/diagnostics/mobile-crash-diagnostics.ts index 0f145ec25ce..6b097c3f87d 100644 --- a/mobile/src/diagnostics/mobile-crash-diagnostics.ts +++ b/mobile/src/diagnostics/mobile-crash-diagnostics.ts @@ -24,6 +24,10 @@ export function recordMobileRenderError( return journal.recordRenderError(error, componentStack) } +export function getPreviousMobileCrashSession(): Promise { + return journal.getLatestAbnormalSession() +} + export function buildMobileCrashDiagnosticsReport(): Promise { return journal.buildReport({ version: Constants.expoConfig?.version ?? 'unknown', diff --git a/mobile/src/diagnostics/mobile-crash-session.ts b/mobile/src/diagnostics/mobile-crash-session.ts index cf2129a4469..a30bd8535c8 100644 --- a/mobile/src/diagnostics/mobile-crash-session.ts +++ b/mobile/src/diagnostics/mobile-crash-session.ts @@ -4,6 +4,7 @@ import { type CrashReportBreadcrumb, type CrashReportBreadcrumbData } from '../../../src/shared/crash-reporting' +import { describeCrashError } from '../../../src/shared/crash-error-description' import { MAX_MOBILE_CRASH_BREADCRUMBS, MOBILE_CRASH_SESSION_STORAGE_KEY, @@ -28,8 +29,6 @@ type JournalOptions = { createSessionId?: () => string } -const MAX_STACK_CHARS = 2_000 - const SAFE_ROUTE_SEGMENTS = new Set([ 'about', 'accounts', @@ -137,11 +136,16 @@ export class MobileCrashSessionJournal { async recordRenderError(error: unknown, componentStack?: string | null): Promise { await this.start() await this.enqueue(async () => { - this.appendBreadcrumb('render_error_contained', describeError(error, componentStack)) + this.appendBreadcrumb('render_error_contained', describeCrashError(error, componentStack)) await this.persistJournal() }) } + async getLatestAbnormalSession(): Promise { + await this.start() + return this.enqueue(async () => this.journal?.latestAbnormalSession ?? null) + } + async buildReport(app: { version: string; platform: string }): Promise { await this.start() return this.enqueue(async () => { @@ -238,39 +242,6 @@ function normalizeAppState(state: string): string { : 'unknown' } -function describeError(error: unknown, componentStack?: string | null): CrashReportBreadcrumbData { - const candidate = error instanceof Error ? error : null - const message = candidate?.message ?? String(error) - const stack = extractStackFrames(candidate?.stack, message) - return { - errorName: sanitizeCrashReportString(candidate?.name || 'NonErrorThrown', 80), - errorFingerprint: fingerprint(message), - ...(stack ? { errorStack: sanitizeCrashReportString(stack, MAX_STACK_CHARS) } : {}), - ...(componentStack?.trim() - ? { componentStack: sanitizeCrashReportString(componentStack.trim(), MAX_STACK_CHARS) } - : {}) - } -} - -function extractStackFrames(stack: string | undefined, message: string): string | undefined { - const messageLineCount = message.split(/\r?\n/).length - const lines = (stack?.split(/\r?\n/) ?? []).slice(messageLineCount) - const firstFrameIndex = lines.findIndex((line) => /^\s+at\b/.test(line)) - if (firstFrameIndex === -1) { - return undefined - } - return lines.slice(firstFrameIndex).join('\n').trim() || undefined -} - -function fingerprint(value: string): string { - let hash = 0x811c9dc5 - for (let index = 0; index < value.length; index += 1) { - hash ^= value.charCodeAt(index) - hash = Math.imul(hash, 0x01000193) - } - return (hash >>> 0).toString(16).padStart(8, '0') -} - function appendSessionLines(lines: string[], session: MobileCrashSessionSnapshot): void { lines.push(`Opened: ${session.openedAt}`) lines.push(`Breadcrumbs (${session.breadcrumbs.length}, oldest first):`) diff --git a/src/main/crash-reporting/crash-breadcrumb-store.ts b/src/main/crash-reporting/crash-breadcrumb-store.ts index 0a9a71f452e..3272a95fa04 100644 --- a/src/main/crash-reporting/crash-breadcrumb-store.ts +++ b/src/main/crash-reporting/crash-breadcrumb-store.ts @@ -5,7 +5,7 @@ import { type CrashReportBreadcrumb } from '../../shared/crash-reporting' -const MAX_BREADCRUMBS = 30 +const MAX_DESKTOP_CRASH_BREADCRUMBS = 30 // Why: retain two thresholds for each renderer surface without growing the ring. const MAX_RETAINED_BREADCRUMBS = 4 // Why: coalesceKey embeds an open-string agentType (length-trimmed only, never @@ -78,7 +78,7 @@ export function recordCrashBreadcrumb( return breadcrumb } breadcrumbs.push(breadcrumb) - if (breadcrumbs.length > MAX_BREADCRUMBS) { + if (breadcrumbs.length > MAX_DESKTOP_CRASH_BREADCRUMBS) { breadcrumbs.shift() } return breadcrumb @@ -178,7 +178,7 @@ function isCoalescedCrumbStillInEvidence( isVisibleToReporter(breadcrumb, reporterOrigin) ) return visibleRecent - .slice(-(MAX_BREADCRUMBS - retained.length)) + .slice(-(MAX_DESKTOP_CRASH_BREADCRUMBS - retained.length)) .some((recentBreadcrumb) => recentBreadcrumb === crumb) } @@ -254,7 +254,7 @@ export function getCrashBreadcrumbSnapshot(reporterOrigin?: string): CrashReport const visibleRecent = breadcrumbs.filter((breadcrumb) => isVisibleToReporter(breadcrumb, reporterOrigin) ) - const recent = visibleRecent.slice(-(MAX_BREADCRUMBS - retained.length)) + const recent = visibleRecent.slice(-(MAX_DESKTOP_CRASH_BREADCRUMBS - retained.length)) return [...retained, ...recent] .sort((left, right) => left.createdAt.localeCompare(right.createdAt)) .map((breadcrumb) => ({ diff --git a/src/shared/crash-error-description.test.ts b/src/shared/crash-error-description.test.ts new file mode 100644 index 00000000000..95de13649c6 --- /dev/null +++ b/src/shared/crash-error-description.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it } from 'vitest' +import { describeCrashError } from './crash-error-description' + +describe('describeCrashError', () => { + it('keeps only stack frames and never records multi-line message content', () => { + const error = new Error( + 'Failed to render prompt:\nSECRET_TOKEN=do-not-share\n at private message-shaped frame' + ) + error.stack = [ + 'Error: Failed to render prompt:', + 'SECRET_TOKEN=do-not-share', + ' at private message-shaped frame', + ' at PrivateScreen (/Users/example/private-repo/screen.tsx:1:1)' + ].join('\n') + const description = describeCrashError( + error, + '\n at PrivateScreen (/Users/example/private-repo/screen.tsx:1:1)' + ) + + expect(description.errorName).toBe('Error') + expect(description.errorFingerprint).toMatch(/^[0-9a-f]{8}$/) + expect(description.errorStack).toContain('at PrivateScreen ([redacted-path]') + expect(description.errorStack).not.toContain('SECRET_TOKEN') + expect(description.errorStack).not.toContain('private message-shaped frame') + expect(description.componentStack).toContain('at PrivateScreen ([redacted-path]') + }) +}) diff --git a/src/shared/crash-error-description.ts b/src/shared/crash-error-description.ts new file mode 100644 index 00000000000..3eefc3a6f1d --- /dev/null +++ b/src/shared/crash-error-description.ts @@ -0,0 +1,41 @@ +import { sanitizeCrashReportString, type CrashReportBreadcrumbData } from './crash-reporting' + +export type CrashErrorDescription = CrashReportBreadcrumbData + +const MAX_STACK_CHARS = 2_000 + +export function describeCrashError( + error: unknown, + componentStack?: string | null +): CrashErrorDescription { + const candidate = error instanceof Error ? error : null + const message = candidate?.message ?? String(error) + const stack = extractStackFrames(candidate?.stack, message) + return { + errorName: sanitizeCrashReportString(candidate?.name || 'NonErrorThrown', 80), + errorFingerprint: fingerprint(message), + ...(stack ? { errorStack: sanitizeCrashReportString(stack, MAX_STACK_CHARS) } : {}), + ...(componentStack?.trim() + ? { componentStack: sanitizeCrashReportString(componentStack.trim(), MAX_STACK_CHARS) } + : {}) + } +} + +function extractStackFrames(stack: string | undefined, message: string): string | undefined { + const messageLineCount = message.split(/\r?\n/).length + const lines = (stack?.split(/\r?\n/) ?? []).slice(messageLineCount) + const firstFrameIndex = lines.findIndex((line) => /^\s+at\b/.test(line)) + if (firstFrameIndex === -1) { + return undefined + } + return lines.slice(firstFrameIndex).join('\n').trim() || undefined +} + +function fingerprint(value: string): string { + let hash = 0x811c9dc5 + for (let index = 0; index < value.length; index += 1) { + hash ^= value.charCodeAt(index) + hash = Math.imul(hash, 0x01000193) + } + return (hash >>> 0).toString(16).padStart(8, '0') +} diff --git a/src/shared/crash-reporting.ts b/src/shared/crash-reporting.ts index 830449bcc9d..e254c9c8e3a 100644 --- a/src/shared/crash-reporting.ts +++ b/src/shared/crash-reporting.ts @@ -127,7 +127,7 @@ export type CrashReportCopyDiagnosticsArgs = { const MAX_STRING_DETAIL_LENGTH = 240 const MAX_STACK_DETAIL_LENGTH = 4_000 const MAX_BREADCRUMB_NAME_LENGTH = 80 -const MAX_BREADCRUMBS = 30 +const MAX_CRASH_REPORT_BREADCRUMBS = 30 const MAX_FORMATTED_REPORT_LENGTH = 64_000 const FORMATTED_REPORT_TRUNCATION_SUFFIX = '\n\n[Crash report truncated to fit feedback endpoint limits.]' @@ -214,7 +214,7 @@ export function sanitizeCrashReportBreadcrumbs( return undefined } const sanitized = breadcrumbs - .slice(-MAX_BREADCRUMBS) + .slice(-MAX_CRASH_REPORT_BREADCRUMBS) .map((breadcrumb): CrashReportBreadcrumb | null => { if (!breadcrumb.name.trim() || !breadcrumb.createdAt.trim()) { return null