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; }