mirror of
https://github.com/stablyai/orca.git
synced 2026-10-01 00:02:10 +00:00
Neil
dfc5083031
The wrapped-row skip added in a2d4fb93e49 was unsound when a logical line's first row has been evicted from the scrollback. `_isRowCoveredByLineStart` returned true for any `isWrapped` row, but the surviving chain of a trimmed line has no line start left in the buffer, so nothing covered those rows and `findNextWithSelection`'s wrap-around loop skipped straight past them. Reproduced, review's scenario at Orca's default 5000-row scrollback (real Terminal + SearchAddon, happy-dom, 80x24, one 4000-row un-newlined blob then 2000 short lines; buffer 5024 rows, row 0 wrapped, resultCount 4 in all builds). Ten findNext calls, selection start row: unpatched RangeError x7, then 3020 3021 8044 edb51c728a4 3020 3021 8044 8045 3020 3021 8044 8045 3020 3021 + a2d4fb93e49 3020 3021 8044 8045 8044 8045 8044 8045 8044 8045 this commit 3020 3021 8044 8045 3020 3021 8044 8045 3020 3021 The two real in-buffer matches were reachable once and then never again: the find bar kept reporting four matches while Enter ping-ponged between two phantom rows. The review's mechanism is right and its tradeoff note #2 was wrong — with a full ring `getLine(-1)` resolves to a stale line rather than undefined, so upstream's rewind does reach the surviving chain. Fix: a row is only skippable once an earlier `_findInLine` in the same call has already scanned its line from an equal or lower offset. The two forward loops always run after the `_findInLine` at `startRow`, which rewinds into the line start, so they are unchanged. The wrap-around loop starts at row 0 and has no such predecessor, so row 0 is always searched; every later wrapped row is then covered by it. Helper renamed to `_isRowCoveredByEarlierSearch` to say what the precondition actually is. Evidence the skip is now upstream-equivalent: a differential fuzz of 600 randomized cases (seeded; random cols/rows/scrollback, blob and short-line mixes that do and do not trim a line head, findNext/findPrevious sequences, options {} / caseSensitive / wholeWord / regex / wholeWord+caseSensitive / decorations), comparing full transcripts of every selection range and resultCount against a build with the skip forced off. 0 divergences with this commit; 9 with the old `_isRowCoveredByLineStart` semantics, so the fuzz has the power to see the regression it is asserting the absence of. No perf regression on the shape the skip exists for: a trimmed-head 5200-row un-newlined line at the default scrollback, no match, scans in 11ms. Regression test drives the real addon through Orca's `safeFind` at a small scrollback (the mechanism is eviction, not size) and asserts the surviving match rows stay reachable across a full cycle. It fails with the old skip ("expected 1 to be greater than 1") and passes with this one. Also fixes doc drift this branch introduced: .github/workflows/pr.yml said the xterm rebuild covers "the two addons"; xterm-upstream.json now lists three. Not changed, and why: - Regex search is still slow (~20s on a 5000-row line) and is still excluded from the skip, because regex can reject at one offset and match at a later one in the same line. That is a pre-existing upstream cost, disclosed in the PR, and closing it needs a different approach than a row skip. - The perf test still only covers the no-match shape. The many-matches shape is dominated by `_bufferColsToStringOffset`, which this patch does not touch; adding a test asserting no improvement there would assert nothing.