mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
Ignacy Łątka
d4b9818d64
Fixes two confirmed defects in the JS-runtime debugger subsystem.
## Defect A — `debugger-component-tree` hangs ~15s and throws on every
error condition
**File:**
`packages/tool-server/src/utils/debugger/scripts/component-tree.ts`
The injected fiber-walk script delivers its success result through the
`__argent_callback` CDP binding, but every error path instead did
`return JSON.stringify({ error: ... })` from the async IIFE — `'No
DevTools hook'`, `'No fiber roots'`, `'Could not find UIManager'`.
The consumer (`evaluateWithBinding`, `cdp-client.ts`) runs
`Runtime.evaluate` with `returnByValue:false`/`awaitPromise:false`, so
the IIFE's return value is discarded. The promise only settles when the
binding fires or the 15s timeout elapses. So on a release/production
build, before the first render, or on a non-RN runtime,
`debugger-component-tree` blocked for ~15s and then threw a generic
`DEBUGGER_CDP_BINDING_TIMEOUT` instead of returning the intended
diagnostic immediately.
**Observed:** callback never invoked (0 calls) on the error paths; the
tool hangs to the binding timeout.
**Expected:** the error is delivered through the binding right away and
surfaces as `Error: <message>` via the consumer's
`JSON.parse(response.result).error` path.
**Fix:** mirror the sibling `inspect-at-point.ts`. Added an
`__argent_fail(msg)` helper that routes the error through
`__argent_callback` in the exact same wire shape as the success path (`{
requestId, result: JSON.stringify({ error: msg }) }`), and converted all
three error paths to call it and `return`.
## Defect B — `LogFileWriter` truncates console levels longer than 5
chars
**File:** `packages/tool-server/src/utils/debugger/log-file-writer.ts`
The flat-line level was rendered as `LEVEL_DISPLAY[entry.level] ??
entry.level.toUpperCase().padEnd(5).slice(0, 5)`. The `.slice(0,5)`
truncated any level not in the 5-entry `LEVEL_DISPLAY` map. CDP's
`Runtime.consoleAPICalled.type` includes `"warning"` (`console.warn`)
and `"assert"` (`console.assert`) — neither is in the map — so
`readAll()`/`readFiltered()` returned `"warni"`/`"asser"`, and
`readFiltered({ level: "warning" })` matched nothing.
**Observed:** writing level `"warning"` → `readAll()[0].level ===
"warni"`; `readFiltered({ level: "assert" }).total === 0`.
**Expected:** the canonical level round-trips exactly for levels of any
length.
**Fix:** drop `.slice(0,5)` — keep `padEnd(5)` for column alignment but
never cut. `LINE_RE` captures the level as `\S+`, so a non-truncated,
whitespace-free level round-trips through `parseFlatLine` unchanged.
Short levels (log/info/warn/error/debug) are unaffected.
## Tests
- `packages/tool-server/test/debugger/comptree-error-channel.test.ts` —
asserts `__argent_callback` is invoked on the error paths and the error
is encoded as `parsed.error`.
- `packages/tool-server/test/debugger/log-file-writer-level.test.ts` —
asserts `"warning"`/`"assert"` round-trip and filter correctly, and
short levels still work.
Both new files fail before the fix and pass after. Full `tool-server`
suite green (1627 tests); repo-root `tsc --build` clean; eslint clean on
changed files.
73 lines
2.5 KiB
TypeScript
73 lines
2.5 KiB
TypeScript
import { describe, it, expect, afterEach } from "vitest";
|
|
import { LogFileWriter } from "../../src/utils/debugger/log-file-writer";
|
|
|
|
/**
|
|
* Regression for level truncation: CDP emits console levels longer than 5 chars
|
|
* ("warning" from console.warn, "assert" from console.assert) that are not in
|
|
* LEVEL_DISPLAY. The old fallback `.padEnd(5).slice(0, 5)` truncated them to
|
|
* "warni"/"asser", so readAll()/readFiltered() returned the wrong level and
|
|
* filtering by the canonical level found nothing. The level must round-trip
|
|
* exactly for levels of any length while short levels keep working as before.
|
|
*/
|
|
let w: LogFileWriter;
|
|
afterEach(() => {
|
|
if (w) w.close();
|
|
});
|
|
|
|
describe("LogFileWriter round-trips levels of any length", () => {
|
|
it("preserves 'warning' through readAll()", () => {
|
|
w = new LogFileWriter(59231);
|
|
w.write({
|
|
id: 0,
|
|
timestamp: new Date(1710000000000).toISOString(),
|
|
level: "warning",
|
|
message: "x",
|
|
});
|
|
expect(w.readAll()[0]!.level).toBe("warning");
|
|
});
|
|
|
|
it("filters by the canonical multi-char level 'warning'", () => {
|
|
// The exact behavior this change restores: filtering by the full CDP level.
|
|
// With the old `.slice(0, 5)` the persisted level was "warni", so a filter
|
|
// for "warning" matched nothing (total === 0).
|
|
w = new LogFileWriter(59234);
|
|
w.write({
|
|
id: 0,
|
|
timestamp: new Date(1710000000000).toISOString(),
|
|
level: "warning",
|
|
message: "x",
|
|
});
|
|
const { total, entries } = w.readFiltered({ level: "warning" });
|
|
expect(total).toBe(1);
|
|
expect(entries).toHaveLength(1);
|
|
expect(entries[0]!.level).toBe("warning");
|
|
});
|
|
|
|
it("filters by 'assert'", () => {
|
|
w = new LogFileWriter(59232);
|
|
w.write({
|
|
id: 0,
|
|
timestamp: new Date(1710000000000).toISOString(),
|
|
level: "assert",
|
|
message: "y",
|
|
});
|
|
const { total, entries } = w.readFiltered({ level: "assert" });
|
|
expect(total).toBe(1);
|
|
expect(entries).toHaveLength(1);
|
|
});
|
|
|
|
it("still preserves short levels that hit the padEnd fallback", () => {
|
|
// "trace" is a real CDP level (console.trace) that is NOT in LEVEL_DISPLAY, so
|
|
// it exercises the changed `.toUpperCase().padEnd(5)` fallback — unlike "warn",
|
|
// which the map short-circuits. A <=5-char level must still round-trip exactly.
|
|
w = new LogFileWriter(59233);
|
|
w.write({
|
|
id: 0,
|
|
timestamp: new Date(1710000000000).toISOString(),
|
|
level: "trace",
|
|
message: "z",
|
|
});
|
|
expect(w.readAll()[0]!.level).toBe("trace");
|
|
});
|
|
});
|