Repository navigation
refactor(react-devtools): reuse CLI output and Metro session ownership - #3694
Conversation
|
There was a problem hiding this comment.
All reported issues were addressed across 21 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Size Report
Startup median (7 runs, lower is better):
|
|
I reviewed f21ea8a. I found no blocking problems in the code, and all 22 checks pass at this head. There are no conflicts. Not blocking, and you can take or leave these: the checkpoint that Two Cubic threads still apply: the empty I did not inspect a transcript of the live RN/iOS run described in the PR body. I did not run either regression test on the base, so I infer from reading the base code that they fail there. I did not recount the production line delta. |
f21ea8a to
3b83321
Compare
|
The earlier findings on f21ea8a are fixed at 3b83321, and I found nothing that needs to change before merge. The patch matches what the earlier review accepted, and the new renderer-ownership test in react-devtools-runtime.test.ts covers the foreign-renderer case. I did not run tests, and I did not run live react-devtools inspect or profile against Metro at this head. Not blocking, and you can take or leave it: metroSessionHintsScope(config) now runs outside the guarded try at the prepare, open, close and reload sites. It can only throw if os.homedir() throws, so this is a small hardening note. If you want it, let the record and clear helpers resolve the scope inside their own try. The open inline threads that still apply are the scope outside the catch thread (same point as above) and the COMPONENT_KINDS prototype lookup thread (use Object.hasOwn or a Map; only hand-edited profiles reach it). These threads do not apply and can be resolved: renderName HOC splitting (old behavior, the move keeps the default), bundleUrl empty-string check (unchanged by this PR), 16 ms frame budget in cli-help.ts and frame budget in debugging-profiling.md (both wordings existed before this PR). Smoke Tests failed in the live iOS simulator fixture E2E, where simctl openurl agent-device-test-app:///snapshot-depth timed out inside the daemon's Apple open path. This diff does not touch that route, so the failure looks unrelated. No conflicts. Please re-run Smoke Tests before merge. |
3b83321 to
fe22e77
Compare
|
Addressed the earlier optional notes in fe22e77: the saved checkpoint explains incomplete prop inspection, successful enrichment replaces that note, command tests call the real CLI dispatcher and return its exit code, and an unconfirmed renderer failure no longer hides the owning renderer. Confirmed ownership still preserves that renderer's error. Also fixed both applicable inline findings and replied to all six threads; four describe unchanged behavior already present on main and are resolved with the base-code evidence. Rebased onto 97c2fb2. All affected local gates pass at fe22e77, including 1,353 related tests; the five-suite regressions went from 8 failed / 44 passed to 53 passed. The PR remains net-negative by 33 production TypeScript lines. Fresh CI is running on the new head. |
|
Code at fe22e77 looks good, and I found nothing new to fix. The Metro scope lookup now runs inside each helper's try block, and the new inherited-key checks and open/close test cover the earlier findings. Smoke Tests failed in smoke:regular-visible-depth-frontier on Of the six earlier threads, two are fixed at this head and four are base behavior that this PR only moves, so you can resolve all six: Metro scope in try and inherited keys are fixed. HOC stripping, empty bundle-url check, help wording and docs wording already existed on base. I did not run tests or live react-devtools inspect or profile against Metro at this head. The renderer-failure change is only checked against the fake runtime in node:vm. The open/close test relies on vi.spyOn intercepting resolveDaemonPaths through the module namespace, and I could not confirm that. Please check that the test fails without the fix. |
Summary
Follow-up to #3613, rebased onto
main. React inspection and profiling share CLI output, component metadata, backend inspection, and Metro session ownership. Raw profiles are saved atomically before optional prop inspection; interrupted captures retain an explanatory note that successful enrichment replaces.Review fixes keep Metro scope resolution inside its best-effort boundary, reject inherited component-kind keys, and continue past failures from renderers that cannot confirm ownership. Command tests now exercise the real CLI dispatcher and its exit code.
24 files; 33 fewer production TypeScript lines. No new dependencies or gate exceptions.
Validation
At
fe22e77668f9c5aee1f7ea204adb4662aaeb0077:pnpm check:affected --run: all runnable gates passed, including 1,353 related tests across 175 files.pnpm check:quickpassed.15adb4196efe22e77668f21ea8a: tree/find/inspect/errors and profile start/stop/report/commit, 8 commits and 480 expected memo updates. Live validation was not repeated for these review fixes.simctl openurltimeout in the snapshot-depth fixture.