Conversation
MobileBrowserPane's browser.screencast subscribe effect never re-ran on a relay migrateTo / direct-socket reconnect — its deps hold the stable logical client — so the double-buffer render state was never reset. After a missed offscreen <Image> onLoad across the blip the pane froze on the last decoded frame while input RPCs kept landing. Drive the subscribe effect off a reconnect signal derived from the logical client's onStateChange so the stream is torn down, recreated, and its render state reset on every reconnect. Adds a behavioral hook test plus a source invariant; no wire/RPC changes. Co-Authored-By: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mobile/src/browser/use-browser-screencast-reconnect-signal.test.tsx (1)
50-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover a reconnect after the first connection.
Line 50 verifies only the initial connection. Add
disconnected -> connected -> disconnected -> connectedand expect one increment. This verifies that the hook records the first connection before the later reconnect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d900f1bb-4938-4f9b-ba54-90c60377a6e9
📒 Files selected for processing (4)
mobile/src/browser/MobileBrowserPane.tsxmobile/src/browser/mobile-browser-pane-source.test.tsmobile/src/browser/use-browser-screencast-reconnect-signal.test.tsxmobile/src/browser/use-browser-screencast-reconnect-signal.ts
Add the disconnected -> connected (first connect, no bump) -> disconnected -> connected (reconnect) -> repeat sequence, proving the signal records the initial connection before bumping on later reconnects and increments once per cycle. Addresses the reconnect-coverage gap flagged in review. Co-Authored-By: Claude <noreply@anthropic.com>
|
Closing this one — superseded by #16122, which reaches the actual root cause (the stuck decoder slot after reconnect) rather than the symptom-level subscription restart this PR took. Thanks for the co-author credit — glad the report helped pin down the mechanism. Happy to test on mobile once #16122 lands. |
ELI5
On mobile, the in-app browser shows the host's browser as a live picture stream. If the connection blips (network drop or relay switch) and reconnects, the picture used to freeze on the last frame — taps still worked, but the image never updated until the tab was reopened. The pane never restarted the picture stream after reconnecting. This PR makes the pane notice the reconnect and restart the stream, so the browser picture comes back to life.
What Changed
useBrowserScreencastReconnectSignal— a hook returning a number that increments only on a reconnect (connected → away → connected), derived from the logical RPC client's existingonStateChange. It ignores the very first connect.MobileBrowserPane'sbrowser.screencastsubscribe effect now depends on this signal, so on every reconnect the effect tears down and recreates the stream and resets its double-buffer render state.One new module + one effect-dependency line. No wire/RPC/stream-opcode changes and no new dependencies.
Why
MobileBrowserPane's screencast subscribe effect depended only on the stable logical client, so it never re-ran on a relaymigrateTo/ direct-socket reconnect. The host tears downPage.startScreencaston reconnect and only restarts it on a freshbrowser.screencastsubscribe — which never came. A single missed offscreen<Image>onLoadacross the blip then leftpendingFrameLayerRefstuck, so every later frame painted only the hidden layer: the display froze on the last decoded frame while input RPCs kept landing. Driving the effect off a reconnect-derived signal is the minimal fix — it reuses the client's own state events and changes no transport behavior.Linked Issue
Fixes #14274
Visual Proof
No visual change. The change recovers a pane that was already frozen; there is no new UI to screenshot. See Testing for how the behavior is verified.
Testing
Automated tests added/updated
I manually tested these changes locally
use-browser-screencast-reconnect-signal.test.tsx— behavioral test (react-test-renderer + mock RPC client): the signal stays0on the first connect and on duplicateconnectedstates, and increments once per reconnect — including across adisconnected → connected → disconnected → connectedsequence from a fresh start and across repeated reconnect cycles.mobile-browser-pane-source.test.ts— source invariant thatMobileBrowserPaneconsumes the reconnect signal in its screencast subscribe-effect dependency array.Behavioral verification of the freeze→resume path needs a paired host and a live transport reconnect (relay migration or direct-socket drop/recover) on a real device/emulator; the unit tests pin the underlying invariant that drives it. Platforms in scope: mobile (iOS/Android). The hook is RN-agnostic and keys off the shared logical client.
Locally:
pnpm typecheckandpnpm lintpass, and the screencast test files pass in isolation. The fullmobile/suite has a pre-existing, unrelated failure insrc/transport/host-store.test.ts(a zod schema-resolution error in a different subsystem; this PR does not touch it). CI is the source of truth for the full run.A bounded pending-frame watchdog (force-swap the visible layer if the offscreen
onLoadis missed outside a reconnect) is intentionally deferred to a follow-up to keep this change small.AI Disclosure
Developed with assistance from Claude (Anthropic). The root-cause analysis, approach, and tests were reviewed by the contributor.
Review
onStateChange/getState(RN-agnostic); works for both direct-socket recovery and relaymigrateTo. No platform-specific code.Checklist
N/Awith reasonpnpm lint,pnpm typecheck, and the in-scopepnpm testpass (CI covers the full suite andpnpm build)Author