Skip to content

refactor(react-devtools): reuse CLI output and Metro session ownership - #3694

Merged
thymikee merged 2 commits into
mainfrom
refactor/react-devtools-ownership
Oct 16, 2026
Merged

thymikee merged 2 commits into
mainfrom
refactor/react-devtools-ownership

Conversation

@thymikee

@thymikee thymikee commented Oct 15, 2026 •

Copy link
Copy Markdown
Member

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.
  • Focused DevTools/Metro suite: 157 tests passed; pnpm check:quick passed.
  • Review regressions, same five-suite command:
base 15adb4196e head fe22e77668
8 failed, 44 passed 53 passed
  • Earlier live RN 0.83.1/iOS 26.2 evidence belongs to f21ea8a: 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.
  • CI pending on the new head. The previous Smoke Tests failure was a simctl openurl timeout in the snapshot-depth fixture.

View guided diff Turn on auto-fix

@thymikee
thymikee added this pull request to stack #3695 October 15, 2026 19:22
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-16 08:56 UTC

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/cli/react-devtools/react-component.ts
Comment thread src/agent-device-client.ts Outdated
Comment thread src/metro/metro-session-hints.ts
Comment thread src/commands/schema/cli-help.ts
Comment thread website/docs/docs/debugging-profiling.md
Comment thread src/cli/react-devtools/react-component.ts Outdated
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.29 MB 5.29 MB -230 B
Package (unpacked) 5.29 MB 5.29 MB -230 B
Package (download) 1.59 MB 1.59 MB -138 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.8 ms 27.8 ms +0.1 ms
CLI --help 78.1 ms 77.6 ms -0.5 ms

@thymikee

Copy link
Copy Markdown
Member Author

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 react-profile-command.ts writes before describeChangedProps (https://github.com/callstack/agent-device/blob/f21ea8a/src/cli/react-devtools/react-profile-command.ts#L201) has no note that changed-prop values were not inspected, so a run killed during that step leaves a saved profile with no reason for the missing prop kinds, and a note in that checkpoint that the enriched rewrite replaces would fix it. commandHarness in react-devtools-command.fixtures.ts (https://github.com/callstack/agent-device/blob/f21ea8a/src/cli/react-devtools/react-devtools-command.fixtures.ts#L52) calls the subcommand and writeCliOutput directly with exit code 0, so it skips runReactDevtoolsCommand, the dispatcher that src/cli.ts uses; could it call that and return its exit code? describeProps now goes through inspectBackendElement (https://github.com/callstack/agent-device/blob/f21ea8a/src/cli/react-devtools/react-devtools-runtime.ts#L346), which stops at the first renderer whose inspectElement throws, where the old loop tried the next renderer, so a renderer without hasElementWithId that throws can now hide the owning renderer's props. Should the search continue when a renderer cannot confirm ownership, or is the new behavior intended?

Two Cubic threads still apply: the empty --bundle-url='' case at #3694 (comment) and the inherited-key lookup at #3694 (comment). Please fix them or dismiss them with a reason. These four do not apply to this PR, so please resolve them: #3694 (comment) (the HOC name split is a verbatim move from base; raise it on #3613 if wanted), #3694 (comment) (the session hints scope has no throw path), #3694 (comment) and #3694 (comment) (the "16 ms frame budget" wording predates this PR).

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.

Base automatically changed from feat/react-devtools-profiler to main October 16, 2026 06:04
@thymikee
thymikee force-pushed the refactor/react-devtools-ownership branch from f21ea8a to 3b83321 Compare October 16, 2026 06:04
@thymikee

Copy link
Copy Markdown
Member Author

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.

@thymikee
thymikee force-pushed the refactor/react-devtools-ownership branch from 3b83321 to fe22e77 Compare October 16, 2026 07:56
@thymikee

Copy link
Copy Markdown
Member Author

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.

@thymikee

Copy link
Copy Markdown
Member Author

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 xcrun simctl openurl (NSPOSIXErrorDomain code=60) in the daemon's Apple open path. This diff only touches the client's Metro hint helpers, the react-devtools CLI and the shared CLI output writer. Metro hints run after the daemon returns, so they cannot reach that call, and the previous head failed the same way. Please re-run Smoke Tests, as that is the only check left before merge. There are no conflicts.

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.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 16, 2026
@thymikee
thymikee merged commit cd16bd0 into main Oct 16, 2026
19 of 20 checks passed
@thymikee
thymikee deleted the refactor/react-devtools-ownership branch October 16, 2026 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant