mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
8b3a8851a2
Fixes #630. ## The bug `by_index` never received `top_n`: `renderByIndex` had no such parameter and the dispatch passed only `(commits, commit_index)`. Reproduced against the released 0.18.1 CLI on a real session — `top_n: 8` returned **306 rows / 20,245 characters**. It matters because `react-profiler-analyze` points at this exact mode for drilling into a slow commit, so it is both the most likely call and the most likely to blow an agent's context. ## Correction to the issue (and to my own first pass) The issue says "the other three modes honour `top_n` correctly". That's not right — **`cascade_tree` ignores it too** (`renderCascadeTree` takes no `topN`; its dispatch also passes two arguments). Only `by_component` and `by_time_range` slice. I've deliberately **not** fixed `cascade_tree` here, and not for the reason that first looked right. A DFS-prefix budget would in fact be structurally safe there. The real problems are that its roots come from a `Set` in insertion order with no subtree-cost aggregation, so any cut keeps an *arbitrary* subtree rather than the expensive one — making it useful needs a cost-ranking pass, which is design work, not a slice. And it dedups by `name:depth`, so it never had the 306-row blow-up that motivated this issue. Filed separately with the other findings. ## Why `top_n` is honoured only when explicitly passed Applying the schema's existing default of 20 looks obviously right and is a trap. These rows are **fibers, not components**, and they collapse hard. Measured on the real commit: | top_n | distinct components covered | |---|---| | 8 | 3 | | 20 | **10** | | 50 | 18 | | (all 306 fibers) | 70 | The analyze report that recommends this mode already shows up to **15** distinct components (`MAX_COMPONENT_ENTRIES`). So a default of 20 would make the escape hatch return *strictly less than the report it escapes from* — silently, since the footer would only say "298 hidden", not "you learned nothing new". `top_n` is therefore `.optional()`, and the two modes that genuinely defaulted to 20 keep it explicitly at their call sites (`params.top_n ?? 20`), so their behaviour is byte-identical. Since that collapse is invisible from a row count, the footer states it outright: ``` _Showing the 8 costliest of 306 fibers — 298 cheaper fibers hidden. Note these are fibers, not components: the full set covers 70 distinct components and this view covers 3. Omit top_n for everything._ ``` ## The trap this had to avoid The root-cause chain block scans the **full** commit, not the truncated table. The fiber carrying the root cause is typically *cheap* — it triggers the cascade rather than doing the work — so it falls outside any `top_n`. Slicing before that lookup would silently drop the single most useful line in the output. There's a regression test that pins exactly this: the only fiber with a `rootCauseParent` is the cheapest, `top_n=5`, and the chain must still be printed. ## Also fixed The analyze report suggested `` `profiler-commit-query mode=by_index` to see all `` **without passing `commit_index`** — an agent following it verbatim queries a different commit than the one being described. Now names the commit. Note I did *not* change it to name a `top_n` value: `totalComponentCount` counts distinct component names, while `by_index` emits one row per fiber, so passing it would still truncate — a subtler falsehood inside the line being corrected. With no default cap, "to see all" is simply true again. ## Verified live | call | before | after | |---|---|---| | `by_index top_n=8` | 306 rows, 20,245 chars | 8 rows, 898 chars, heading `Top Fibers`, `**Fibers:** 306 (showing 8)` | | `by_index` (no top_n) | 306 rows | 306 rows, heading `Full Detail`, no notice | | `by_component` | — | unchanged | ## Checks - 3093 tests pass; 7 new. Note the A/B against the pre-fix source is not meaningful for this file — `renderByIndex` wasn't exported, so those failures are import errors, not behavioural. The real before/after is the live table above, captured against the released 0.18.1 build. - extract-tools 46/46; SpiderShield average 9.10 (tool `description:` untouched); prettier, eslint, both typechecks clean; lock untouched. - Touches `renderByIndex` only — no overlap with PR #663, which owns `renderByComponent` in the same file.
98 lines
3.7 KiB
TypeScript
98 lines
3.7 KiB
TypeScript
import { describe, it, expect } from "vitest";
|
|
import { renderByIndex } from "../src/tools/profiler/query/profiler-commit-query";
|
|
import type { DevToolsFiberCommit } from "../src/utils/react-profiler/types/input";
|
|
|
|
function fiber(
|
|
componentName: string,
|
|
actualDuration: number,
|
|
extra: Partial<DevToolsFiberCommit> = {}
|
|
): DevToolsFiberCommit {
|
|
return {
|
|
commitIndex: 1,
|
|
timestamp: 2868,
|
|
commitDuration: 18.7,
|
|
componentName,
|
|
actualDuration,
|
|
selfDuration: actualDuration / 2,
|
|
didRender: true,
|
|
changeDescription: null,
|
|
...extra,
|
|
} as DevToolsFiberCommit;
|
|
}
|
|
|
|
/** 30 fibers spanning 10 distinct component names, descending in cost. */
|
|
function commit(): DevToolsFiberCommit[] {
|
|
const out: DevToolsFiberCommit[] = [];
|
|
for (let i = 0; i < 30; i++) {
|
|
out.push(fiber(`Comp${i % 10}`, 30 - i));
|
|
}
|
|
return out;
|
|
}
|
|
|
|
describe("profiler-commit-query by_index — top_n", () => {
|
|
it("returns every fiber when top_n is not given", () => {
|
|
// by_index is the drill-down the analyze report points at for a full
|
|
// breakdown, so an unrequested cap would make it show less than the report.
|
|
const out = renderByIndex(commit(), 1);
|
|
expect(out.split("\n").filter((l) => l.startsWith("| `")).length).toBe(30);
|
|
expect(out).toContain("Full Detail");
|
|
expect(out).not.toContain("cheaper fibers hidden");
|
|
});
|
|
|
|
it("caps the table when top_n is given, keeping the costliest fibers", () => {
|
|
const out = renderByIndex(commit(), 1, 8);
|
|
const rows = out.split("\n").filter((l) => l.startsWith("| `"));
|
|
expect(rows.length).toBe(8);
|
|
// Sorted by actualDuration desc, so the retained rows are 30..23.
|
|
expect(rows[0]).toContain("| 30.0 |");
|
|
expect(rows[7]).toContain("| 23.0 |");
|
|
});
|
|
|
|
it("reports the true fiber total alongside the shown count", () => {
|
|
const out = renderByIndex(commit(), 1, 8);
|
|
expect(out).toContain("**Fibers:** 30 (showing 8)");
|
|
// The heading must stop claiming full detail once it is not full.
|
|
expect(out).not.toContain("Full Detail");
|
|
});
|
|
|
|
it("says how many distinct components the truncated view actually covers", () => {
|
|
// The trap this guards: 8 fibers can be far fewer than 8 components, so a
|
|
// caller cannot infer coverage from the row count.
|
|
const out = renderByIndex(commit(), 1, 8);
|
|
expect(out).toContain("22 cheaper fibers hidden");
|
|
expect(out).toContain("10 distinct components");
|
|
expect(out).toContain("Omit top_n for everything");
|
|
});
|
|
|
|
it("keeps the root-cause chain when its fiber is too cheap to make the cut", () => {
|
|
// The fiber carrying the root cause is usually cheap — it triggers the
|
|
// cascade rather than doing the work — so it falls outside any top_n.
|
|
// Scanning the truncated list instead of the full commit would silently
|
|
// drop the single most useful line in this output.
|
|
const commits = [
|
|
...commit(),
|
|
fiber("TinyTrigger", 0.01, {
|
|
rootCauseParent: "ParentThatChanged",
|
|
rootCauseChain: ["Root", "Middle"],
|
|
rootCauseReason: "props",
|
|
}),
|
|
];
|
|
const out = renderByIndex(commits, 1, 5);
|
|
expect(out).toContain("**Root cause chain:**");
|
|
expect(out).toContain("ParentThatChanged");
|
|
// ...and the cheap fiber itself is indeed not in the table.
|
|
expect(out.split("\n").filter((l) => l.startsWith("| `TinyTrigger`")).length).toBe(0);
|
|
});
|
|
|
|
it("adds no truncation notice when top_n exceeds the fiber count", () => {
|
|
const out = renderByIndex(commit(), 1, 500);
|
|
expect(out).not.toContain("cheaper fibers hidden");
|
|
expect(out).toContain("Full Detail");
|
|
expect(out).not.toContain("(showing");
|
|
});
|
|
|
|
it("leaves the not-found path alone", () => {
|
|
expect(renderByIndex(commit(), 99, 5)).toBe("_Commit #99 not found in stored data._");
|
|
});
|
|
});
|