-
Notifications
You must be signed in to change notification settings - Fork 0
inspector FSD and metro proxy reconnect + cached Redux #127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add features/tabs-visibility (getTabsVisibility, useTabsVisibility, toggle UI) - Add widgets/title-bar (TitleBar using tabs visibility) - Simplify __root.tsx to layout, server start, SSE, settings modal only - Import getTabsVisibility from feature in devtools page and routes - Add use-tabs-visibility unit tests
…w DevTools - When existing RN inspector: send reconnect inject again so refresh triggers __ChromeRemoteDevToolsReconnect() - When new DevTools connects and RN already exists: send cached Redux stores so Redux/AsyncStorage panel works after Inspector refresh - Add INJECT_RECONNECT_EXPRESSION constant and unit test for payload shape
📊 Test Coverage Report - InspectorSummary
File CoverageClick to expand
Generated by test coverage script |
📊 Test Coverage Report - ServerSummary
File CoverageClick to expand
Generated by Rust test coverage script (cargo-llvm-cov) |
Integration Tests1 tests 1 ✅ 13s ⏱️ Results for commit 3dcf43c. ♻️ This comment has been updated with latest results. |
📊 Test Results✅ Test Status: PASSED📸 ScreenshotsScreenshots are available as artifacts. Download them here. Artifact name: Screenshot files
|
ohah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI review
Purpose and description
The PR description matches the changes: FSD refactor for title bar and tabs visibility, and Metro proxy fixes for reconnect on refresh and cached Redux for new DevTools. Root is simplified; server sends a second inject when RN is already connected and pushes cached Redux stores to newly connected DevTools.
What's done well
- Clear separation:
features/tabs-visibilityandwidgets/title-barfollow FSD; root only handles layout, server start, SSE, and settings. - Reconnect behavior: re-sending the inject when
rn_connection_id.is_some()correctly covers the Inspector-refresh case (new DevTools iframe = new Metro proxy connection). - Cached Redux: sending
INIT_INSTANCE+INITfor each cached store to the new DevTools restores the Redux/AsyncStorage panel after refresh. INJECT_RECONNECT_EXPRESSIONis extracted and tested; the unit test locks the payload shape.- Tabs visibility is covered by tests (getTabsVisibility, useTabsVisibility, toggle and event).
- Root no longer starts the server on every load when it is already running (
is_server_runningcheck), avoiding Metro proxy WS drops on refresh.
Improvement suggestions
- Magic number: The 200 ms delay before sending cached Redux could be a named constant (e.g.
REDUX_CACHE_SEND_DELAY_MS) so the intent and tuning point are clear (see inline). - Edge case: If the RN app disconnects between spawning the Redux-send task and the 200 ms sleep, the task will still run and send to the DevTools; that is acceptable (no stale association). If desired, a short comment could note that the delay allows the new DevTools to be ready.
- Tests: Current tests are good; no further changes required for this PR.
Testing
- Run Inspector (web/Tauri), toggle tabs visibility, confirm devtools page and routes respect it; run
bun testin inspector for tabs-visibility. - Connect RN via Metro proxy, open Inspector, refresh the Inspector tab: app should reconnect and Redux/AsyncStorage panel should show data; run
cargo test -p chrome-remote-devtools-serverfor the inject test.
| let rn_manager_redux = rn_manager.clone(); | ||
| let logger_redux = logger.clone(); | ||
| let metro_id_redux = metro_devtools_id.clone(); | ||
| let inspector_id_for_redux = conn_id.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a module-level constant (e.g. REDUX_CACHE_SEND_DELAY_MS: u64 = 200) and using it here so the intent and tuning point are clear.
Maintainability perspective
|
Unnecessary fallback / scripts perspective
Nothing in this PR looks like unnecessary fallback or script bloat. |
…y tab - Use ArrayBufferLike for MMKV getBuffer so v3 (SharedArrayBuffer) is accepted - Export MMKVStorageInput; example app uses it for registerMMKVDevTools - Call sendAllSnapshots() in setMMKVConnectionReady() so snapshots are sent when WebSocket connects, avoiding empty MMKV tab when panel opened early
📊 Test Coverage Report - React Native InspectorSummary
File CoverageClick to expand
Generated by test coverage script |
📱 Maestro Android Test Results✅ PASSED Maestro Android tests have completed. Check the workflow run for details. |
📱 Maestro iOS Test Results✅ PASSED Maestro iOS tests have completed. Check the workflow run for details. |
…d 8081 - Replace custom toast with @radix-ui/react-toast; add Provider and Viewport in app - Run adb reverse for both Server URL port (8080) and Metro URL port (8081) - Add getMetroPort() in server-url; tooltip shows both ports
feat/inspector-adb-reverse-and-tabs-fix
Purpose
Improve Inspector and Metro proxy behavior: FSD refactor for title bar and tabs visibility, and fix React Native Inspector reconnect and Redux/AsyncStorage after refresh.
Work content
Inspector FSD refactor
features/tabs-visibility:getTabsVisibility/useTabsVisibilityand tab visibility toggle UI; used by the title bar and by the devtools page and routes.widgets/title-bar:TitleBarcomponent that uses tabs visibility; root layout now only handles server start, SSE, and settings modal and composes<TitleBar />.getTabsVisibilityfor unset/"true"/"false"/other,useTabsVisibilityfor initial state, toggle updating state and localStorage, andtabs-visibility-changedispatch).Server Metro proxy
__ChromeRemoteDevToolsReconnect()runs and the app reconnects.INJECT_RECONNECT_EXPRESSIONand a unit test that checks the inject payload shape (Runtime.evaluatewith expression containing__ChromeRemoteDevToolsReconnect).React Native Inspector – MMKV DevTools
registerMMKVDevTools:getBuffernow typed asArrayBufferLikesoSharedArrayBufferis accepted; exportedMMKVStorageInput; example app uses it for the storages object.setMMKVConnectionReady()now callssendAllSnapshots()so snapshots are sent when the WebSocket connects, even if the MMKV panel had opened earlier.Inspector (Tauri) – adb reverse and Toast
@radix-ui/react-toast); Provider and Viewport in app root, Toast in title bar for success/error.getMetroPort()in server-url and run both viaPromise.allwhen ports differ. Single toast summarizes result.How to test
bun testin the inspector package for tabs-visibility tests.cargo test -p chrome-remote-devtools-serverfor the inject payload test.Relates to #59