From a4c0150311232c4e9b52ed03647d1e8ddacfdd99 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Fri, 4 Sep 2026 19:43:14 -0700 Subject: [PATCH] fix(mobile): carry the turn key instead of caching a handler in a ref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builds on the scope-isolation fix: that kept (and extended) a ref that is written during render — once to memoize a per-turn handler, once to prune dead turns, once to reset on a scope change. React Doctor's "Ref mutated during render" is what CI's `check:react-doctor:changed` was failing on (x2), and on mobile it is a real hazard rather than a style note: react-freeze discards renders, and a discarded render would leave the cache mutated. Pass the settled turn's key down the row instead and let it call one stable handler with it. That preserves both properties the cache was bought for — per scope isolation, and identity stability so a streaming transcript does not defeat the row's memo — with no ref writes and no pruning to get wrong. The scope-keyed expanded set and the 128-turn cap are untouched; their tests move to the new contract and one now pins handler identity across a re-render. Note for future changes here: `check:code-quality:changed` does NOT cover this. CI additionally runs the standalone react-doctor CLI, which has rules the oxlint plugin config does not enable. --- .../src/session/MobileNativeChatMessage.tsx | 8 ++- mobile/src/session/MobileNativeChatView.tsx | 1 + ...obile-native-chat-turn-disclosure.test.tsx | 21 ++++-- .../use-mobile-native-chat-turn-disclosure.ts | 67 ++++--------------- 4 files changed, 36 insertions(+), 61 deletions(-) diff --git a/mobile/src/session/MobileNativeChatMessage.tsx b/mobile/src/session/MobileNativeChatMessage.tsx index 648d1d396ee..cc6c086b1f3 100644 --- a/mobile/src/session/MobileNativeChatMessage.tsx +++ b/mobile/src/session/MobileNativeChatMessage.tsx @@ -105,6 +105,7 @@ function MobileNativeChatMessageImpl({ onOpenFile, turnStatus, turnExpanded, + turnKey, onToggleTurn, activeTurnIsWorking, structuredActivityUi = false @@ -122,7 +123,10 @@ function MobileNativeChatMessageImpl({ turnStatus?: NativeChatTurnStatus | null /** Whether the turn caret has disclosed this turn's activity. */ turnExpanded?: boolean - onToggleTurn?: () => void + /** Set only when this row's turn has settled and can disclose its activity. */ + turnKey?: string + /** Stable across renders; the row supplies its own key when tapped. */ + onToggleTurn?: (turnKey: string) => void /** Session-level working state for this message's turn; gates the live tool row. */ activeTurnIsWorking?: boolean /** Structured lane only: live tool progress plus the turn-status disclosure. */ @@ -230,7 +234,7 @@ function MobileNativeChatMessageImpl({ thinking={turnStatus.thinking} workedSeconds={turnStatus.workedSeconds} expanded={turnExpanded ?? false} - onToggleExpanded={turnStatus.workedSeconds != null ? onToggleTurn : undefined} + onToggleExpanded={turnKey && onToggleTurn ? () => onToggleTurn(turnKey) : undefined} /> ) : null} diff --git a/mobile/src/session/MobileNativeChatView.tsx b/mobile/src/session/MobileNativeChatView.tsx index a10b88002bd..59c024435b6 100644 --- a/mobile/src/session/MobileNativeChatView.tsx +++ b/mobile/src/session/MobileNativeChatView.tsx @@ -275,6 +275,7 @@ export function MobileNativeChatView({ onScrollToMessage={onScrollToMessage} onOpenFile={onOpenFile} structuredActivityUi={structuredActivityUi} + onToggleTurn={turns.onToggleTurn} {...turns.resolveRow(index, item)} /> ), diff --git a/mobile/src/session/use-mobile-native-chat-turn-disclosure.test.tsx b/mobile/src/session/use-mobile-native-chat-turn-disclosure.test.tsx index 5b81673ac02..8467684ce16 100644 --- a/mobile/src/session/use-mobile-native-chat-turn-disclosure.test.tsx +++ b/mobile/src/session/use-mobile-native-chat-turn-disclosure.test.tsx @@ -99,8 +99,18 @@ describe('useMobileNativeChatTurnDisclosure', () => { .findByType('result') .props.disclosure.resolveRow(0, refreshed[0]) - expect(first.onToggleTurn).toBeTypeOf('function') - expect(second.onToggleTurn).toBe(first.onToggleTurn) + // The row carries the key; the handler itself lives on the hook and stays + // stable for the scope, so a re-render never disturbs a row's memo. + expect(first.turnKey).toBe('u1') + expect(second.turnKey).toBe('u1') + const firstHandler = renderer!.root.findByType('result').props.disclosure.onToggleTurn + expect(firstHandler).toBeTypeOf('function') + act(() => { + renderer?.update( + createElement(Harness, { messages: [...refreshed], enabled: true, isWorking: false }) + ) + }) + expect(renderer!.root.findByType('result').props.disclosure.onToggleTurn).toBe(firstHandler) } finally { vi.useRealTimers() } @@ -124,10 +134,9 @@ describe('useMobileNativeChatTurnDisclosure', () => { act(() => { renderer?.update(createElement(Harness, { messages, enabled: true, isWorking: false })) }) - const row = renderer!.root - .findByType('result') - .props.disclosure.resolveRow(index, messages[index]) - act(() => row.onToggleTurn()) + const disclosureNow = renderer!.root.findByType('result').props.disclosure + const row = disclosureNow.resolveRow(index, messages[index]) + act(() => disclosureNow.onToggleTurn(row.turnKey)) } const disclosure = renderer!.root.findByType('result').props.disclosure diff --git a/mobile/src/session/use-mobile-native-chat-turn-disclosure.ts b/mobile/src/session/use-mobile-native-chat-turn-disclosure.ts index af6cfac1e66..46b58f29cba 100644 --- a/mobile/src/session/use-mobile-native-chat-turn-disclosure.ts +++ b/mobile/src/session/use-mobile-native-chat-turn-disclosure.ts @@ -1,4 +1,4 @@ -import { useCallback, useMemo, useRef, useState } from 'react' +import { useCallback, useMemo, useState } from 'react' import type { NativeChatMessage } from '../../../src/shared/native-chat-types' import { MOBILE_UNANCHORED_TURN_KEY, @@ -13,7 +13,8 @@ const MAX_EXPANDED_TURNS = 128 export type MobileNativeChatTurnRow = { turnStatus: NativeChatTurnStatus | null turnExpanded: boolean - onToggleTurn?: () => void + /** Set only on a settled turn — the one row that has activity to disclose. */ + turnKey?: string activeTurnIsWorking: boolean } @@ -35,6 +36,7 @@ export function useMobileNativeChatTurnDisclosure({ active: NativeChatTurnStatus | null /** True when the live turn has no user message to hang its status row under. */ activeTurnIsUnanchored: boolean + onToggleTurn: (turnKey: string) => void resolveRow: (index: number, message: NativeChatMessage) => MobileNativeChatTurnRow } { const turnStatuses = useMobileNativeChatTurnStatus({ @@ -67,55 +69,20 @@ export function useMobileNativeChatTurnDisclosure({ }, [scopeKey] ) - // Why: a fresh closure per row per render defeats the message row's memo, and a - // streaming turn re-renders the list ~20x/s. One stable handler per turn instead. - const toggleHandlers = useRef<{ - scopeKey: string - byTurn: Map void> - }>({ scopeKey, byTurn: new Map() }) - const toggleHandlerFor = useCallback( - (turnKey: string): (() => void) => { - if (toggleHandlers.current.scopeKey !== scopeKey) { - toggleHandlers.current = { scopeKey, byTurn: new Map() } - } - const existing = toggleHandlers.current.byTurn.get(turnKey) - if (existing) { - return existing - } - const handler = (): void => toggleExpandedTurn(turnKey) - toggleHandlers.current.byTurn.set(turnKey, handler) - return handler - }, - [scopeKey, toggleExpandedTurn] - ) - // Resolve each row's turn boundary once — a findLast per row is quadratic on a // long transcript. const turnKeys = useMemo(() => { - if (toggleHandlers.current.scopeKey !== scopeKey) { - toggleHandlers.current = { scopeKey, byTurn: new Map() } - } if (!enabled) { - toggleHandlers.current.byTurn.clear() return EMPTY_TURN_KEYS } let turnKey: string | undefined - const keys = messages.map((message) => { + return messages.map((message) => { if (message.role === 'user') { turnKey = message.id } return turnKey }) - // Drop handlers for turns that left the transcript so a long session does not - // retain a closure per turn forever. - const live = new Set(keys.filter((key): key is string => key !== undefined)) - for (const key of toggleHandlers.current.byTurn.keys()) { - if (!live.has(key)) { - toggleHandlers.current.byTurn.delete(key) - } - } - return keys - }, [enabled, messages, scopeKey]) + }, [enabled, messages]) const { active, activeTurnKey, completedByTurn } = turnStatuses const resolveRow = useCallback( @@ -132,10 +99,11 @@ export function useMobileNativeChatTurnDisclosure({ return { turnStatus, turnExpanded: turnKey ? expandedTurnIds.has(turnKey) : false, - // Only a settled turn has anything to disclose; leaving the handler off - // every other row keeps their props identity-stable. - onToggleTurn: - turnKey && turnStatus?.workedSeconds != null ? toggleHandlerFor(turnKey) : undefined, + // Why: the key travels and the row calls one stable handler with it. A + // closure per row would be a new identity every render of a streaming + // transcript, defeating the row's memo; caching one per turn would mean + // writing a ref during render, which react-freeze can discard. + turnKey: turnKey && turnStatus?.workedSeconds != null ? turnKey : undefined, // With no user boundary at all, the session's working state stays authoritative. activeTurnIsWorking: enabled && @@ -144,20 +112,13 @@ export function useMobileNativeChatTurnDisclosure({ (turnKey === undefined && activeTurnKey === MOBILE_UNANCHORED_TURN_KEY)) } }, - [ - turnKeys, - enabled, - activeTurnKey, - active, - completedByTurn, - expandedTurnIds, - toggleHandlerFor, - isWorking - ] + [turnKeys, enabled, activeTurnKey, active, completedByTurn, expandedTurnIds, isWorking] ) return { active, + /** Stable for a given chat scope, so it never disturbs a row's memo. */ + onToggleTurn: toggleExpandedTurn, activeTurnIsUnanchored: enabled && active != null && activeTurnKey === MOBILE_UNANCHORED_TURN_KEY, resolveRow