From 14edd54f3fc75daf107fd549cefe2178adf1dd86 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Wed, 28 Oct 2026 20:35:22 -0700 Subject: [PATCH] fix(mobile): retire browser pointer queues with their pane (#34825) * fix(mobile): retire browser pointer queues with their pane * test(mobile): use T[] array types in pointer retirement test --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- .../browser/use-mobile-browser-commands.ts | 106 ++----- ...mobile-browser-pointer-retirement.test.tsx | 268 ++++++++++++++++++ .../browser/use-mobile-browser-wheel-queue.ts | 119 ++++++++ 3 files changed, 414 insertions(+), 79 deletions(-) create mode 100644 mobile/src/browser/use-mobile-browser-pointer-retirement.test.tsx create mode 100644 mobile/src/browser/use-mobile-browser-wheel-queue.ts diff --git a/mobile/src/browser/use-mobile-browser-commands.ts b/mobile/src/browser/use-mobile-browser-commands.ts index d2fbea43b93..d0d0c262697 100644 --- a/mobile/src/browser/use-mobile-browser-commands.ts +++ b/mobile/src/browser/use-mobile-browser-commands.ts @@ -17,13 +17,10 @@ import { browserPointerClick, browserPointerDown, browserPointerMove, - browserPointerUp, - browserPointerWheel + browserPointerUp } from './mobile-browser-command-operations' import type { BrowserPageCommandSend, BrowserPageParams } from './use-mobile-browser-request' import { - browserWheelDeltaFromScreen, - computeBrowserFrameGeometry, computeBrowserTouchClickRadiusCss, mapScreenToBrowserPoint, type BrowserPoint, @@ -32,15 +29,9 @@ import { } from './browser-touch-geometry' import type { BrowserPointerModifier } from './MobileBrowserPointerModifiers' import type { BrowserDialogState } from './mobile-browser-stream-events' +import { useMobileBrowserWheelQueue } from './use-mobile-browser-wheel-queue' const TOUCH_CLICK_RADIUS_DIP = 14 -type PendingWheelCommand = { - base: BrowserPageParams - point: BrowserPoint - gestureId: number - dx: number - dy: number -} type SendBrowserRequest = ( send: BrowserPageCommandSend, options?: { showBusy?: boolean; suppressError?: boolean; timeoutMs?: number } @@ -79,8 +70,15 @@ export function useMobileBrowserCommands(args: MobileBrowserCommandArgs) { zoomRef } = args - const pendingWheelCommandRef = useRef(null) const dialogAnswerTokenRef = useRef(0) + const { commandGenerationRef, commandsMountedRef, sendWheel } = useMobileBrowserWheelQueue({ + client, + frameMetadataRef, + layoutRef, + pageParams, + setError, + zoomRef + }) // A send claims the draft here, so a repeat tap before re-render finds it empty. The pane remounts // per page, so the draft outlives a reconnect; only unmount ends a send's claim on it. const keyboardDraftRef = useRef({ mounted: true, value: keyboardValue }) @@ -95,48 +93,13 @@ export function useMobileBrowserCommands(args: MobileBrowserCommandArgs) { } }, []) - const wheelCommandInFlightRef = useRef(false) - - const flushPendingWheelCommand = useCallback(() => { - if (wheelCommandInFlightRef.current) { - return - } - const pending = pendingWheelCommandRef.current - if (!pending || !client) { - return - } - pendingWheelCommandRef.current = null - wheelCommandInFlightRef.current = true - void (async () => { - try { - const moveReply = await browserPointerMove.request(client, { - ...pending.base, - x: pending.point.x, - y: pending.point.y - }) - browserPointerMove.interpret(moveReply) - const wheelReply = await browserPointerWheel.request(client, { - ...pending.base, - dx: pending.dx, - dy: pending.dy - }) - browserPointerWheel.interpret(wheelReply) - setError(null) - } catch { - // Scroll bursts race navigation; keep usable stream instead of replacing it with transient selector errors. - } finally { - wheelCommandInFlightRef.current = false - flushPendingWheelCommand() - } - })() - }, [client]) - const sendPointerClick = useCallback( async (point: BrowserPoint, button: 'left' | 'right') => { const base = pageParams() - if (!client || !base) { + if (!commandsMountedRef.current || !client || !base) { return } + const generation = commandGenerationRef.current try { browserPointerClick.interpret( await browserPointerClick.request( @@ -161,11 +124,17 @@ export function useMobileBrowserCommands(args: MobileBrowserCommandArgs) { { timeoutMs: 5_000 } ) ) - setError(null) + if (generation === commandGenerationRef.current) { + setError(null) + } return } catch (error) { // Why: a timed-out click may still run on the host, and the move/down/up replay drops modifiers. - if (isRpcDeliveryUnknown(error) || pointerModifiers.length > 0) { + if ( + generation !== commandGenerationRef.current || + isRpcDeliveryUnknown(error) || + pointerModifiers.length > 0 + ) { return } } @@ -176,11 +145,17 @@ export function useMobileBrowserCommands(args: MobileBrowserCommandArgs) { y: point.y }) browserPointerMove.interpret(moveReply) + if (generation !== commandGenerationRef.current) { + return + } const downReply = await browserPointerDown.request(client, { ...base, button }) browserPointerDown.interpret(downReply) + // Release an admitted press even when its pane retired while mouseDown was pending. const upReply = await browserPointerUp.request(client, { ...base, button }) browserPointerUp.interpret(upReply) - setError(null) + if (generation === commandGenerationRef.current) { + setError(null) + } } catch { // Pointer sends race navigation; keep stream and surface actionable failures through navigation/stream paths. } @@ -196,33 +171,6 @@ export function useMobileBrowserCommands(args: MobileBrowserCommandArgs) { ) }, []) - const sendWheel = useCallback( - (point: BrowserPoint, screenDx: number, screenDy: number, gestureId: number) => { - const base = pageParams() - if (!client || !base) { - return - } - const geometry = computeBrowserFrameGeometry(layoutRef.current, frameMetadataRef.current) - const delta = browserWheelDeltaFromScreen(screenDx, screenDy, geometry, zoomRef.current.scale) - if (Math.abs(delta.dx) < 1 && Math.abs(delta.dy) < 1) { - return - } - const pending = pendingWheelCommandRef.current - pendingWheelCommandRef.current = - pending && pending.base.page === base.page && pending.gestureId === gestureId - ? { - base, - point, - gestureId, - dx: pending.dx + delta.dx, - dy: pending.dy + delta.dy - } - : { base, point, gestureId, ...delta } - flushPendingWheelCommand() - }, - [client, flushPendingWheelCommand, pageParams] - ) - const mapTouchPoint = useCallback((locationX: number, locationY: number): BrowserPoint | null => { return mapScreenToBrowserPoint( locationX, diff --git a/mobile/src/browser/use-mobile-browser-pointer-retirement.test.tsx b/mobile/src/browser/use-mobile-browser-pointer-retirement.test.tsx new file mode 100644 index 00000000000..5d758670559 --- /dev/null +++ b/mobile/src/browser/use-mobile-browser-pointer-retirement.test.tsx @@ -0,0 +1,268 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, describe, expect, it } from 'vitest' +import type { RpcClient } from '../transport/rpc-client' +import { stubRpcClient } from '../transport/rpc-client-test-stub' +import type { RpcResponse } from '../transport/types' +import { useMobileBrowserCommands } from './use-mobile-browser-commands' +import { useMobileBrowserRequest } from './use-mobile-browser-request' + +function deferred() { + let resolve!: (value: Value) => void + const promise = new Promise((settle) => { + resolve = settle + }) + return { promise, resolve } +} + +const accepted: RpcResponse = { id: 'reply', ok: true, result: {} } +const refused: RpcResponse = { + id: 'reply', + ok: false, + error: { code: 'unknown-method', message: 'Unknown method' } +} +type Commands = ReturnType +const renderers: ReactTestRenderer[] = [] + +afterEach(() => { + act(() => { + for (const renderer of renderers.splice(0)) { + renderer.unmount() + } + }) +}) + +function mount(client: RpcClient) { + let commands: Commands | null = null + const errors: (string | null)[] = [] + function Screen({ currentClient }: { currentClient: RpcClient }) { + const { pageParams, sendBrowserRequest } = useMobileBrowserRequest({ + busyRef: { current: false }, + client: currentClient, + pageId: 'page', + setBusy: () => {}, + setError: () => {}, + worktreeId: 'workspace' + }) + commands = useMobileBrowserCommands({ + client: currentClient, + pageParams, + sendBrowserRequest, + frameMetadataRef: { current: null }, + keyboardValue: '', + layoutRef: { current: null }, + onToast: () => {}, + pointerModifiers: [], + setDialog: () => {}, + setError: (value) => { + if (typeof value !== 'function') { + errors.push(value) + } + }, + setKeyboardValue: () => {}, + setPointerModifiers: () => {}, + zoomRef: { current: { scale: 1, offsetX: 0, offsetY: 0 } } + }) + return null + } + let renderer!: ReactTestRenderer + act(() => { + renderer = create(createElement(Screen, { currentClient: client })) + }) + renderers.push(renderer) + return { + get commands(): Commands { + if (!commands) { + throw new Error('not mounted') + } + return commands + }, + errors, + unmount: () => act(() => renderer.unmount()), + replace: (currentClient: RpcClient) => + act(() => renderer.update(createElement(Screen, { currentClient }))) + } +} + +describe('browser pointer commands after owner retirement', () => { + it.each([8, 80])('does not drain scroll work for %i departed panes', async (owners) => { + const move = deferred() + const calls: string[] = [] + const client = stubRpcClient({ + sendRequest: async (method) => { + calls.push(method) + return method === 'browser.mouseMove' ? move.promise : accepted + } + }) + for (let index = 0; index < owners; index++) { + const screen = mount(client) + screen.commands.sendWheel({ x: 10, y: 20 }, 0, 10, index) + screen.commands.sendWheel({ x: 10, y: 25 }, 0, 20, index) + screen.unmount() + } + expect(calls).toEqual(Array.from({ length: owners }, () => 'browser.mouseMove')) + await act(async () => { + move.resolve(accepted) + await move.promise + }) + expect(calls).toHaveLength(owners) + + const liveCalls: { method: string; params: unknown }[] = [] + const live = mount( + stubRpcClient({ + sendRequest: async (method, params) => { + liveCalls.push({ method, params }) + return accepted + } + }) + ) + await act(async () => { + live.commands.sendWheel({ x: 5, y: 6 }, 0, 30, 1) + }) + expect(liveCalls).toEqual([ + { + method: 'browser.mouseMove', + params: { worktree: 'id:workspace', page: 'page', x: 5, y: 6 } + }, + { + method: 'browser.mouseWheel', + params: { worktree: 'id:workspace', page: 'page', dx: 0, dy: -30 } + } + ]) + expect(live.errors).toEqual([null]) + }) + + it('does not replay a late refused click after leaving the pane', async () => { + const click = deferred() + const calls: string[] = [] + const screen = mount( + stubRpcClient({ + sendRequest: async (method) => { + calls.push(method) + return method === 'browser.mouseClick' ? click.promise : accepted + } + }) + ) + const pending = screen.commands.sendPointerClick({ x: 10, y: 20 }, 'left') + screen.unmount() + await act(async () => { + click.resolve(refused) + await pending + }) + expect(calls).toEqual(['browser.mouseClick']) + expect(screen.errors).toEqual([]) + }) + + it('releases an already admitted mouse down after leaving the pane', async () => { + const down = deferred() + const calls: string[] = [] + const screen = mount( + stubRpcClient({ + sendRequest: async (method) => { + calls.push(method) + if (method === 'browser.mouseClick') { + return refused + } + return method === 'browser.mouseDown' ? down.promise : accepted + } + }) + ) + let pending!: Promise + await act(async () => { + pending = screen.commands.sendPointerClick({ x: 10, y: 20 }, 'left') + }) + expect(calls).toEqual(['browser.mouseClick', 'browser.mouseMove', 'browser.mouseDown']) + screen.unmount() + await act(async () => { + down.resolve(accepted) + await pending + }) + expect(calls.at(-1)).toBe('browser.mouseUp') + expect(screen.errors).toEqual([]) + }) + + it('does not start a fallback press after its move outlives the pane', async () => { + const move = deferred() + const calls: string[] = [] + const screen = mount( + stubRpcClient({ + sendRequest: async (method) => { + calls.push(method) + if (method === 'browser.mouseClick') { + return refused + } + return method === 'browser.mouseMove' ? move.promise : accepted + } + }) + ) + let pending!: Promise + await act(async () => { + pending = screen.commands.sendPointerClick({ x: 10, y: 20 }, 'left') + }) + expect(calls).toEqual(['browser.mouseClick', 'browser.mouseMove']) + screen.unmount() + await act(async () => { + move.resolve(accepted) + await pending + }) + expect(calls).toEqual(['browser.mouseClick', 'browser.mouseMove']) + }) + + it('lets a replacement client scroll without an old callback draining its queue', async () => { + const oldMove = deferred() + const oldCalls: string[] = [] + const screen = mount( + stubRpcClient({ + sendRequest: async (method) => { + oldCalls.push(method) + return method === 'browser.mouseMove' ? oldMove.promise : accepted + } + }) + ) + screen.commands.sendWheel({ x: 1, y: 2 }, 0, 3, 1) + const nextMove = deferred() + const newCalls: { method: string; params: unknown }[] = [] + screen.replace( + stubRpcClient({ + sendRequest: async (method, params) => { + newCalls.push({ method, params }) + return method === 'browser.mouseMove' && newCalls.length === 1 + ? nextMove.promise + : accepted + } + }) + ) + screen.commands.sendWheel({ x: 4, y: 5 }, 0, 6, 2) + screen.commands.sendWheel({ x: 7, y: 8 }, 0, 9, 2) + screen.commands.sendWheel({ x: 7, y: 8 }, 0, 12, 2) + expect(newCalls.map(({ method }) => method)).toEqual(['browser.mouseMove']) + await act(async () => { + oldMove.resolve(accepted) + await oldMove.promise + }) + expect(oldCalls).toEqual(['browser.mouseMove']) + expect(newCalls).toHaveLength(1) + await act(async () => { + nextMove.resolve(accepted) + await nextMove.promise + }) + expect(newCalls).toEqual([ + { + method: 'browser.mouseMove', + params: { worktree: 'id:workspace', page: 'page', x: 4, y: 5 } + }, + { + method: 'browser.mouseWheel', + params: { worktree: 'id:workspace', page: 'page', dx: 0, dy: -6 } + }, + { + method: 'browser.mouseMove', + params: { worktree: 'id:workspace', page: 'page', x: 7, y: 8 } + }, + { + method: 'browser.mouseWheel', + params: { worktree: 'id:workspace', page: 'page', dx: 0, dy: -21 } + } + ]) + }) +}) diff --git a/mobile/src/browser/use-mobile-browser-wheel-queue.ts b/mobile/src/browser/use-mobile-browser-wheel-queue.ts new file mode 100644 index 00000000000..79fb090254e --- /dev/null +++ b/mobile/src/browser/use-mobile-browser-wheel-queue.ts @@ -0,0 +1,119 @@ +import { useCallback, useLayoutEffect, useRef, type Dispatch, type SetStateAction } from 'react' +import type { RpcClient } from '../transport/rpc-client' +import type { BrowserScreencastFrameMetadata } from '../transport/browser-screencast-protocol' +import { browserPointerMove, browserPointerWheel } from './mobile-browser-command-operations' +import type { BrowserPageParams } from './use-mobile-browser-request' +import { + browserWheelDeltaFromScreen, + computeBrowserFrameGeometry, + type BrowserPoint, + type BrowserTouchLayout, + type BrowserZoomState +} from './browser-touch-geometry' + +type PendingWheelCommand = { + base: BrowserPageParams + point: BrowserPoint + gestureId: number + dx: number + dy: number +} + +type BrowserWheelQueueArgs = { + client: RpcClient | null + frameMetadataRef: { current: BrowserScreencastFrameMetadata | null } + layoutRef: { current: BrowserTouchLayout | null } + pageParams: () => BrowserPageParams | null + setError: Dispatch> + zoomRef: { current: BrowserZoomState } +} + +// Retires queued wheel work with its pane/client so departed owners cannot drain into a live one. +export function useMobileBrowserWheelQueue(args: BrowserWheelQueueArgs) { + const { client, frameMetadataRef, layoutRef, pageParams, setError, zoomRef } = args + const pendingWheelCommandRef = useRef(null) + const wheelCommandInFlightRef = useRef(false) + const commandGenerationRef = useRef(0) + const commandsMountedRef = useRef(false) + + useLayoutEffect(() => { + commandsMountedRef.current = true + return () => { + commandsMountedRef.current = false + commandGenerationRef.current += 1 + pendingWheelCommandRef.current = null + wheelCommandInFlightRef.current = false + } + }, [client, pageParams]) + + const flushPendingWheelCommand = useCallback(() => { + if (!commandsMountedRef.current || wheelCommandInFlightRef.current) { + return + } + const pending = pendingWheelCommandRef.current + if (!pending || !client) { + return + } + pendingWheelCommandRef.current = null + wheelCommandInFlightRef.current = true + const generation = commandGenerationRef.current + void (async () => { + try { + const moveReply = await browserPointerMove.request(client, { + ...pending.base, + x: pending.point.x, + y: pending.point.y + }) + browserPointerMove.interpret(moveReply) + if (generation !== commandGenerationRef.current) { + return + } + const wheelReply = await browserPointerWheel.request(client, { + ...pending.base, + dx: pending.dx, + dy: pending.dy + }) + browserPointerWheel.interpret(wheelReply) + if (generation === commandGenerationRef.current) { + setError(null) + } + } catch { + // Scroll bursts race navigation; keep usable stream instead of replacing it with transient selector errors. + } finally { + if (generation === commandGenerationRef.current) { + wheelCommandInFlightRef.current = false + flushPendingWheelCommand() + } + } + })() + }, [client]) + + const sendWheel = useCallback( + (point: BrowserPoint, screenDx: number, screenDy: number, gestureId: number) => { + const base = pageParams() + if (!commandsMountedRef.current || !client || !base) { + return + } + const geometry = computeBrowserFrameGeometry(layoutRef.current, frameMetadataRef.current) + const delta = browserWheelDeltaFromScreen(screenDx, screenDy, geometry, zoomRef.current.scale) + if (Math.abs(delta.dx) < 1 && Math.abs(delta.dy) < 1) { + return + } + const pending = pendingWheelCommandRef.current + pendingWheelCommandRef.current = + pending && pending.base.page === base.page && pending.gestureId === gestureId + ? { + base, + point, + gestureId, + dx: pending.dx + delta.dx, + dy: pending.dy + delta.dy + } + : { base, point, gestureId, ...delta } + flushPendingWheelCommand() + }, + [client, flushPendingWheelCommand, pageParams] + ) + + return { commandGenerationRef, commandsMountedRef, sendWheel } +}