mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
feat/android-atomic-clear
13 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 filip131311
|
9e45b98135 |
fix(profiler): measure CPU windows from the samples, not from the question (#685)
Fixes #619. ## Reproduced, then verified, on the reporter's own session Their artifacts were still on disk (`react-profiler-20260731-150333`: 1873 samples, 25114.0 ms, one hot commit at t=12307.5 ms), so every number below is from the session in the issue rather than a fixture. ## Two defects **Self-time was the question, not the answer.** `avgIntervalMs = (endMs - startMs) / totalSamples`, so self-time = *requested window width* × share of hits. Same samples, three windows, three answers: ``` [0, 25114] → createAnimatedNode 29.12 [0, 50000] → 2× [0, 100000] → 4× ``` **Samples were displaced by the first hot commit.** `clockOffsetMs = firstCommitTimestampMs - cpuStartMs`, applied when `|diff| > 1000`. `startTime` is a since-boot monotonic value (1_276_275_277_894 µs ≈ 14.8 days), so that guard is *always* satisfied and the offset was *always* the first hot commit — 12307.5 ms here, which is exactly how far every sample moved. **Together they predict the reported 2×**: displaced samples covered `25114.0 − 12307.5 = 12806.5` ms of a full-range query, so `25114.0 / 12806.5 = 1.961`. The reporter measured `completeRoot` 52.87 vs 26.82 — a ratio of 1.971. ## The fix **Clock: subtract, don't infer.** React DevTools emits every commit as `performance.now() - profilingStartTime`, so commit timestamps are already ms-since-profiling-start. Sample times are now the same thing — `(accumulatedUs - startTime) / 1000` — and `buildCpuSampleIndex` no longer accepts a commit timestamp at all, so the inference cannot come back. **Weights: each sample's own interval, clipped to the window.** A sample stands for the interval that *ended* at it, so a window cutting through one counts only the part inside. That gives four properties the old formula had none of: independent of the requested bounds, additive across adjoining windows, bounded by the window (a 45 ms commit can no longer be credited with more than 45 ms), and summing to the profile's own duration. Per-sample rather than averaged because the sampler is not isochronous — measured on the real profile, deltas run 0–36.6 ms around a 13.1 ms median. An average smears a stall evenly across every function in the window. *(A note on my own reasoning: I initially justified per-sample weighting with a synthetic fixture showing a 20× cost difference reported as a tie. That demonstrates the mechanism but overstates it — real Hermes sampling is roughly periodic, so the effect is a redistribution, not a rank inversion. The durable arguments are window-invariance and additivity.)* **Finding nothing now says which kind of nothing.** Outside the recorded range; inside it but unreached by any sample; a profile with no samples at all; and — the case that actually dominates — a window **fully covered by samples that were all idle**. 99% of samples in this session were idle, so a window can be completely covered and still rank nothing. Reporting that as "no CPU hotspots found" is what made a miss read as "this commit was cheap". Every table now states what it measured: sample count, covered ms, idle ms, and the sampling interval — plus a warning when the window is narrower than ~3 intervals, or when a sampler stall lands wholly on one function. **The on-disk index is versioned.** A file written before this change holds displaced timestamps and no interval starts; reusing one would answer from cache with exactly the numbers being removed. Readers reject it and rebuild from the raw profile, which is always retained. The same validation stops a truncated index deserializing into an empty profile that would answer "no hotspots" forever (`new Float64Array(undefined)` is silently empty). ## The reporter's five queries, after ``` 12300–12345 → recordProfilingDurations 15.05ms, completeRoot 4.86ms (was: nothing) 10000–15000 → 371 samples, real hotspots (was: nothing) 0–1000 → 71 samples, 1000.0ms covered, ALL IDLE — stated (was: nothing) 24000–25114 → 85 samples, 1113.1ms covered, ALL IDLE — stated (was: touch events from t≈12s) 0–25114 → 1872 samples, 25113.1ms covered, self-times correct (was: 2×) ``` Two remain empty — correctly, and they now say why. ## Tests 15 new cases in `test/react-profiler/cpu-correlate.test.ts`, all pure. There was **no test at all** for either file before, which is why this survived. Covered: window invariance across four widths, sub-window partitioning, boundedness by window width, additivity, per-interval weighting, rebasing a since-boot start time, coverage within one interval of the profile's reported duration, defensive deltas (missing/negative/NaN), the four empty-window shapes, and index round-trip plus v1/truncated rejection. Mutation-verified: restoring the window-derived average fails 8 of 15. Full tool-server suite green — 3101 passed / 298 files. ## Behaviour change — worth calling out in the release notes Numbers move in both directions. Windowed self-times fall by the ratio of the window to its sampled coverage; previously-empty windows now return data; and per-commit CPU blocks in **every** analyze report change wholesale, because they were reading a region of the profile the commit never occupied. **Any before/after optimisation baseline recorded with the current build must be re-measured.** ## Not in scope - **`time-align.ts:50-58` has the same family of bug** — `buildReactAnchor` sets `monotonicStartMs` from the since-boot `cpuProfileStartTimeUs` while commit timestamps are profile-relative, so commits are projected ~device-uptime into the past and **no hang can ever correlate with a commit**. It is invisible to the existing test because that test passes `cpuProfilePath: null`. Filing separately: fixing it makes `buildReactAnchor`'s second parameter and a `readCpuProfile` call dead, so it is not the one-liner it looks like. - `queryCpuWindow` emits one row per *node id*, so a function reached via several call paths is listed repeatedly with its cost split. Real, and moves numbers again — separate issue. - `component_cpu` applies its top-50 cut per window *before* aggregating, truncating the long tail. - `renderCallTree` and `react-profiler-cpu-summary` still derive self-time their own ways; unifying them onto these weights is a follow-up. - Hermes ignores the requested `sample_interval_us` (100 µs requested, ~13.4 ms observed). Not argent's bug, but it is why no fix may derive an interval from the requested config. |
||
|
filip131311
|
98fe7a86fc |
test(profiler): type component-names tests with the module's exported types (#771)
Unbreaks the **Dead Code** gate on main, red since #663 merged. ## What happened #663 branched before the knip workflow existed (#572), so its own PR checks never ran the gate. Its `component-names.ts` exports three types with no importers — `StrippedName`, `ComponentAnnotation`, `ComponentNameResolution` — which pushed the knip second pass to **218** parked issues against the **215** ceiling. Every open PR now inherits the red check (e.g. #687, whose own commit adds no parked items). Local runs hid this: counted the way `knip.jsonc` prescribes — against a tree with **no build output** — the failure reproduces exactly. ## The fix Import the types where the tests naturally need them, which also removes a wart: the two `renderComponentNameMiss(r as never)` casts existed precisely because the narrowing `ComponentNameResolution` enables was never written. The new `expectMiss` helper narrows properly and fails loudly if the resolver ever stops returning a miss. No ceiling change: 47 + 156 + 12 = **215**, exactly at the limit. Note the budget now has zero headroom — the next PR that parks an export will trip it, which is the gate working as designed. ## Checks - clean-tree `npm run knip` exit 0 (both passes) - component-names tests 18/18, tool-server test typecheck clean, prettier clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) |
||
|
filip131311
|
f136c024cc |
fix(profiler): accept the component names the analyze report actually prints (#663)
Fixes #632. ## The problem `react-profiler-analyze` strips `Forget()` / `Memo()` / `ForwardRef()` wrappers for readability, then the drill-down tools matched the raw DevTools name with `===`. The report names a component and the very next call refuses it. React Compiler is on by default in Expo SDK 54, so this hits essentially every new Expo app. ## Reproduced live Real session on an Expo SDK 54 app (`experiments.reactCompiler: true`, RN 0.81.5) — 821 fiber renders, 16 commits. The report printed `` `ParallaxScrollView` [React Compiler] ``: ``` profiler-cpu-query component_cpu ParallaxScrollView → "_Component `ParallaxScrollView` not found in commit data._" profiler-cpu-query component_cpu Forget(ParallaxScrollView) → "_No CPU samples found during `Forget(...)` commits._" ``` The two messages differ, and that is the proof: "not found in commit data" means the name never matched; the other is emitted only *after* a match. **Two things beyond the report:** 1. **`profiler-commit-query mode=by_component` has the same bug** — same refusal, full table under the raw name. 2. **It is not React-Compiler-specific.** `Memo()` and `ForwardRef()` are stripped too; the same report printed `` `InnerScreen` [forwardRef] ``. ## The workflow was a deadlock The AST index is keyed on bare source identifiers, so `react-profiler-component-source` accepts only the name the query tools reject: | name | query tools | component-source | |---|---|---| | `IconSymbol` | ✗ not found | ✓ file + line + source | | `Forget(IconSymbol)` | ✓ resolves | ✗ not found | There was **no single name an agent could carry** from analyze → drill-down → read source. Fixing only the query half would have left the issue's stated workflow broken. ## Approach Match a query against each recorded name's **display form**, rather than stripping both sides. Stripping both sides looks equivalent and is not: - it would quietly answer a query for `Forget(Foo)` with `Memo(Foo)` when only the latter exists — the agent asked about the compiler-wrapped fiber and silently got the memo one; - it isn't even reliable, because the display stripper stops after 4 wrappers, so a deeply wrapped name still *displays* with a wrapper and would never match. Comparing against the display form compares against exactly the string that was printed. **Exact matches always win and are never widened.** `StaticContainer` and `Memo(StaticContainer)` both appear in one ordinary 18-second session — I checked all 70 distinct names in the dump, and that is the one real collision. Asking for the bare name returns the bare fiber, and the output now names the sibling instead of silently showing one of two. **Ambiguity is refused, not merged.** Where a display name maps to several distinct fibers the tools list the exact strings to retry with. `Memo(X)` and `X` are often one component's two fibers whose durations nest, so a merged total describes no real component; and picking the largest silently picks the memo wrapper's inclusive time, which is the wrong fiber for self-cost analysis. The AST lookups get the reverse fallback: raw → stripped → expo-router file suffix removed (the report shows `TabTwoScreen(./(tabs)/explore.tsx)`; the source declares `TabTwoScreen`). Loosening is safe there in a way it is not for commit data — a wrong guess against the index simply misses and returns `found: false`. ## Verified against the live session | Case | Result | |---|---| | `component_cpu ParallaxScrollView` | resolves to `Forget(ParallaxScrollView)` — was refused | | `by_component ParallaxScrollView` | full table + `> Resolved … (React Compiler). Either name works here.` | | `by_component Forget(ParallaxScrollView)` | **byte-identical to before**, no note | | `by_component ParallaxScroll` (typo) | miss + `Forget(ParallaxScrollView)` as a re-queryable suggestion | | `component-source Forget(IconSymbol)` | `found: true`, `icon-symbol.tsx:28` — was `found: false` | | `component-source Forget(TabTwoScreen(./(tabs)/explore.tsx))` | `found: true`, `app/(tabs)/explore.tsx:12` | ## Visible report change (intentional) Fixing the analyze-side lookup means wrapped components stop losing their source location. In the live session exactly one row changes — every other row is a node_modules component with no user source: ``` before: | `IconSymbol` [React Compiler] | 6 | 0.1ms | … | — | after: | `IconSymbol` [React Compiler] | 6 | 0.1ms | … | `ui/icon-symbol.tsx:28` | ``` ## Deliberately out of scope - **`profiler-combined-report.ts:448`** correlates leaks to components by substring-matching raw names into native symbols, so wrapped components are structurally excluded. Same bug class — *affected, deferred*, not unaffected. - **A deeper namespace bug, filed separately:** `react-profiler-stop` joins fiber metadata by name across two different namespaces, so wrapped fibers lose `parentName`, `hookTypes` and `isCompilerOptimized` entirely. Measured on this session: **41 wrapped fibers, 0 with any of the three**, versus 181/272 bare fibers carrying `parentName`. That is why the report's header says `React Compiler: ✗` while the same document tags six components `[React Compiler]`. ## Checks - 3104 tests pass (298 files); 18 new, built on names taken verbatim from the real session - the `annotateComponentName` move is a pure refactor — verified output-identical before anything else changed - `03-tag.ts`'s `ANIMATED_PATTERN` deliberately left alone (it matches unanchored and covers `Motion`/`Transition`, which the stripper must not) - tool `description:` fields unchanged, so the SpiderShield average is untouched; skills gate 10.0; prettier, eslint, both typechecks clean --- > **#687 is stacked on this branch** — it conflicts with this one in `profiler-commit-query.ts`'s imports, so it was rebased on top. Merge this first. |
||
|
Hubert Gancarczyk
|
afd722924f |
feat(flow): support concurrent flow recordings (#574)
## Problem
Only one flow could be recorded at a time, anywhere on the machine.
Recording state was three module globals in `flow-utils.ts` — harmless
in a per-client process, but the tool-server is a **host-wide singleton
per install bundle**. Every MCP client, every subagent sharing a
parent's connection, every `argent` CLI call and every project on the
machine attaches to the same process, the same `Registry`, and those
same three globals. HTTP requests are served concurrently with no
serialization.
Three failures followed, all reproduced against the pre-change code:
1. **A second recording clobbered the first.** `startRecordingSession`
just overwrote, so the first agent's next `flow-add-step` silently
appended into the *other* agent's flow file — in a different project.
2. **Any agent could finish any agent's recording.**
`flow-finish-recording`'s schema was literally `z.object({})`.
3. **Replay stomped recording.** `resolveFlowFilePath` called
`setActiveProjectRoot`, so a `flow-execute` in project B rebound the
global root mid-recording of project A. Already worked around, locally
only, in `flow-add-step`.
## Approach
Recording state is now a `Map` keyed by the resolved flow file path,
`<project_root>/.argent/flows/<name>.yaml` — the identity of the
artifact being built, so "same key" means "same output file" and two
sessions on one key are a genuine collision rather than an accident of
scoping. `flow-add-step`, `flow-add-echo` and `flow-finish-recording`
each take `name` + `project_root`, so every call is self-contained;
required rather than optional-with-fallback, since a fallback would
succeed while an agent is alone and fail only once a second recording
exists.
`resolveFlowFilePath` is pure, which fixes defect 3 and retires
`FLOW_PROJECT_ROOT_REQUIRED` (the state it guarded can no longer exist).
Rejected alternatives — keying by device udid (flows are deliberately
device-portable, and `flow-add-echo`/`flow-finish-recording` have no
device at all), an opaque `recordingId` (an agent that loses the token
cannot recover), and per-caller identity from the transport
(`ToolContext` carries no caller identity, and subagents share their
parent's MCP connection, so it would fail the exact scenario being
fixed).
### Locking, and why readers needed more than a lock
Every mutation of one flow file runs under a lock keyed by that path —
appends, `flow-start-recording`'s truncate-and-register, and
`flow-finish-recording`'s read-summarize-clear. Keyed by file rather
than owned by the session, because a restart *replaces* the session: a
session-owned lock cannot exclude the operation that supersedes it. Per
file, not global, so two recordings never queue behind each other. An
append whose session was finished, restarted or evicted meanwhile fails
with `FLOW_NO_ACTIVE_RECORDING` rather than writing into a different
take.
The lock serializes writers only, and no reader of a flow YAML can join
it — `flow-execute`'s own load, its `run:` fragment load,
`flow-read-prerequisite`, `flow-add-step`'s sibling-fragment check, and
the `argent` CLI reading from another process entirely, where an
in-process lock cannot reach. Both writers used a plain `fs.writeFile`,
which opens `O_TRUNC`, so a reader could land between the truncate and
the write. That case is silent: `parseFlow("")` returns `{ steps: [] }`
with no error and `summarize` derives `ok` from "no failures", so a
`flow-execute` racing an append reports a **top-level PASS having
replayed zero steps**. Both writes now go through a sibling temp file
and a rename, so a reader sees either the whole old file or the whole
new one — and unlike the lock, that also holds cross-process.
### `stop-all-simulator-servers` gets a `devices` scope
Every agent is told to call this at session end, and unscoped it walked
the whole registry — so agent A wrapping up tore down agent B's devtools
mid-recording, degrading B's flow to brittle coordinate taps. It now
takes an optional `devices` scope and reports `unmatched` ids, so a
mistyped id doesn't read as a clean machine.
Which namespaces count as "owned by a device" turned out to be the
load-bearing part, because nothing cascades to most of them and
`unmatched` turns any omission into an actively wrong diagnosis — a
correct device id reported as a typo. The set now covers `AXService`
(the in-sim ax daemon, spawned `--timeout 3600`; an iOS session that
only ran boot/launch/describe owns this and nothing else),
`ScreenRecordingSession` (an ffmpeg child plus the touch-visualizer
overlay it enabled on the device), `NativeProfilerSession` (an xctrace
child, or an on-device perfetto process and its trace file) and
`JsRuntimeDebugger` (a bound loopback server, the CDP socket to Metro, a
log handle). The debugger URNs interpose the Metro port —
`<ns>:<port>:<deviceId>` — so the matcher understands both shapes,
consuming only the first colon so a wireless adb serial after the port
still compares whole.
`stop-simulator-server` shared none of this and had drifted: it looked
URNs up with an exact, case-sensitive `services.get()`, so a lower-cased
UDID silently no-op'd there while the scoped stop-all reaped it. Both
tools now share one matcher. Their namespace *sets* stay deliberately
different, documented where they are defined — `stop-simulator-server`
is also the documented recovery for a wedged transport, and widening it
to devtools/AX would make a routine retry drop the native-devtools
connection another agent's recording depends on, which is the hazard the
`devices` scope exists to prevent.
## Verification
**End-to-end, three devices, one shared tool-server** (iPhone 16 Pro,
iPhone 17 Pro, Pixel 3a): three interleaved recordings, plus an
unrelated `flow-execute` in a fourth project while all three were live.
Both iOS devices held their own live native-devtools concurrently (two
per-UDID sockets, `connected: true`). Each YAML contained exactly its
own steps in order, taps captured as selectors rather than coordinates,
and neither picked up another's or the third root's path. All three
replayed green — also through the **released 0.17.0 CLI** against the
modified server, which checks wire compatibility. A device-scoped
teardown of sim 1 left sim 2 and the emulator running.
Re-run against the final build:
- Two interleaved recordings across two project roots on one device,
with a **separate process** polling one of the flow files as fast as it
could for the duration. Zero empty or truncated observations; each YAML
held exactly its own steps; both replayed green (6/6 and 5/5). Against
the pre-change in-place write the same reader observes zero-step reads
interleaved among the real ones — the silent-PASS input.
- A scoped teardown of the iPhone reaped `AXService` and
`NativeDevtools` for that UDID only, reported no `unmatched`, and left
the second sim and the emulator running. Lower-casing the UDID matched;
a genuinely bogus id was still reported.
- With a screen recording live on the Pixel 3a, a scoped teardown reaped
`ScreenRecordingSession` and the **ffmpeg child actually exited** (1 →
0). Before the fix it survived to the 180 s cap while the tool called
the correct serial a typo.
The pre-change repros were re-run afterwards to confirm observed
behavior changed, not just the tests.
`npx vitest run` in `packages/tool-server` (295 files, 3154 tests) and
`packages/argent-cli` (280), `tsc --build`, `tsc --noEmit` on both test
projects, eslint and prettier — all green.
## Review notes
Several rounds of review found real defects in this change; each was
reproduced before fixing and re-verified after. Worth knowing when
reading the diff, since the tests exist to pin them:
- `flow-start-recording` truncated the `.yaml` outside any lock, so a
step from the take being discarded landed in the freshly reset file and
was reported as success (26-29 of 200 runs).
- `flow-finish-recording` never took the lock; its `await fs.readFile`
was a yield during which a concurrent append committed, leaving the
reported summary disagreeing with disk (11-16 of 200).
- A failed finish destroyed the recording. Hand-editing the `.yaml`
mid-recording is a documented workflow, so `parseFlow` can legitimately
throw — and the error told the agent to call `flow-start-recording`,
which truncates the very take it would recover.
- `unmatched` reported a device this session had just stopped, because
`disposeService` returns a node to IDLE and keeps it.
- A bare IP claimed every wireless-adb device at that address, since an
adb serial is itself `ip:port`.
- The eviction backstop stamped with `Date.now()`; ties at millisecond
resolution let it drop the session that was just used.
- `requireRecordingSession` ended with "Call flow-start-recording first"
— reached for a key that was finished, superseded or evicted as well as
one never started, and on those branches the file on disk is fully
populated while that call truncates it and reports no `restarted`.
- The atomic write regressed twice on its own terms: its scratch name
was derived from the flow file's basename, which has no length cap, so a
long flow name that appended fine before failed `ENAMETOOLONG`; and the
cleanup guard began after the temp file was created, leaking a scratch
file into a committed directory on any write that failed after opening.
- Two tests passed against implementations they claimed to reject: the
`stop-simulator-server` narrowness guard used a udid that classifies as
*android*, where the iOS-only services it guards can never appear, so
widening the iOS branch kept it green; and the client-mode finish
asserted only the *shape* of `savedTo`, never its content, though that
directive is the only thing that lands the file in client mode.
## Notes for reviewers
- Client/remote persist mode is the least-exercised path, and now has
its own concurrency tests (overlapping appends, and a restart
superseding an in-flight one) — it previously had none. The
`project_root` key is byte-stable across the file boundary (`kind:
"probe"` passes it through unchanged), which is what keeps a remote
recording addressable from `flow-add-step`, which declares no file
input.
- Two spellings of one flow file still mint two sessions — and two
independent locks — over one file, which bypasses every guarantee here.
Two ways in: the root spelled two ways (a symlink, or a case variant on
APFS), and the likelier one, the *name* cased two ways on a
case-insensitive volume. Neither is normalized away because the correct
normalization is the filesystem's: case-folding the key would wrongly
merge two distinct flows on ext4, and resolving symlinks is impossible
in client mode, where the root does not exist on this host. Documented
at `getFlowPath`.
- No recording tool can detect that another agent took its key.
`flow-add-step`, `flow-add-echo` and `flow-finish-recording` all
re-resolve the key on every call, so after a takeover they act on
whichever take is now live and report success: an append lands in the
other agent's file, and a finish finishes and clears it. The only case
that fails loudly is the narrow one the liveness check covers - an
append already IN FLIGHT when the restart lands. There is no caller
identity to key ownership on (see the rejected alternatives above), so
this is spelled out for the agent under "Pick a name unique to your
task" in `argent-create-flow` rather than mechanized.
- Skew hazard in one direction only: a *new* client against an *older*
tool-server. Zod strips the undeclared `name`/`project_root`, and the
old server falls back to its module globals — so concurrent recordings
silently collapse onto one session. The other direction (released 0.17.0
CLI against this server) was verified end-to-end.
- `packages/argent-private/docs/reference.md` still describes
`flow-add-step` as appending to "the current recording", and names two
tool ids that do not exist (`flow-insert-echo`/`flow-run` are file
names; the ids are `flow-add-echo`/`flow-execute`). It is a submodule,
so the fix cannot ride on this branch.
- A restart reported `discardedSteps` from the superseded session's
in-memory flow while it truncates the *file*. Hand-editing the `.yaml`
mid-recording is documented, so the two diverge: four steps wiped, one
reported. It now counts the file, and reports no number at all when that
file cannot be read or parsed - 0 is the answer a genuinely empty take
gives. Client mode still counts from memory (this host cannot see the
client's file), and the description now says which mode it is promising.
- `ChromiumJsRuntimeDebugger` cascades from `ChromiumCdp` and was
therefore already being torn down - silently, while `NetworkInspector`
and `ReactProfilerSession` cascade the same way and were named.
`stopped` is documented as the services that were live and got shut
down, so it is listed now too.
- The client-mode case named for a superseded in-flight append restarted
*between* calls, so the next append re-resolved the key and succeeded -
the guard was never reached, and neutering it left the whole file green.
It now parks an append in its live sub-tool call across the restart.
- `stop-tools.test.ts`' mock handed the tool the live service map where
the real `getSnapshot` copies, so a cascade rewrote the state the sweep
was still reading and the answer depended on map insertion order. The
mock copies and recurses like `_teardown` now, and the cascade case runs
under both orders.
- Prose the code contradicted, each verified against source before
rewriting: Vega owns `JsRuntimeDebugger`/`NetworkInspector` once the
debugger has run (and can match anything at all through an ERROR node);
"nothing here cascades" was denied twice in its own file; an iOS
boot/launch/describe session owns `NativeDevtools` too; `:tcp` is
mintable but unreached in production; the tool-server is a singleton
*per install bundle*, not per machine; `argent flow list` does enumerate
`.argent/flows`; the stop-tools comments narrated a "before" that only
existed on this branch.
Found while verifying, out of scope, worth their own issues: a `tool:
flow-execute` self-reference recurses past both cycle guards (`run:` is
guarded, a nested `flow-execute` reseeds the stack); react-profiler
artifacts collide host-wide on a second-resolution filename;
`stop-metro` is port-scoped and kills another agent's bundler;
`debugger-reload-metro`'s HTTP fallback broadcasts to every attached
client. A tool's `capability` is enforced only by the HTTP layer, not by
`registry.invokeTool`, so a call that reaches the registry another way -
`flow-add-step` takes `command` as a bare string - can resolve a service
the tool declares it does not support, leaving an ERROR node behind
(e.g. `SimulatorServer:<vega serial>`).
---------
Co-authored-by: Hubert Gancarczyk <claude-hubert.gancarczyk@swmansion.com>
|
||
|
Ignacy Łątka
|
abcbce991e |
fix(profiler): keep all same-named components and surface platform variants (#368)
## Problem The AST component index (`buildAstIndexWithDiagnostics`) is keyed by component **name**. When two files define a component with the same name — most commonly platform variants, e.g. `List` in `List.tsx` and `List` in `List.web.tsx` — the index kept whichever file the directory walk happened to reach **first** (`if (!index.has(name))`) and threw the rest away. So a lookup for `List` could return the wrong platform variant's source, with no signal that other matches even existed. And because it depended on filesystem walk order, *which* `List` survived was effectively arbitrary. Raised in review on #287. ## Fix - **Collect every match per name** instead of keeping the first walked. - **Choose a deterministic primary**: base files are preferred over `.web` / `.ios` / `.android` / `.native` variants (those only override the base when bundling for that platform), then ties break on path → line → column. The result no longer depends on directory-walk order. - **Surface the rest** as `otherMatches` on the index entry. `react-profiler-component-source` now returns `otherMatches: [{ file, line, col }]` alongside the primary, so a caller asking for `List` can tell the locations apart instead of trusting a single arbitrary file. `react-profiler-analyze` needs no change — it enriches findings with the (now deterministic) primary location, which is strictly better than before; surfacing per-finding alternates there is out of scope. ## Tests `packages/tool-server/test/react-profiler/ast-index.test.ts` (index layer) and a new `component-source-duplicates.test.ts` (end-to-end through the tool's `execute`): - same name in `List.tsx` + `List.web.tsx` → primary is the base file, its source is returned, `List.web.tsx` is under `otherMatches`; - only platform variants (`.android` + `.ios`, no base) → deterministic primary by path; - uniquely-named component → no `otherMatches`. The two collision assertions fail on pre-fix code (no `otherMatches`, arbitrary primary) and pass with the fix. Verified locally with Node 20: `vitest run test/react-profiler/` — 80 passed; `tsc --build` clean; prettier + eslint clean. |
||
|
Ignacy Łątka
|
fa5af59931 |
refactor: replace hand-rolled parsers with standard packages (#373)
## What & why
Swept the repo for hand-rolled regex/string parsers that reimplement a
solved problem, and replaced the genuinely fragile ones with
battle-tested packages (or deps already in the tree). Each swap is
paired with a regression test for an input the old code got wrong.
| Site | Was | Now |
|------|-----|-----|
| `adb.ts` `resolveAvdPath`, `linux-preflight.ts` `readMb` | per-key
anchored regex over AVD `.ini` / `config.ini` | **`ini`** |
| `workspace-reader.ts` `extractEnvKeys` | split-on-first-`=` line loop
| **`dotenv`** (`parse`) |
| `uninstall.ts` `readBundledSkillName` | `^name:(.+)$` capture +
outer-quote strip | **`yaml`** (already a dep) |
| `mcp-configs.ts` `escapeRegExp` | hand-rolled char-class escape |
**`escape-string-regexp`** |
| `export.ts` xctrace version, `session-context.ts` RN minor |
`match(/(\d+)\./)` / `split(".")[1]` | **`semver`** `coerce` (already a
dep) |
| `source-resolver.ts` `parseDebugStack` | `at fn (file:l:c)` regex
(required both line+col) | **`stacktrace-parser`** |
### Bugs these fix (covered by new tests)
- **ini**: a quoted `path = "/Users/My Name/.../X.avd"` kept its quotes,
failed the `startsWith("/")` guard → AVD unresolved. Inline comments /
trailing junk after a sizing value made the anchored regex return `null`
→ sizing warning silently lost.
- **dotenv**: `export FOO=bar` produced the key `"export FOO"` (dropped
by the identifier filter); a multi-line quoted value leaked its
continuation lines (`FAKE=...`) as phantom keys.
- **yaml**: a `name:` value with a trailing `# comment` was taken
verbatim (comment included); odd quoting only had one outer pair
stripped.
- **stacktrace-parser**: a `:line`-only frame (no column, as Hermes/JSC
can emit) became an unusable `unknown` frame.
## Deliberately left alone (not "flaky", or no safe swap)
To avoid churn and regressions, I did **not** touch:
- **`uiautomator-parser.ts`** hand-rolled XML parser — a deliberate,
documented choice with a dedicated hardening suite, including a
*leniency* test for malformed dumps (stray `</node>`). A spec XML parser
would reject inputs this is built to recover from.
- **`ios-profiler/pipeline/xml-parser.ts`** (xctrace XML) — the most
tangled parser, but it has no real `.trace` fixtures and only thin
entity-decode tests. A rewrite can't be proven equivalent without
device-generated traces, so swapping it blind risks silently breaking
iOS CPU/hang/leak reports. Best done separately with a captured fixture.
- **`http.ts` `extractHostname`** — DNS-rebinding guard; `URL.hostname`
keeps the IPv6 brackets the loopback check compares without, so the
"fix" isn't simpler and the path is security-sensitive.
- **`adb.ts` `shellQuote`** — the POSIX single-quote escape is the
standard correct form.
- **`parseAdbDevices`, `android-screen.ts` `wm size`, `detect-app.ts`
dumpsys** — tool-specific CLI-output scraping with no clean package
equivalent (and documented edge-case handling).
- **`workspace-reader.ts` `extractMetroPort`** — a best-effort heuristic
over JS config; a correct replacement needs an AST parse, out of scope
here.
- **`config-parse.ts` Yarn/npm age-gate parsing** — a precise,
unit-tested domain parser; `ms` would introduce a bare-number ambiguity.
## Verification
- `tsc --build` clean across the workspace graph.
- Tests: tool-server **1205**, installer **264**, update-core **31** —
all pass (incl. the new regression tests).
- `prettier --check` clean on all changed files.
- Production esbuild bundle (`bundle-tools.cjs`) emits a valid
`tool-server.cjs`/`mcp-server.mjs` with
`ini`/`dotenv`/`stacktrace-parser` inlined (`node --check` OK). *(The
script's final step errored only on the auth-gated
`trace-processor.wasm` download, unrelated to bundling.)*
New runtime deps: `ini`, `dotenv`, `stacktrace-parser` (tool-server),
`escape-string-regexp` (installer) — all small, pure-JS. Lockfile change
is purely additive.
|
||
|
Ignacy Łątka
|
d987c25ad0 |
fix(argent): declare + ship tree-sitter so react-profiler-component-source finds components (#287)
## Problem `react-profiler-component-source` returned `found:false` for **every** component in a standard TS/TSX Expo project. ## Two root causes (both verified live against the Bluesky app) 1. **tree-sitter never declared/shipped.** `@swmansion/argent` didn't depend on `tree-sitter`/`tree-sitter-typescript`, so the parser `require()` threw, was swallowed, and the AST index was always empty. Fixed by adding them as runtime deps and keeping them `external` in the CJS tools bundle so the native addons are `require()`d at runtime (not inlined). Verified: `HostingProvider`, `LoggedOutLayout`, `Login` now resolve with correct file:line. 2. **memo()/forwardRef() components were skipped.** The AST matcher's `variable_declarator` branch only indexed `arrow_function`/`function_expression` value nodes; a wrapped component (`const X = memo(...)` / `forwardRef(...)` / `React.memo(...)`) has a `call_expression` value node, so it was never indexed — and profiler findings are disproportionately memo-wrapped, i.e. exactly the components users look up. Now recognises memo/forwardRef wrapper calls (incl. `React.`-prefixed and nested `memo(forwardRef(...))`) and indexes the declared name; `memo` sets `isMemoized`. Verified live: `ThreadItemReadMore`, `CenteredView`, `ScrollView`, `BookmarkButton`, `Logo` now resolve. ## Tests `ast-index.test.ts` covers the idiomatic forms (default-export/plain/arrow) and the memo/forwardRef/React.memo/nested wrappers; the wrapped-form test fails on pre-fix code. Rebased onto latest main (re-merged the `bundle-tools.cjs` helper refactor; bundle still externalizes tree-sitter). CI green. |
||
|
Paweł Fornagiel
|
5b147e9c25 |
fix: profiler attribution on older React DevTools backends (#262)
## Summary - Resolve profiler display-name lookup across both `getDisplayNameForElementID` and legacy `getDisplayNameForFiberID` backends. - Add a fallback diagnostic (`displayNameApiAvailable` / `unattributed_reason`) so fully-unattributed captures are reported as an API-surface mismatch instead of a transient lifecycle race. - Extend multi-renderer script coverage for modern, legacy, mixed, and missing display-name accessor scenarios. |
||
|
Ignacy Łątka
|
2180dd8e6a |
ci: typecheck test files in CI (#202)
We've had lots of type errors in our tests, scripts and other non-core
code despite all CI being green. This PR fixes that.
This PR also removes some redundant features like `@ts-check` inline
directives which have been replaced by proper ts config files.
<details>
## AI Summary
Audit of every CI workflow against every package's `package.json` and
`tsconfig` surfaced several gaps where CI did partial coverage. This PR
closes them and fixes every pre-existing type error the new gates
surfaced.
## Gaps closed
| # | Gap | Fix |
|---|---|---|
| 1 | `tsc --build` only covers `src/**`; test files (`test/**`,
`tests/**`) never typecheck. Vitest transforms tests with esbuild —
strips types instead of checking them. | New `tsconfig.test.json` per
test-bearing package (`composite: false`, `noEmit`, `rootDir: "."`,
includes `src/**` + `test/**` (or `tests/**`) + `vitest.config.ts`). New
`typecheck:tests` npm script per package. New CI step `npm run
typecheck:tests --workspaces --if-present`. |
| 2 | Build/publish scripts (`scripts/*.{cjs,mjs}`,
`packages/argent/scripts/*.cjs`, `packages/skills/scripts/install.js`) —
JS, never typechecked. `sync-readme.cjs` runs in `prepack`;
`postinstall.cjs` runs on every install; `bundle-tools.cjs` is
build-critical. A typo only surfaces at runtime. | Root
`tsconfig.scripts.json` with `allowJs` + `checkJs` + `noImplicitReturns`
covers all script paths. Root `typecheck:scripts` npm script. New CI
step. Per-file `// @ts-check` comments removed — tsconfig drives
inclusion. |
| 3 | `argent-cli` had no `typecheck:tests` script. When tests get added
there they'd silently bypass the gate. | Added `tsconfig.test.json` +
`typecheck:tests` script. `--if-present` would have skipped it anyway,
so this is future-proofing. |
| 4 | Local `prettier --check .` walked into the `argent-private`
submodule and reported violations there. CI didn't see this because
`actions/checkout@v4` doesn't init submodules — so local `prettier
--check` did not match CI behavior. | New `.prettierignore` excludes the
submodule plus `dist/`, `node_modules/`, lock files, `tsbuildinfo`.
No-op in CI; aligns local with remote. |
## Test type errors fixed
These were already present. The `npm test` passed because esbuild
stripped the offending types:
- **argent-installer**: widen `scope` literal type for
dead-code-elimination guard; non-null `addAllowlist!`/`removeAllowlist!`
(optional methods on adapter).
- **argent-mcp**: add `.js` extensions for NodeNext module resolution.
- **registry**: broaden `StaticBlueprintResult` blueprint api from `{
id; deps? }` to `Record<string, unknown>` so existing tests can pass
arbitrary api shapes.
- **tool-server**:
- non-null `zodSchema!` on tool defs that always carry one
(`launchAppTool`, `restartAppTool`, etc.)
- cast sentinel `"ignored"` strings to `DeviceInfo` in factory-rejection
tests
- match the 4-arg `dispatchByPlatform<IosServices, AndroidServices,
Params, Result>` signature (tests still passed the old 3-arg shape)
- `assertFlowRunResult` type guard for the `FlowRunResult |
FlowPrerequisiteNotice` union before `.steps` access
- swap broken `typeof import("supertest").default` for static `import
supertest` (supertest is `export = supertest`)
- fill in the 4 fields missing from a `NativeProfilerSessionApi` mock
(`xctraceProcess`, `recordingTimedOut`, `recordingExitedUnexpectedly`,
`lastExitInfo`)
- cast a vitest `Mock` to a plain function at one direct call site where
the `Procedure | Constructable` union loses callability
- fix `update-checker.test.ts` reading `../../package.json` (off-by-one
— went past tool-server; was relying on a Vite resolver quirk)
- replace one cross-package `../../../registry/src/index` import with
`@argent/registry` so registry's private `services` field isn't compared
between src and dist declarations
All fixes are type-level. Full `npm test --workspaces --if-present`
still reports **991 passed across 87 test files**.
</details>
|
||
|
Paweł Fornagiel
|
8a3b5a1ff7 |
fix(react-profiler): self-bootstrap DevTools backend when no client is attached (#205)
## Summary `react-profiler-start` failed in any RN bridgeless dev build with no external React DevTools client (Fusebox React tab / `npx react-devtools`) connected — returning the misleading error *"No React renderer interface attached yet. Wait for the app to render its first commit and retry."* even when the app had rendered. This PR makes the tool self-bootstrap the DevTools backend so profiling works out of the box, and replaces the one-size-fits-all error with mode-specific actionable messages. Companion to #199 (schema fix). With both landed, `react-profiler-start` is end-to-end usable from MCP clients against a vanilla RN 0.76 dev app with zero extra setup. > [!IMPORTANT] > The bootstrap path mutates global runtime state (Bridge + Agent + hook subscriptions via `connectWithCustomMessagingProtocol`). Unit tests cover every reason branch, but the live behavior — especially coexistence with a later-attached `npx react-devtools` client and the absence of subscription leaks across repeated start/stop cycles — must be **smoke-tested against a working RN application** before this merges. ## Root cause argent's profiler delegates React commit capture to the in-app DevTools backend via `rendererInterface.startProfiling(...)`. The two maps on `__REACT_DEVTOOLS_GLOBAL_HOOK__` look similar but are populated by different actors: | Map | Populated by | When | |---|---|---| | `hook.renderers` | React itself via `hook.inject(renderer)` | First render | | `hook.rendererInterfaces` | DevTools backend's `attach(hook, id, renderer, global)` (in `react-devtools-shared/src/backend/renderer.js`) | When `initBackend(hook, agent, global)` runs | In an RN 0.76 bridgeless dev build, `setUpReactDevTools.js` runs at bundle load and calls `react-devtools-core.connectToDevTools({ host: 'localhost', port: 8097, websocket })`. That opens a WebSocket; `initBackend` only runs on `ws.onopen`. With no DevTools client listening on 8097, the connection never opens, `initBackend` never runs, `rendererInterfaces` stays empty — but `hook.renderers` is populated because React doesn't care about the backend. So `rendererInterfaces.size === 0` while `renderers.size === 2` is the steady-state of a dev build with no DevTools client. argent's pre-flight check saw the empty map and threw with a misleading message ("wait for first commit") that conflated three distinct failure modes: 1. App hasn't rendered yet (legitimate wait). 2. App rendered but no DevTools backend attached (this PR — the common case). 3. Production build / no `react-devtools-core` in bundle (different recovery). Direct CDP verification of the hook from the bug state: ```js { rIds: [1, 2], riIds: [], reactDevtoolsAgent: undefined } ``` ## Fix When the pre-flight detects `rendererInterfaceFound: false` with the hook present, `react-profiler-start` now runs a self-bootstrap script (`BOOTSTRAP_DEVTOOLS_BACKEND_SCRIPT`) that: 1. **Looks up `react-devtools-core` in the Metro module registry** by scanning `__r.getModules()` entries for a `verboseName` matching `react-devtools-core/dist/backend`. Handles both `Map<id, meta>` (modern Metro) and array-of-meta (older Metro) shapes — mirrors the pattern in `debugger-component-tree`. 2. **Calls `connectWithCustomMessagingProtocol`** (rdt-core ≥5.1) with no-op `onSubscribe` / `onUnsubscribe` / `onMessage` handlers. The function internally constructs a `Bridge` + `Agent` and runs `initBackend(hook, agent, window)`, which iterates `hook.renderers` and calls `attach()` on each — exactly the call path that populates `rendererInterfaces`. The fake wall discards every outbound message, which is fine because argent calls into the rendererInterface directly and never reads from the bridge. 3. **Returns one of seven distinct reasons**, each mapping (via `bootstrapFailureMessage` in `utils/react-profiler/devtools-bootstrap.ts`) to its own actionable error message: - `no-hook` → "production build" guidance - `no-renderers` → "wait for first commit" - `no-metro-modules` → bundle isn't Metro-style - `no-rdt-module` → react-devtools-core stripped from bundle (production) - `unsupported-rdt-version` → rdt-core <5.1 (RN <0.74) → fall back to `npx react-devtools` - `metro-scan-error` / `bootstrap-threw` / `bootstrap-no-effect` → unexpected, with verbatim error text After a successful bootstrap, `REACT_NATIVE_PROFILER_SETUP_SCRIPT` is **re-run** to install argent's `__argent_startWrapped__` wrappers on the freshly-attached interfaces. Without this re-run, `buildStartScript`'s post-start `__argent_isProfiling__` check would falsely report `startProfiling-threw` (the wrapper sets the flag; an unwrapped interface leaves it false even on a successful start). ### Idempotency / leak protection `initBackend` adds subscriptions to `hook.sub('renderer', ...)` on every call, so naive re-invocation would leak listeners. The script short-circuits with `reason: 'already-attached'` when `rendererInterfaces.size > 0`, so repeated `react-profiler-start` calls within a session never re-bootstrap. Verified empirically: listener counts stay flat across multiple start/stop cycles. ### Coexistence with external DevTools If `npx react-devtools` is later started while argent has already bootstrapped, RN's bundled reconnect path calls `connectToDevTools` again, which goes through `attachRenderer` → sees `rendererInterface != null` (argent's already there) → re-emits `renderer-attached` without re-attaching. Both backends coexist; argent talks to the renderer interface directly, so the active "agent" is irrelevant to profiling. ## Files changed - `packages/tool-server/src/utils/react-profiler/scripts.ts` — adds `BOOTSTRAP_DEVTOOLS_BACKEND_SCRIPT` (the injected IIFE that does the Metro scan + connect call). - `packages/tool-server/src/utils/react-profiler/devtools-bootstrap.ts` *(new)* — `BootstrapResult` type, `BootstrapReason` union, and the `bootstrapFailureMessage()` helper that maps each reason to its agent-facing error. Mirrors the `session-ownership.ts` convention (types + pure helpers as a sibling of `scripts.ts`, unit-testable without CDP / vitest mocking). - `packages/tool-server/src/tools/profiler/react/react-profiler-start.ts` — calls bootstrap on empty `rendererInterfaces`, re-runs setup, throws via the imported `bootstrapFailureMessage`. Also replaces two pre-existing error strings ("hook not present", `how_to_reclaim`) with the same agent-friendly phrasing used elsewhere in this PR. - `packages/tool-server/test/react-profiler/bootstrap-devtools-backend.test.ts` *(new)* — 12 unit tests covering every reason branch (Map + array Metro registries, require() throws, getModules throws, connect throws, connect is a no-op, idempotent already-attached path). ## Manual verification (iPhone Air, RN 0.76.7 bridgeless, no external DevTools) 1. Reload bundle → confirm `rIds: [1,2], riIds: []` (bug state). 2. Pre-fix: `react-profiler-start` returns *"No React renderer interface attached yet…"* 3. Post-fix: `react-profiler-start` returns `{ started_at, hermes_version: "for RN 0.76.7", detected_architecture: "bridgeless" }`. Hook now shows `riIds: [1,2]`, `__argent_startWrapped__: true` on each, `reactDevtoolsAgent` set, `__argent_isProfiling__: true`. 4. Forced a React state-hook dispatch during the session and stopped: `total_react_commits: 2`, `unattributed_ms: 8.41` — real commit data captured through the bootstrapped backend. 5. Second `react-profiler-start` in same session returns success via `already-attached` early-exit (verified directly against the script). Hook listener counts (`renderer`, `renderer-attached`, `react-devtools`) stable across multiple cycles — no subscription leak. 6. Simulated `no-renderers` by clearing `h.renderers` → got *"React has not rendered yet, so there is nothing to profile. Ask the user to wait until the app shows React content…"* |
||
|
Paweł Fornagiel
|
62d0fe0c73 |
refactor: overhaul React profiler - DevTools backend, session ownership, richer data (#156)
## What this PR does
This refactor:
- replaces the DFS fiber-walking hook inside the app's JS runtime with
the **React DevTools backend** as the source of profiling data
- adds a session-ownership system so two tool-server instances can't
silently clobber each other
- gives agent a new status tool for recovery after interruptions
- improves what the profiling analysis report shows the agent
I realise this a big pull request, but changes began to affect each
other when it comes to registered scripts on the react-native side.
This, all-in-all, should however cover our needs when it comes to the
react profiler and if it proves sufficient - we will only have to
iterate on small fixes afterwards.
If I were you, I would skip over the implementation of scripts on the
native-side and focus on what the tool-server does when it comes to
profiling right now.
---
## Profiling workflow
The overall flow has not changed from the agent's perspective - agent
calls `react-profiler-start`, do something in the app, call
`react-profiler-stop`.
What changed is how the profiling data is collected.
**Before:** `react-profiler-start` injected a custom `onCommitFiberRoot`
hook that walked the live fiber tree at every React commit in DFS-like
way and appended records to a global `__ARGENT_DEVTOOLS_COMMITS__` array
in the app's JS heap. On stop, the array was fetched in chunks over CDP.
**Now:** `react-profiler-start` calls `ri.startProfiling(true)` on the
React DevTools backend's `rendererInterface`. React's own profiler
accumulates commit data internally. On stop, a single
`STOP_AND_READ_SCRIPT` call stops the backend and reads
`ri.getProfilingData()` in one round-trip. The DevTools backend data is
richer, more accurate, and doesn't require maintaining a bespoke
fiber-walking implementation.
---
## Session ownership
The profiler runs inside the app's JS heap. There's only one "slot": it
used to be that when two tool-server instances (or the same instance
after a process restart) both try to start profiling, the second call
silently overwrites the first and all data from the first session is
gone.
Right now, there is an approach which blocks silent overtake of the
profiling session.
When `react-profiler-start` runs, it:
1. Reads the current state from the runtime using `READ_STATE_SCRIPT` (a
pure, side-effect-free read).
2. If a session is already running, it reads the
`__ARGENT_PROFILER_OWNER__` object written into `globalThis`, which
contains `sessionId`, `startedAtEpochMs`, and `lastHeartbeatEpochMs`.
3. Classifies the existing session as **fresh** or **stale** using
`classifyStaleness()` (stale = no heartbeat for > 5 minutes).
If the session is fresh and you don't pass `{ force: true }`, start
refuses and returns an `already_running` payload instead of overwriting.
4. If the session is stale or `force: true`, it calls
`STOP_FOR_TAKEOVER_SCRIPT` to cleanly stop the prior session, then
starts a new one.
### Heartbeats
Each `sessionId` UUID is written to `__ARGENT_PROFILER_OWNER__` in the
runtime at start. While profiling, any side query tool
(`react-profiler-renders`, `react-profiler-fiber-tree`) that this same
tool-server process calls will bump `lastHeartbeatEpochMs` via
`HEARTBEAT_SCRIPT`. This keeps the session from being classified as
stale as long as the owning process is still making calls.
### What the agent sees when something goes wrong
Previously, if you called `react-profiler-start` into an already-running
session you'd get a silent clobber or a confusing Hermes error. Now:
- **Running session detected, still fresh:** `{ already_running: true,
owner: { sessionId, startedAtEpochMs, lastHeartbeatEpochMs },
age_seconds, how_to_reclaim }` — the agent can read the owner timestamp
and decide whether to tell the user to wait or force-reclaim.
- **Running session detected, stale:** automatic takeover with a log
note.
- **No owner record (e.g. another DevTools client started the
session):** also automatic takeover.
### New `react-profiler-status` tool
A read-only tool that reports the current state without starting or
stopping anything. Returns `session_status` as one of `"active"` /
`"taken_over"` / `"stopped"` / `"no_react_runtime"`, plus the full owner
record. Use this after a debugger disconnect, an unexpected error, or
when the agent restarts mid-session to decide whether to continue,
reclaim, or give up.
The agent has instructions to use the tool whenever it is interrupted
during the profiling by the user or otherwise. Will have to look into
how much this is actually used.
---
## What changed in the data we collect
The old hook read fiber fields directly (`fiber.type.displayName`,
`fiber.memoizedProps`, `fiber.selfBaseDuration`, etc.) at commit time.
This meant:
- Names for **transient components** (modals, tooltips, navigation
screens) that unmounted before stop were already lost - the fiber was
gone.
- Change-detection logic (which props changed, which hooks fired) was
reimplemented manually against the fiber internals.
The new approach reads `ri.getProfilingData()` which gives us the
DevTools backend's own commit log. Each commit entry has
`fiberActualDurations`, `fiberSelfDurations`, `changeDescriptions`,
`timestamp`, and `duration` - the same data React DevTools Profiler
shows in its browser panel.
### Fiber name cache for transient components
To address the lost-name problem, `FIBER_ROOT_TRACKER_SCRIPT` now hooks
`onCommitFiberRoot` and, immediately after the DevTools backend finishes
processing each commit (synchronously, while the fiber is still in
memory), populates `globalThis.__argent_fiberNames__` with `fiberID →
displayName` entries. Because fiber IDs are monotonically increasing and
never reused, this cache never goes stale. `STOP_AND_READ_SCRIPT` uses
this cache as a fallback for any fiber whose name
`getDisplayNameForElementID` can no longer resolve.
### Unattributed fiber tracking
When a transient fiber renders during the session but still can't be
named at stop time (not in the name cache, not in the DevTools instance
map), its work is now **surfaced rather than silently dropped**. The
stop tool computes `unattributedByCommit`: a per-commit tuple of
`[commitIndex, droppedFiberCount, droppedSelfMs]`. This flows through
to:
- The session metadata file on disk (`unattributedByCommit` array).
- The pipeline (`buildHotCommitSummaries` receives and indexes it).
- The `HotCommitSummary` type gains `unattributedMs` and
`unattributedFiberCount`.
- The generated report shows a warning line per commit with unattributed
work, e.g. `⚠️ 12ms unattributed — 3 fibers unmounted before stop`.
- `react-profiler-stop` returns top-level `unattributed_ms`,
`unattributed_fiber_count`, `unattributed_commit_count` in its response
so the agent immediately knows if there's a gap.
### Actual vs. self duration now both surfaced
The old report showed only `selfDuration` (exclusive render time). Both
are now captured in `HotCommitComponentEntry` and rendered in the report
as:
```
- `ComponentName` — 4.2ms self, 18.7ms w/children
```
A legend at the top of the report explains when to sum self and when not
to sum inclusive time. The summary table also shows both columns.
### Component name unwrapping
Display names from the DevTools backend can be wrapped:
`Forget(Memo(MyButton))`, `ForwardRef(Icon)`, etc. The pipeline's render
stage now strips these wrappers using `annotateComponentName()` and
appends a human-readable tag, so the report shows `\`MyButton\`
[React.memo + React Compiler]` instead of the raw internal string. The
raw DevTools name is still used in all tool-call suggestions (since
every pipeline stage keys on the original string).
---
## What scripts do what
All injected JS scripts now live in a single file —
`packages/tool-server/src/utils/react-profiler/scripts.ts` — grouped by
responsibility:
| Script | When injected | What it does |
|---|---|---|
| `REACT_NATIVE_PROFILER_SETUP_SCRIPT` | Every `react-profiler-start`
(idempotent) | Wraps `ri.startProfiling` / `ri.stopProfiling` to track
`isProfiling` state and `startedAtEpochMs` without clock skew; installs
the `__argent_profilerHeartbeat` helper |
| `FIBER_ROOT_TRACKER_SCRIPT` | On CDP connect (idempotent) | Intercepts
`onCommitFiberRoot` to populate the `__argent_fiberNames__` commit-time
name cache |
| `READ_STATE_SCRIPT` | Every `react-profiler-start`,
`react-profiler-status` | Pure read: returns `{ hookExists,
rendererInterfaceFound, isRunning, owner, nowEpochMs }` |
| `buildStartScript(ownerJson)` | `react-profiler-start` on the happy
path | Calls `ri.startProfiling`, writes owner record, returns
post-start verification flags |
| `STOP_FOR_TAKEOVER_SCRIPT` | `react-profiler-start` on the takeover
path | Stops the prior session cleanly before starting a new one |
| `HEARTBEAT_SCRIPT` | `react-profiler-renders`,
`react-profiler-fiber-tree` | Bumps `lastHeartbeatEpochMs` while the
owning process is still active |
| `STOP_AND_READ_SCRIPT` | `react-profiler-stop` | Stops the backend
profiler and reads `getProfilingData()` + name cache in one round-trip |
| `RESOLVE_FIBER_META_SCRIPT` | `react-profiler-stop` (if commits > 0) |
Walks the live fiber tree to collect `hookTypes`, `parentName`, and
`isCompilerOptimized` metadata per component |
|
||
|
Filip131311
|
8c2a313c76 |
chore: format codebase with Prettier
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Ignacy Łątka
|
cd41fa4d3b |
fix: write profiler output to /tmp instead of project root (#73)
* fix: write profiler output to /tmp instead of project root The profiler was creating an argent-profiler-cwd/ directory inside the user's project root. Output is now written to /tmp/argent-profiler-cwd/ using os.tmpdir() so no artifacts are left in the project tree. * fix: remove project_root from profiler tools, drop getDebugDir param No backwards compatibility needed — project_root is stripped from ios-profiler-start and profiler-load schemas entirely, and getDebugDir takes no arguments now that the path is always /tmp/argent-profiler-cwd/. * fix: remove stale project_root from skill docs, add dump.test.ts - SKILL.md and diagnostic-tools.md still referenced project_root for profiler-load after it was removed from the schema — now corrected - Add 17 tests covering getDebugDir, writeDump, writeDumpCompact, readCpuProfile, and readCommitTree (all passing) |