mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
filip131311
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.
3.7 KiB
3.7 KiB