From f2f6493316fcde52314c9f29742043937faa20ad Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 29 May 2026 19:27:24 -0700 Subject: [PATCH] Move mobile browser ref mirrors out of effects (#3230) * Move mobile browser refs out of effects * Use layout effect for mobile browser ref mirrors Co-authored-by: Orca --------- Co-authored-by: Jinwoo-H Co-authored-by: Orca --- mobile/src/browser/MobileBrowserPane.tsx | 25 ++++++++--------- .../mobile-browser-pane-source.test.ts | 27 +++++++++++++++++++ 2 files changed, 40 insertions(+), 12 deletions(-) create mode 100644 mobile/src/browser/mobile-browser-pane-source.test.ts diff --git a/mobile/src/browser/MobileBrowserPane.tsx b/mobile/src/browser/MobileBrowserPane.tsx index cfaed3061..b53ef24d6 100644 --- a/mobile/src/browser/MobileBrowserPane.tsx +++ b/mobile/src/browser/MobileBrowserPane.tsx @@ -1,5 +1,13 @@ import { Buffer } from 'buffer' -import { useCallback, useEffect, useMemo, useRef, useState, type ReactNode } from 'react' +import { + useCallback, + useEffect, + useLayoutEffect, + useMemo, + useRef, + useState, + type ReactNode +} from 'react' import { ActivityIndicator, AppState, @@ -209,21 +217,14 @@ export function MobileBrowserPane({ } }, [addressFocused, tab.url]) - useEffect(() => { + useLayoutEffect(() => { + // Why: gesture and stream handlers need committed values before passive + // Effects flush, without leaking refs from an uncommitted render. frameMetadataRef.current = frameMetadata - }, [frameMetadata]) - - useEffect(() => { layoutRef.current = layout - }, [layout]) - - useEffect(() => { dialogRef.current = dialog - }, [dialog]) - - useEffect(() => { zoomRef.current = zoom - }, [zoom]) + }, [dialog, frameMetadata, layout, zoom]) useEffect(() => { lastZoomResetUrlRef.current = tab.url || 'about:blank' diff --git a/mobile/src/browser/mobile-browser-pane-source.test.ts b/mobile/src/browser/mobile-browser-pane-source.test.ts new file mode 100644 index 000000000..69cd5af53 --- /dev/null +++ b/mobile/src/browser/mobile-browser-pane-source.test.ts @@ -0,0 +1,27 @@ +import { readFileSync } from 'node:fs' +import { describe, expect, it } from 'vitest' + +const source = readFileSync(new URL('./MobileBrowserPane.tsx', import.meta.url), 'utf8') + +function sliceBetween(startPattern: string, endPattern: string): string { + const start = source.indexOf(startPattern) + expect(start).toBeGreaterThanOrEqual(0) + const end = source.indexOf(endPattern, start) + expect(end).toBeGreaterThan(start) + return source.slice(start, end) +} + +describe('MobileBrowserPane source invariants', () => { + it('mirrors handler refs in a layout effect instead of during render', () => { + const mirrorBlock = sliceBetween( + 'useLayoutEffect(() => {', + ' useEffect(() => {\n lastZoomResetUrlRef.current' + ) + + expect(mirrorBlock).toContain('frameMetadataRef.current = frameMetadata') + expect(mirrorBlock).toContain('layoutRef.current = layout') + expect(mirrorBlock).toContain('dialogRef.current = dialog') + expect(mirrorBlock).toContain('zoomRef.current = zoom') + expect(mirrorBlock).toContain('}, [dialog, frameMetadata, layout, zoom])') + }) +})