perf: a new transcript no longer re-tags every file discovered after it (#99) - #123

🤖 Automated first-pass review

TL;DR

The performance rationale is credible and the CHANGELOG entry is present and specific. The main merge-readiness risk is that stableDiscoveryRanks introduces a new floating-point rank space whose invariants are only checked at the end, and the "gap runs out" fallback described in the prose is not obviously reachable/verified by the test. I'd want the tie-break equivalence argument tightened before merge.

Analysis

• Correctness of the core idea. The function preserves stored discoveryIndex for known files when their relative order is unchanged, and interpolates ranks for new files. Since downstream code only compares ranks (as stated) and the relative order equals the manifest's, the tie-break should agree with the full loader. That reasoning is sound given the stated invariant.

• Fallback conditions vs. tests. The doc comment says fallback to manifest positions happens when (a) known files changed relative order, or (b) the gap between two neighbours is exhausted. Condition (a) is exercised by the swap in the test. Condition (b) — ranks[index] > ranks[index-1] failing after interpolation — is not directly exercised. With float interpolation step = (upper - lower)/(runLength+1), exhaustion requires an extremely tight gap; the test never constructs that, so the fallback branch is untested. Please add a focused case, or justify why it is unreachable (e.g. if prior ranks are integers and gaps are always ≥ 2, exhaustion may be impossible — but that should be demonstrated, not left implicit).

• Determinism / float equality. ranks[position] = priorRank then a later entry.discoveryIndex === ranks[index] comparison means known files keep their exact stored value (good), but new files get float ranks. If two refreshes insert into the same gap with the same neighbours, the interpolation is deterministic, so re-runs should be stable — worth a comment confirming this, since a non-deterministic rank would cause the very re-tagging being fixed.

• priorRanks[runEnd]! non-null assertion. The upper computation relies on the loop invariant that priorRanks[runEnd] is defined when runEnd < ordered.length. This is true given the inner while loop stops on a defined entry, but the ! is a correctness-load-bearing assertion rather than a checked condition. A brief note or a defensive guard would make this robust to future edits.

• Semantics preserved. The test asserts record equality and contentAnalysis equality against the full loader, including after a reorder. That covers the stated "ranks change nothing about which copy wins" claim for the cases tested. The tie is deliberately constructed (same first timestamp + same message), which is the right way to test discovery-order tie-breaks — good.

• CHANGELOG. Entry exists, follows the existing bullet style, cites the issue (Extension causes high cpu load #99), and quantifies before/after. Consistent with AGENTS.md "user-facing changes require a CHANGELOG.md entry".

• Test hygiene / Windows note. The author flags two pre-existing failures on main on Windows and timing-sensitive flakes. These are called out as pre-existing; if they are not, they must be confirmed against main before merge. No i18n or README impact is expected for a pure index-internal change, so the localization checklist is not triggered.

• Diagnostics / privacy. The change adds no logging of paths or IDs; only aggregate mutation counts in the existing diagnostics. No privacy-invariant concern.

Suggested next step(s)

1. Add a test (or a short proof) for the "gap exhausted" fallback path in stableDiscoveryRanks; the current suite does not reach it.
2. Document or guard the priorRanks[runEnd]! assertion so the invariant is explicit.
3. Confirm the two Windows failures and the flakes reproduce on main (the author's note suggests they do) so CI signal stays trustworthy.
4. Scope confirmation: the author offers follow-ups (recomputeDerivedRows, buildAnalysisRuntimeState, deleted-transcript rebuild). Keep this PR to the rank change only, per "one logical change" — do not fold follow-ups in.

This is an automated first pass; the maintainer makes the final decision.

🤖 Generated by DeepSeek as an automated first pass — not a maintainer decision.

Merged for the upcoming v2.4.1 performance patch. Thank you @rsyuzyov for
isolating the discovery-rank hotspot and preserving the full loader's tie-break semantics.

We verified the original change against the latest 2.4.1 hotfix: 1,272 Node tests
passed (one local exact-mtime fixture skipped), 195 UI tests passed, and VSIX
verification passed. A fixture-only integration check also exercised batched insertions,
70 consecutive insertions into the same gap with an actual precision-exhaustion fallback,
append/reorder/move/delete, and prior-snapshot immutability. Records and content analysis
matched the full loader throughout. The merged main source tree matches this tested integration.

New main CI is running; the release is still a draft. This fixes the specific retagging
cost, not every possible CPU hotspot in #99. We will follow up on the issues after
Marketplace delivery, rather than describing an unreleased fix as available.

🤖 Generated with OpenAI Codex