From ea702556d11eb61b5835e23aeff29f002010f835 Mon Sep 17 00:00:00 2001 From: thymikee Date: Thu, 10 Sep 2026 14:34:21 +0200 Subject: [PATCH] fix: fail tree observation without an attached app and replace re-flushed roots Observation commands (get tree, get component, find, count, errors, profile start) returned an empty result with exit 0 when no React app was attached, so "No components with errors or warnings" was indistinguishable from a check that never ran. They now fail with a typed `no-app-attached` reason and a message that says whether an app ever connected. When another React DevTools backend attaches to the same app (React Native DevTools opening, another agent), react-devtools-core creates a fresh renderer interface with a new fiber-ID space and flushes the whole tree through the hook-wide operations channel every agent subscribes to. Later commits reach only that new interface, so the daemon's existing root is frozen, not merely duplicated. The bridge now asks the tree to reconcile a second root on the same connection: a root that structurally duplicates an older root of the same renderer replaces it; a genuinely different root is kept. Reported through callstack/agent-device#2430. --- .changeset/brave-owls-observe.md | 11 +++ README.md | 2 +- .../skills/react-devtools/SKILL.md | 3 +- .../react-devtools/references/commands.md | 2 + .../src/__tests__/component-tree.test.ts | 65 ++++++++++++ .../src/component-tree.ts | 35 +++++++ packages/agent-react-devtools/src/daemon.ts | 55 ++++++++--- .../src/devtools-bridge.ts | 8 ++ packages/agent-react-devtools/src/types.ts | 5 + packages/e2e-tests/src/cli-commands.test.ts | 99 +++++++++++++++++++ .../e2e-tests/src/connection-health.test.ts | 13 +-- .../e2e-tests/src/daemon-auto-restart.test.ts | 4 +- packages/e2e-tests/src/helpers.ts | 1 + 13 files changed, 275 insertions(+), 28 deletions(-) create mode 100644 .changeset/brave-owls-observe.md diff --git a/.changeset/brave-owls-observe.md b/.changeset/brave-owls-observe.md new file mode 100644 index 0000000..a3fdd2f --- /dev/null +++ b/.changeset/brave-owls-observe.md @@ -0,0 +1,11 @@ +--- +'agent-react-devtools': minor +--- + +Tree observation commands (`get tree`, `get component`, `find`, `count`, +`errors`, `profile start`) now fail with a typed `no-app-attached` reason and a +non-zero exit code when no React app is attached, instead of reporting an empty +result that reads as a clean pass. When another React DevTools backend attaches +to the same app (React Native DevTools opening, another agent) and re-flushes the +tree under a fresh fiber-ID space, the daemon now replaces its frozen copy +instead of counting every component twice. diff --git a/README.md b/README.md index 5221a90..5863dd1 100644 --- a/README.md +++ b/README.md @@ -152,7 +152,7 @@ Components with errors or warnings are annotated in tree and search output: @c5 [fn] Form ⚠2 ✗1 ``` -Use the `errors` command to list only components with issues: +Use the `errors` command to list only components with issues. When no app is attached, it exits 1 with `No React app is attached` rather than reporting a clean tree: ```sh agent-react-devtools errors diff --git a/packages/agent-react-devtools/skills/react-devtools/SKILL.md b/packages/agent-react-devtools/skills/react-devtools/SKILL.md index c9bee74..e3e1e19 100644 --- a/packages/agent-react-devtools/skills/react-devtools/SKILL.md +++ b/packages/agent-react-devtools/skills/react-devtools/SKILL.md @@ -167,7 +167,8 @@ agent-react-devtools status # Should show 1 connected app ## Important Rules - **Labels reset** when the app reloads or components unmount/remount. After a reload, use `wait --connected` then re-check with `get tree` or `find`. -- **`status` first** — if status shows 0 connected apps, the React app is not connected. Web users may need to run `npx agent-react-devtools init`; React Native users need both manual steps in [setup.md](references/setup.md). +- **`status` first** — if status shows 0 connected apps, the React app is not connected. Web users may need to run `npx agent-react-devtools init`; React Native users need both manual steps in [setup.md](references/setup.md). Tree observation commands exit 1 with `No React app is attached` in that state; treat that as "nothing was observed", not as a clean result. +- **Another DevTools attaching reassigns IDs** — when React Native DevTools (or another agent) attaches to the same app, React re-flushes the tree under new IDs and the daemon replaces its copy. Re-run `get tree` or `find` before reusing earlier `@cN` labels. - **Headed browser required** — if using `agent-browser`, always use `--headed` mode. Headless Chromium does not properly load the devtools connect script. - **Profile while interacting** — profiling only captures renders that happen between `profile start` and `profile stop`. Make sure the relevant interaction happens during that window. - **Use `--depth`** on large trees — a deep tree can produce a lot of output. Start with `--depth 3` or `--depth 4` and go deeper only on the subtree you care about. diff --git a/packages/agent-react-devtools/skills/react-devtools/references/commands.md b/packages/agent-react-devtools/skills/react-devtools/references/commands.md index cea5384..28028fa 100644 --- a/packages/agent-react-devtools/skills/react-devtools/references/commands.md +++ b/packages/agent-react-devtools/skills/react-devtools/references/commands.md @@ -65,6 +65,8 @@ Output example: `⚠N` = N warnings, `✗N` = N errors. Returns "No components with errors or warnings" when everything is clean. +When no React app is attached, this and every other tree observation command (`get tree`, `get component`, `find`, `count`, `profile start`) exits 1 with `No React app is attached to the DevTools daemon ...` instead of an empty result, so a missing app can never read as a clean pass. + Error/warning annotations also appear in `get tree`, `get component`, and `find` output when counts are non-zero. ## Profiling diff --git a/packages/agent-react-devtools/src/__tests__/component-tree.test.ts b/packages/agent-react-devtools/src/__tests__/component-tree.test.ts index b29f7ce..e04a9b5 100644 --- a/packages/agent-react-devtools/src/__tests__/component-tree.test.ts +++ b/packages/agent-react-devtools/src/__tests__/component-tree.test.ts @@ -492,3 +492,68 @@ describe('ComponentTree', () => { }); }); }); + +describe('ComponentTree.reconcileReflushedRoot', () => { + const ROOT_OP = (id: number) => [1, id, 11, 0, 1, 0, 0]; + + function fullTree(rendererId: number, rootId: number, base: number): number[] { + return buildOps(rendererId, rootId, ['App', 'Header', 'Item'], (strId) => [ + ...ROOT_OP(rootId), + ...addOp(base + 1, 5, rootId, strId('App')), + ...addOp(base + 2, 8, base + 1, strId('Header')), + ...addOp(base + 3, 5, base + 1, strId('Item')), + ...addOp(base + 4, 5, base + 1, strId('Item')), + ]); + } + + it('drops the older root when a new root of the same renderer duplicates it', () => { + const tree = new ComponentTree(); + tree.applyOperations(fullTree(1, 100, 0)); + expect(tree.getComponentCount()).toBe(5); + + // Another backend attached: same fibers re-flushed under a fresh ID space + tree.applyOperations(fullTree(1, 500, 1000)); + expect(tree.getComponentCount()).toBe(10); + + expect(tree.reconcileReflushedRoot(500)).toBe(100); + expect(tree.getComponentCount()).toBe(5); + expect(tree.getRootIds()).toEqual([500]); + expect(tree.getNode(1)).toBeUndefined(); + expect(tree.getNode(1001)?.displayName).toBe('App'); + expect(tree.findByName('Item', true)).toHaveLength(2); + }); + + it('keeps a genuine second root whose structure differs', () => { + const tree = new ComponentTree(); + tree.applyOperations(fullTree(1, 100, 0)); + tree.applyOperations( + buildOps(1, 500, ['Sidebar'], (strId) => [ + ...ROOT_OP(500), + ...addOp(1001, 5, 500, strId('Sidebar')), + ]), + ); + + expect(tree.reconcileReflushedRoot(500)).toBeNull(); + expect(tree.getRootIds()).toEqual([100, 500]); + expect(tree.getComponentCount()).toBe(7); + }); + + it('never matches roots across renderers', () => { + const tree = new ComponentTree(); + tree.applyOperations(fullTree(1, 100, 0)); + tree.applyOperations(fullTree(2, 500, 1000)); + + expect(tree.reconcileReflushedRoot(500)).toBeNull(); + expect(tree.getComponentCount()).toBe(10); + }); + + it('only considers roots added before the reflushed one', () => { + const tree = new ComponentTree(); + tree.applyOperations(fullTree(1, 100, 0)); + tree.applyOperations(fullTree(1, 500, 1000)); + + // Asking about the older root must not delete the newer, live one + expect(tree.reconcileReflushedRoot(100)).toBeNull(); + expect(tree.getRootIds()).toEqual([100, 500]); + }); +}); diff --git a/packages/agent-react-devtools/src/component-tree.ts b/packages/agent-react-devtools/src/component-tree.ts index 2ae1c42..b6c3824 100644 --- a/packages/agent-react-devtools/src/component-tree.ts +++ b/packages/agent-react-devtools/src/component-tree.ts @@ -547,6 +547,41 @@ export class ComponentTree { this.removeNode(rootId); } + /** + * A second React DevTools backend attaching to the same app (React Native + * DevTools opening, another agent) re-flushes the whole tree through the + * shared hook under a fresh fiber-ID space, and later commits reach only that + * new root. The copy this tree already holds is therefore frozen, not merely + * duplicated. When `rootId` structurally duplicates an older root of the same + * renderer, drop the older root and return its id. + */ + reconcileReflushedRoot(rootId: number): number | null { + const root = this.nodes.get(rootId); + if (!root) return null; + for (const olderId of this.roots) { + if (olderId === rootId) break; + const older = this.nodes.get(olderId); + if (!older || older.rendererId !== root.rendererId) continue; + if (this.subtreesMatch(olderId, rootId)) { + this.removeNode(olderId); + return olderId; + } + } + return null; + } + + private subtreesMatch(a: number, b: number): boolean { + const x = this.nodes.get(a); + const y = this.nodes.get(b); + if (!x || !y) return false; + if (x.type !== y.type || x.displayName !== y.displayName || x.key !== y.key) return false; + if (x.children.length !== y.children.length) return false; + for (let i = 0; i < x.children.length; i++) { + if (!this.subtreesMatch(x.children[i], y.children[i])) return false; + } + return true; + } + /** * Look up the @cN label for a given component ID. * Returns undefined if the ID has no label assigned. diff --git a/packages/agent-react-devtools/src/daemon.ts b/packages/agent-react-devtools/src/daemon.ts index d0aa12d..a2d918d 100644 --- a/packages/agent-react-devtools/src/daemon.ts +++ b/packages/agent-react-devtools/src/daemon.ts @@ -139,6 +139,30 @@ class Daemon { }); } + /** + * Commands that observe the live component tree must fail when no app is + * attached. An empty tree is not evidence of anything: reporting it as a + * result lets "no components with errors" stand in for "nothing was observed". + */ + private requireAttachedApp(): IpcResponse | null { + const health = this.bridge.getConnectionHealth(); + if (health.connectedApps > 0) return null; + + const state = health.lastDisconnectAt !== null + ? `app disconnected ${Math.round((Date.now() - health.lastDisconnectAt) / 1000)}s ago, waiting for reconnect` + : health.hasEverConnected + ? 'app disconnected' + : 'no app has connected since the daemon started'; + return { + ok: false, + reason: 'no-app-attached', + error: + `No React app is attached to the DevTools daemon on port ${this.port} (${state}). ` + + 'Start the app in development mode, then run `agent-react-devtools wait --connected`. ' + + 'React Native 0.87+ apps also need `agent-react-devtools init`.', + }; + } + private async handleCommand(cmd: IpcCommand, conn: net.Socket): Promise { try { switch (cmd.type) { @@ -160,6 +184,8 @@ class Daemon { }; case 'get-tree': { + const detached = this.requireAttachedApp(); + if (detached) return detached; let resolvedRoot: number | undefined; if (cmd.root !== undefined) { resolvedRoot = this.tree.resolveId(cmd.root); @@ -178,21 +204,12 @@ class Daemon { if (resolvedRoot !== undefined && treeData.length === 0) { return { ok: false, error: `Component ${cmd.root} not found` }; } - const response: IpcResponse = { - ok: true, - data: { nodes: treeData, totalCount }, - }; - if (treeData.length === 0) { - const health = this.bridge.getConnectionHealth(); - if (health.hasEverConnected && health.connectedApps === 0 && health.lastDisconnectAt !== null) { - const ago = Math.round((Date.now() - health.lastDisconnectAt) / 1000); - response.hint = `app disconnected ${ago}s ago, waiting for reconnect...`; - } - } - return response; + return { ok: true, data: { nodes: treeData, totalCount } }; } case 'get-component': { + const detachedForComponent = this.requireAttachedApp(); + if (detachedForComponent) return detachedForComponent; const resolvedId = this.tree.resolveId(cmd.id); if (resolvedId === undefined) { return { ok: false, error: `Component ${cmd.id} not found` }; @@ -212,25 +229,30 @@ class Daemon { } case 'find': - return { + return this.requireAttachedApp() ?? { ok: true, data: this.tree.findByName(cmd.name, cmd.exact), }; case 'count': - return { + return this.requireAttachedApp() ?? { ok: true, data: this.tree.getCountByType(), }; - case 'errors': + case 'errors': { + const detachedForErrors = this.requireAttachedApp(); + if (detachedForErrors) return detachedForErrors; this.tree.getTree(); return { ok: true, data: this.tree.getComponentsWithErrorsOrWarnings(), }; + } - case 'profile-start': + case 'profile-start': { + const detachedForProfile = this.requireAttachedApp(); + if (detachedForProfile) return detachedForProfile; this.profiler.start(cmd.name); // Snapshot existing component names so they survive unmounts for (const id of this.tree.getAllNodeIds()) { @@ -239,6 +261,7 @@ class Daemon { } this.bridge.startProfiling(); return { ok: true, data: 'Profiling started' }; + } case 'profile-stop': { await this.bridge.stopProfilingAndCollect(); diff --git a/packages/agent-react-devtools/src/devtools-bridge.ts b/packages/agent-react-devtools/src/devtools-bridge.ts index 86214bf..908bf43 100644 --- a/packages/agent-react-devtools/src/devtools-bridge.ts +++ b/packages/agent-react-devtools/src/devtools-bridge.ts @@ -257,6 +257,14 @@ export class DevToolsBridge { } const added = this.tree.applyOperations(operations); + // A second root on one connection is either a genuine multi-root app or a + // re-flush from another DevTools backend attaching; the tree decides by structure. + const roots = operations.length >= 2 ? this.connectionRoots.get(ws) : undefined; + if (roots && roots.size > 1) { + const replaced = this.tree.reconcileReflushedRoot(operations[1]); + if (replaced !== null) roots.delete(replaced); + } + // Cache display names during profiling so unmounted components are still identifiable if (this.profiler.isActive()) { for (const node of added) { diff --git a/packages/agent-react-devtools/src/types.ts b/packages/agent-react-devtools/src/types.ts index 1ca62fa..9689afc 100644 --- a/packages/agent-react-devtools/src/types.ts +++ b/packages/agent-react-devtools/src/types.ts @@ -196,10 +196,15 @@ export type IpcCommand = | { type: 'wait'; condition: 'connected'; timeout?: number } | { type: 'wait'; condition: 'component'; name: string; timeout?: number }; +/** Machine-readable failure reasons, for callers that must not key on error text. */ +export type IpcFailureReason = 'no-app-attached'; + export interface IpcResponse { ok: boolean; data?: unknown; error?: string; + /** Set alongside `error` when the failure has a typed cause */ + reason?: IpcFailureReason; /** The @cN label, passed through when commands use label-based IDs */ label?: string; /** Contextual hint for empty or stale results */ diff --git a/packages/e2e-tests/src/cli-commands.test.ts b/packages/e2e-tests/src/cli-commands.test.ts index 75f349c..8852dac 100644 --- a/packages/e2e-tests/src/cli-commands.test.ts +++ b/packages/e2e-tests/src/cli-commands.test.ts @@ -83,3 +83,102 @@ describe('CLI commands (e2e)', () => { expect(result.stdout).toContain('Usage:'); }); }); + +describe('CLI commands without an attached app (e2e)', () => { + let stateDir: string; + let port: number; + let daemon: ChildProcess | null = null; + + beforeEach(async () => { + stateDir = createTempStateDir(); + port = getTestPort(); + daemon = startDaemon(port, stateDir); + await waitForDaemon(stateDir); + }); + + afterEach(async () => { + await stopDaemon(daemon, stateDir); + daemon = null; + }); + + for (const args of [['errors'], ['count'], ['get', 'tree'], ['find', 'App'], ['profile', 'start']]) { + it(`should fail \`${args.join(' ')}\` instead of reporting an empty result`, async () => { + const result = await runCli(args, stateDir); + expect(result.exitCode).toBe(1); + expect(result.stderr).toContain('No React app is attached'); + expect(result.stderr).toContain(String(port)); + expect(result.stdout).not.toContain('No components'); + expect(result.stdout).not.toContain('0 components'); + }); + } + + it('should still report status and honour wait timeouts', async () => { + const status = await runCli(['status'], stateDir); + expect(status.exitCode).toBe(0); + expect(status.stdout).toContain('0 connected'); + + const wait = await runCli(['wait', '--connected', '--timeout', '1'], stateDir); + expect(wait.exitCode).toBe(1); + }); +}); + +describe('CLI commands when another DevTools backend attaches (e2e)', () => { + let stateDir: string; + let port: number; + let daemon: ChildProcess | null = null; + let ws: WebSocket | null = null; + + const fullTree = (rootId: number, base: number) => + buildOperations(1, rootId, (s) => [ + rootOp(rootId), + addOp(base + 1, ELEMENT_TYPE_FUNCTION, rootId, s('App')), + addOp(base + 2, ELEMENT_TYPE_MEMO, base + 1, s('Header')), + addOp(base + 3, ELEMENT_TYPE_FUNCTION, base + 1, s('UserProfile')), + addOp(base + 4, ELEMENT_TYPE_HOST, base + 1, s('div')), + ]); + + beforeEach(async () => { + stateDir = createTempStateDir(); + port = getTestPort(); + daemon = startDaemon(port, stateDir); + await waitForDaemon(stateDir); + ws = await connectMockApp(port); + sendOperations(ws!, fullTree(100, 0)); + await sleep(300); + }); + + afterEach(async () => { + if (ws && ws.readyState === WebSocket.OPEN) ws.close(); + await stopDaemon(daemon, stateDir); + daemon = null; + ws = null; + }); + + it('should replace the frozen tree instead of counting it twice', async () => { + // React Native DevTools (or another agent) attaching re-flushes the same + // tree through the shared hook under a fresh fiber-ID space. + sendOperations(ws!, fullTree(500, 1000)); + await sleep(300); + + const count = await runCli(['count'], stateDir); + expect(count.stdout).toContain('5 components'); + + const found = await runCli(['find', 'UserProfile', '--exact'], stateDir); + expect(found.stdout.trim().split('\n')).toHaveLength(1); + expect(found.stdout).toContain('id:1003'); + }); + + it('should keep a genuinely different second root', async () => { + sendOperations( + ws!, + buildOperations(1, 500, (s) => [ + rootOp(500), + addOp(1001, ELEMENT_TYPE_FUNCTION, 500, s('Sidebar')), + ]), + ); + await sleep(300); + + const count = await runCli(['count'], stateDir); + expect(count.stdout).toContain('7 components'); + }); +}); diff --git a/packages/e2e-tests/src/connection-health.test.ts b/packages/e2e-tests/src/connection-health.test.ts index d50dd5c..442344b 100644 --- a/packages/e2e-tests/src/connection-health.test.ts +++ b/packages/e2e-tests/src/connection-health.test.ts @@ -87,7 +87,7 @@ describe('Connection health (e2e)', () => { await sleep(300); }); - it('should show hint when tree is empty after disconnect', async () => { + it('should refuse tree observation after disconnect instead of reporting an empty tree', async () => { const ws = await connectMockApp(port); await sleep(300); @@ -103,14 +103,11 @@ describe('Connection health (e2e)', () => { ws.close(); await sleep(300); - // get-tree should return hint const resp = await sendIpcCommand(socketPath, { type: 'get-tree' }); - expect(resp.ok).toBe(true); - expect(resp.hint).toBeDefined(); - expect(resp.hint).toContain('disconnected'); - expect(resp.hint).toContain('waiting for reconnect'); - const { nodes } = resp.data as { nodes: Array }; - expect(nodes).toHaveLength(0); + expect(resp.ok).toBe(false); + expect(resp.reason).toBe('no-app-attached'); + expect(resp.error).toContain('disconnected'); + expect(resp.error).toContain('waiting for reconnect'); }); it('wait --connected should resolve immediately when already connected', async () => { diff --git a/packages/e2e-tests/src/daemon-auto-restart.test.ts b/packages/e2e-tests/src/daemon-auto-restart.test.ts index 4f62415..e111729 100644 --- a/packages/e2e-tests/src/daemon-auto-restart.test.ts +++ b/packages/e2e-tests/src/daemon-auto-restart.test.ts @@ -37,7 +37,7 @@ describe('Daemon auto-restart on rebuild', () => { infoBefore.buildMtime = 1000; fs.writeFileSync(infoPath, JSON.stringify(infoBefore, null, 2)); - const result = await runCli(['get', 'tree'], stateDir); + const result = await runCli(['profile', 'slow'], stateDir); expect(result.exitCode).toBe(0); const infoAfter = JSON.parse(fs.readFileSync(infoPath, 'utf-8')); @@ -49,7 +49,7 @@ describe('Daemon auto-restart on rebuild', () => { const infoPath = path.join(stateDir, 'daemon.json'); const infoBefore = JSON.parse(fs.readFileSync(infoPath, 'utf-8')); - const result = await runCli(['get', 'tree'], stateDir); + const result = await runCli(['profile', 'slow'], stateDir); expect(result.exitCode).toBe(0); const infoAfter = JSON.parse(fs.readFileSync(infoPath, 'utf-8')); diff --git a/packages/e2e-tests/src/helpers.ts b/packages/e2e-tests/src/helpers.ts index 75c4288..e39290c 100644 --- a/packages/e2e-tests/src/helpers.ts +++ b/packages/e2e-tests/src/helpers.ts @@ -9,6 +9,7 @@ interface IpcResponse { ok: boolean; data?: unknown; error?: string; + reason?: string; hint?: string; }