Commit Graph

1 Commits

Author SHA1 Message Date
filip131311 8b3a8851a2 fix(profiler): honour top_n in profiler-commit-query by_index (#667)
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.
2026-08-11 17:18:11 +02:00