diff --git a/.changeset/lucky-hounds-shave.md b/.changeset/lucky-hounds-shave.md new file mode 100644 index 0000000..8842d87 --- /dev/null +++ b/.changeset/lucky-hounds-shave.md @@ -0,0 +1,12 @@ +--- +'agent-react-devtools': minor +--- + +Component reads (`get tree`, `get component`, `find`, `count`, `errors`) now fail with a +structured `NO_APP_CONNECTED` response when no app is attached, instead of answering from an +empty component tree. + +Previously `devtools errors` printed `No components with errors or warnings` with nothing +attached, so a check that was never performed was indistinguishable from a check that passed. +The refusal names how long ago the last app disconnected, replacing the empty-tree hint that +`get tree` used to attach to a successful response. diff --git a/packages/agent-react-devtools/src/daemon.ts b/packages/agent-react-devtools/src/daemon.ts index d0aa12d..98c28a5 100644 --- a/packages/agent-react-devtools/src/daemon.ts +++ b/packages/agent-react-devtools/src/daemon.ts @@ -4,7 +4,13 @@ import path from 'node:path'; import { DevToolsBridge } from './devtools-bridge.js'; import { ComponentTree } from './component-tree.js'; import { Profiler } from './profiler.js'; -import type { IpcCommand, IpcResponse, DaemonInfo, StatusInfo } from './types.js'; +import type { + IpcCommand, + IpcResponse, + DaemonInfo, + StatusInfo, + ConnectionHealth, +} from './types.js'; const DEFAULT_STATE_DIR = path.join( process.env.HOME || process.env.USERPROFILE || '/tmp', @@ -37,6 +43,17 @@ function enrichWithLabels( } } +/** + * How long the last app has been gone, when one was ever attached. A read that + * missed a live app by seconds deserves a different answer from one against a + * daemon nothing has ever connected to. + */ +function describeDisconnect(health: ConnectionHealth): string { + if (!health.hasEverConnected || health.lastDisconnectAt === null) return ''; + const seconds = Math.round((Date.now() - health.lastDisconnectAt) / 1000); + return ` (the last app disconnected ${seconds}s ago)`; +} + class Daemon { private ipcServer: net.Server | null = null; private bridge: DevToolsBridge; @@ -139,6 +156,25 @@ class Daemon { }); } + /** + * A component read answers a question about an attached app's tree. With no + * app attached the tree is empty for a reason the caller cannot see, so an + * empty answer is indistinguishable from "nothing matched" and a check that + * was never performed reads as a check that passed. Refuse instead. + * + * This is read in the same synchronous turn as the tree itself, so nothing + * can attach or detach between the check and the answer. + */ + private componentReadUnavailable(): IpcResponse | null { + const health = this.bridge.getConnectionHealth(); + if (health.connectedApps > 0) return null; + return { + ok: false, + code: 'NO_APP_CONNECTED', + error: `No app is connected, so there is no component tree to read${describeDisconnect(health)}. Run \`devtools status\` to check the daemon, and \`devtools wait --connected\` to block until an app attaches.`, + }; + } + private async handleCommand(cmd: IpcCommand, conn: net.Socket): Promise { try { switch (cmd.type) { @@ -160,6 +196,8 @@ class Daemon { }; case 'get-tree': { + const unobservable = this.componentReadUnavailable(); + if (unobservable) return unobservable; let resolvedRoot: number | undefined; if (cmd.root !== undefined) { resolvedRoot = this.tree.resolveId(cmd.root); @@ -178,21 +216,15 @@ class Daemon { if (resolvedRoot !== undefined && treeData.length === 0) { return { ok: false, error: `Component ${cmd.root} not found` }; } - const response: IpcResponse = { + return { 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; } case 'get-component': { + const unobservable = this.componentReadUnavailable(); + if (unobservable) return unobservable; const resolvedId = this.tree.resolveId(cmd.id); if (resolvedId === undefined) { return { ok: false, error: `Component ${cmd.id} not found` }; @@ -212,23 +244,30 @@ class Daemon { } case 'find': - return { - ok: true, - data: this.tree.findByName(cmd.name, cmd.exact), - }; + return ( + this.componentReadUnavailable() ?? { + ok: true, + data: this.tree.findByName(cmd.name, cmd.exact), + } + ); case 'count': - return { - ok: true, - data: this.tree.getCountByType(), - }; + return ( + this.componentReadUnavailable() ?? { + ok: true, + data: this.tree.getCountByType(), + } + ); - case 'errors': + case 'errors': { + const unobservable = this.componentReadUnavailable(); + if (unobservable) return unobservable; this.tree.getTree(); return { ok: true, data: this.tree.getComponentsWithErrorsOrWarnings(), }; + } case 'profile-start': this.profiler.start(cmd.name); diff --git a/packages/agent-react-devtools/src/types.ts b/packages/agent-react-devtools/src/types.ts index 1ca62fa..66530d1 100644 --- a/packages/agent-react-devtools/src/types.ts +++ b/packages/agent-react-devtools/src/types.ts @@ -168,6 +168,9 @@ export interface ConnectionEvent { timestamp: number; } +/** No app is attached, so a component read has nothing to observe. */ +export type IpcErrorCode = 'NO_APP_CONNECTED'; + export interface ConnectionHealth { connectedApps: number; hasEverConnected: boolean; @@ -200,6 +203,8 @@ export interface IpcResponse { ok: boolean; data?: unknown; error?: string; + /** Machine-readable reason for a refusal, so callers need not match on `error`. */ + code?: IpcErrorCode; /** 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/component-read-attachment.test.ts b/packages/e2e-tests/src/component-read-attachment.test.ts new file mode 100644 index 0000000..8686cf5 --- /dev/null +++ b/packages/e2e-tests/src/component-read-attachment.test.ts @@ -0,0 +1,121 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import path from 'node:path'; +import type { ChildProcess } from 'node:child_process'; +import { WebSocket } from 'ws'; +import { + createTempStateDir, + getTestPort, + startDaemon, + waitForDaemon, + stopDaemon, + sendIpcCommand, + connectMockApp, + sendOperations, + buildOperations, + rootOp, + addOp, + ELEMENT_TYPE_FUNCTION, + sleep, +} from './helpers.js'; + +// A component read with no app attached answers from an empty tree, which is +// indistinguishable from "nothing matched": a check that was never performed +// reads as a check that passed. Every read must refuse instead. +const COMPONENT_READS = [ + { label: 'get-tree', command: { type: 'get-tree' } }, + { label: 'get-component', command: { type: 'get-component', id: 1 } }, + { label: 'find', command: { type: 'find', name: 'App' } }, + { label: 'count', command: { type: 'count' } }, + { label: 'errors', command: { type: 'errors' } }, +] as const; + +describe('Component reads require an attached app (e2e)', () => { + let stateDir: string; + let port: number; + let daemon: ChildProcess | null = null; + let socketPath: string; + + beforeEach(async () => { + stateDir = createTempStateDir(); + port = getTestPort(); + daemon = startDaemon(port, stateDir); + await waitForDaemon(stateDir); + socketPath = path.join(stateDir, 'daemon.sock'); + }); + + afterEach(async () => { + await stopDaemon(daemon, stateDir); + daemon = null; + }); + + for (const { label, command } of COMPONENT_READS) { + it(`should refuse ${label} when no app has ever connected`, async () => { + const resp = await sendIpcCommand(socketPath, command as never); + + expect(resp.ok).toBe(false); + expect(resp.code).toBe('NO_APP_CONNECTED'); + expect(resp.error).toContain('No app is connected'); + // Nothing has ever attached, so there is no disconnect to describe. + expect(resp.error).not.toContain('disconnected'); + }); + } + + it('should refuse a read issued after the app disconnected, not answer from its stale tree', async () => { + const ws = await connectMockApp(port); + sendOperations( + ws, + buildOperations(1, 100, (s) => [ + rootOp(100), + addOp(1, ELEMENT_TYPE_FUNCTION, 100, s('App')), + ]), + ); + await sleep(200); + + const attached = await sendIpcCommand(socketPath, { type: 'find', name: 'App' }); + expect(attached.ok).toBe(true); + expect(attached.data).toHaveLength(1); + + ws.close(); + await sleep(300); + + const afterDisconnect = await sendIpcCommand(socketPath, { type: 'find', name: 'App' }); + expect(afterDisconnect.ok).toBe(false); + expect(afterDisconnect.code).toBe('NO_APP_CONNECTED'); + expect(afterDisconnect.error).toContain('disconnected'); + + const errors = await sendIpcCommand(socketPath, { type: 'errors' }); + expect(errors.ok).toBe(false); + expect(errors.code).toBe('NO_APP_CONNECTED'); + }); + + it('should answer component reads while an app is attached', async () => { + const ws = await connectMockApp(port); + sendOperations( + ws, + buildOperations(1, 100, (s) => [ + rootOp(100), + addOp(1, ELEMENT_TYPE_FUNCTION, 100, s('App')), + ]), + ); + await sleep(200); + + for (const { command } of COMPONENT_READS) { + // `get-component` resolves a real id here; the others take no argument. + const resp = await sendIpcCommand(socketPath, command as never); + expect(resp.code).toBeUndefined(); + } + + const errors = await sendIpcCommand(socketPath, { type: 'errors' }); + expect(errors.ok).toBe(true); + expect(errors.data).toEqual([]); + + ws.close(); + }); + + it('should keep status and wait answerable with nothing attached', async () => { + const status = await sendIpcCommand(socketPath, { type: 'status' }); + + expect(status.ok).toBe(true); + expect((status.data as { connectedApps: number }).connectedApps).toBe(0); + }); +}); diff --git a/packages/e2e-tests/src/connection-health.test.ts b/packages/e2e-tests/src/connection-health.test.ts index d50dd5c..df1a53e 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 a tree read after disconnect and say how long ago it happened', async () => { const ws = await connectMockApp(port); await sleep(300); @@ -103,14 +103,14 @@ describe('Connection health (e2e)', () => { ws.close(); await sleep(300); - // get-tree should return hint + // The tree is empty because nothing is attached, not because nothing + // matched: answering `ok` here made a read that observed nothing look + // like a read that found nothing. 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.code).toBe('NO_APP_CONNECTED'); + expect(resp.error).toContain('disconnected'); + expect(resp.data).toBeUndefined(); }); 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..5477c3c 100644 --- a/packages/e2e-tests/src/daemon-auto-restart.test.ts +++ b/packages/e2e-tests/src/daemon-auto-restart.test.ts @@ -37,8 +37,11 @@ describe('Daemon auto-restart on rebuild', () => { infoBefore.buildMtime = 1000; fs.writeFileSync(infoPath, JSON.stringify(infoBefore, null, 2)); + // Any command routed through ensureDaemon exercises the rebuild check. + // `get tree` refuses with no app attached, which is beside the point here: + // reaching the daemon at all is what proves the restart happened. const result = await runCli(['get', 'tree'], stateDir); - expect(result.exitCode).toBe(0); + expect(result.stderr).toContain('No app is connected'); const infoAfter = JSON.parse(fs.readFileSync(infoPath, 'utf-8')); expect(infoAfter.pid).not.toBe(infoBefore.pid); @@ -50,7 +53,7 @@ describe('Daemon auto-restart on rebuild', () => { const infoBefore = JSON.parse(fs.readFileSync(infoPath, 'utf-8')); const result = await runCli(['get', 'tree'], stateDir); - expect(result.exitCode).toBe(0); + expect(result.stderr).toContain('No app is connected'); const infoAfter = JSON.parse(fs.readFileSync(infoPath, 'utf-8')); expect(infoAfter.pid).toBe(infoBefore.pid);