Skip to content

Commit 424bab4

Browse files
committed
Add regression tests then fix fifth round of Copilot review comments
Tests added first (src/render.test.ts) as regression guards for the three tui.ts-level state bugs — they exercise the pure functions involved and document the correct behavior that the tui.ts fixes must preserve: - isCursorVisible: stale scrollOffset > cursor (result of filter shrinking rows without clamping scrollOffset) always returns false, confirming the cursor is above the viewport and scrollOffset must be clamped - buildRows + isCursorVisible: invalid regex after toggle produces 0 rows; cursor at former position is out-of-range and must be re-clamped to 0 (Math.min(cursor, Math.max(0, rows.length - 1))) - renderGroups: sticky repo header (\u25b2) is NOT shown when cursor is on a repo row even when scrollOffset > 0 (only shown on extract rows whose repo row has scrolled above the viewport) Fixes (src/tui.ts): 1. stickyHeaderLines condition in getViewportHeight(): change \`scrollOffset > 0 ? 1 : 0\` to \`scrollOffset > 0 && cursor >= scrollOffset ? 1 : 0\` so the viewport is only shrunk by one when a sticky header will actually be rendered by renderGroups, preventing premature scroll-stop on repo rows. 2. scrollOffset not clamped after filter reduces rows: add \`scrollOffset = Math.min(scrollOffset, cursor)\` after cursor clamping in Ctrl+W/Alt+Backspace, Backspace, Del, and printable-char (paste) handlers. Without this, a previously-scrolled viewport could remain beyond the end of the shrunken row list, producing an empty groups area with no recovery. 3. Tab toggle (filterRegex) did not rebuild rows or clamp cursor/scrollOffset: after \`filterRegex = !filterRegex\`, now calls buildRows and applies the same clamping as all other filter-state-changing handlers. See PR #65
1 parent aecd617 commit 424bab4

2 files changed

Lines changed: 87 additions & 8 deletions

File tree

src/render.test.ts

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,25 @@ describe("isCursorVisible", () => {
421421
const rows = buildRows(groups);
422422
expect(isCursorVisible(rows, groups, 0, 0, 10)).toBe(true);
423423
});
424+
425+
it("returns false when scrollOffset is stale (greater than cursor after filter shrinks rows)", () => {
426+
// Regression guard for Bug 2: in filter-mode edit handlers (Backspace, Del,
427+
// word-delete, paste) tui.ts clamps `cursor` to the new rows length but does
428+
// NOT clamp `scrollOffset`. When the filter reduces rows, cursor is clamped
429+
// down but scrollOffset can remain larger than cursor, causing the cursor to
430+
// appear "above the viewport" — isCursorVisible returns false and the while
431+
// loop `while (scrollOffset < cursor ...)` never runs (already false), so the
432+
// tui gets stuck showing an empty groups area.
433+
//
434+
// Scenario: 4 rows, user was at cursor=3, scrollOffset=2. Filter reduces rows
435+
// to 2. Cursor clamped to 1. scrollOffset NOT clamped → still 2 > cursor=1.
436+
const groups = [makeGroup("org/repo", ["a.ts", "b.ts", "c.ts"], false)];
437+
const rows = buildRows(groups); // [repo, ext0, ext1, ext2]
438+
// cursor=1 but scrollOffset=2: cursor is *above* the viewport window
439+
expect(isCursorVisible(rows, groups, 1, 2, 10)).toBe(false);
440+
// The fix: clamp scrollOffset = Math.min(scrollOffset, cursor) = Math.min(2,1) = 1
441+
expect(isCursorVisible(rows, groups, 1, 1, 10)).toBe(true);
442+
});
424443
});
425444

426445
// ─── buildSummaryFull ──────────────────────────────────────────────────────────────
@@ -593,6 +612,27 @@ describe("renderGroups", () => {
593612
expect(stripped).toContain("org/repoA"); // repo name shown in sticky line
594613
});
595614

615+
it("does NOT show sticky repo header when cursor is on a repo row (even with scrollOffset > 0)", () => {
616+
// Regression guard for Bug 1: getViewportHeight() in tui.ts used to subtract
617+
// a sticky-header line whenever scrollOffset > 0, but renderGroups only shows
618+
// the sticky header when the cursor is on an *extract* row whose repo header
619+
// has scrolled above the viewport. When cursor is on a repo row, no sticky
620+
// header is emitted and the subtracted line makes isCursorVisible() return
621+
// false prematurely (the cursor appears invisible one step too early).
622+
//
623+
// rows: [repo(0), ext(0,0), repo(1), ext(1,0)]
624+
// cursor=2 (repo1), scrollOffset=1 → repo(0) < scrollOffset, but cursor is on
625+
// repo(1), not an extract → sticky header must NOT be shown.
626+
const groups = [
627+
makeGroup("org/repoA", ["a.ts"], false),
628+
makeGroup("org/repoB", ["b.ts"], false),
629+
];
630+
const rows = buildRows(groups);
631+
const out = renderGroups(groups, 2, rows, 40, 1, "q", "org");
632+
const stripped = out.replace(/\x1b\[[0-9;]*m/g, "");
633+
expect(stripped).not.toContain("▲");
634+
});
635+
596636
it("stops rendering rows when viewport is filled (overflow break)", () => {
597637
// Extract rows with fragments have h=2; with termHeight=9 viewportHeight=2
598638
// row0(repo,h=1): fits. row1(ext,h=2): 1+2=3 > 2 AND usedLines=1>0 → break
@@ -1061,6 +1101,34 @@ describe("buildRows (filterTarget + filterRegex)", () => {
10611101
const extracts = rows.filter((r) => r.type === "extract");
10621102
expect(extracts.length).toBe(0);
10631103
});
1104+
1105+
it("toggling regex to invalid pattern collapses rows to 0 — cursor must be clamped (regression guard for Bug 3)", () => {
1106+
// Regression guard for Bug 3: the Tab key in tui.ts toggles filterRegex but
1107+
// did not rebuild rows or clamp cursor/scrollOffset afterward. When the new
1108+
// regex is invalid (or simply more restrictive), the row list shrinks. If
1109+
// cursor was pointing at a now-removed row, isCursorVisible returns false yet
1110+
// the scroll-adjust while-loop doesn't fire (cursor is already 0 or the
1111+
// condition `scrollOffset < cursor` is already false), leaving cursor
1112+
// pointing at an invalid index — renderGroups skips the cursor highlight.
1113+
//
1114+
// The fix: after `filterRegex = !filterRegex`, rebuild rows and clamp:
1115+
// const newRows = buildRows(groups, filterInput, filterTarget, filterRegex);
1116+
// cursor = Math.min(cursor, Math.max(0, newRows.length - 1));
1117+
// scrollOffset = Math.min(scrollOffset, cursor);
1118+
const groups = [makeGroup("org/repo", ["src/foo.ts", "src/bar.ts"], false)];
1119+
// Before toggle: regex=false, pattern="src" → 2 extract rows visible
1120+
const rowsBefore = buildRows(groups, "src", "path", false);
1121+
expect(rowsBefore.filter((r) => r.type === "extract")).toHaveLength(2);
1122+
let cursor = 2; // cursor on second extract
1123+
// After toggle to regex=true with an invalid pattern:
1124+
const rowsAfter = buildRows(groups, "[invalid", "path", true);
1125+
expect(rowsAfter).toHaveLength(0); // invalid regex → no matches, no rows
1126+
// isCursorVisible with stale cursor (still 2, rows=[]) must return false
1127+
expect(isCursorVisible(rowsAfter, groups, cursor, 0, 10)).toBe(false);
1128+
// The required clamp:
1129+
cursor = Math.min(cursor, Math.max(0, rowsAfter.length - 1));
1130+
expect(cursor).toBe(0);
1131+
});
10641132
});
10651133

10661134
// ─── buildFilterStats — filterTarget + filterRegex ───────────────────────────

src/tui.ts

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -93,9 +93,12 @@ export async function runInteractive(
9393
let barLines = 0;
9494
if (filterMode) barLines = 2;
9595
else if (filterPath || filterTarget !== "path" || filterRegex) barLines = 1;
96-
// When scrolled past the top, renderGroups shows a sticky repo header
97-
// that consumes one additional line from the available viewport.
98-
const stickyHeaderLines = scrollOffset > 0 ? 1 : 0;
96+
// When scrolled past the top and the cursor is within the visible window,
97+
// renderGroups may show a sticky repo header that consumes one extra line.
98+
// Mirror the condition precisely: sticky only appears when the cursor row is
99+
// an extract whose repo row has scrolled above the viewport (repoRowIndex <
100+
// scrollOffset). `cursor >= scrollOffset` is the necessary pre-condition.
101+
const stickyHeaderLines = scrollOffset > 0 && cursor >= scrollOffset ? 1 : 0;
99102
return termHeight - 6 - barLines - stickyHeaderLines;
100103
};
101104

@@ -175,8 +178,12 @@ export async function runInteractive(
175178
cursor = Math.min(cursor, Math.max(0, newRows.length - 1));
176179
scrollOffset = Math.min(scrollOffset, cursor);
177180
} else if (key === KEY_TAB) {
178-
// Tab — toggle regex mode
181+
// Tab — toggle regex mode; rebuilds rows immediately and clamps
182+
// cursor/scrollOffset so the current position stays valid.
179183
filterRegex = !filterRegex;
184+
const newRows = buildRows(groups, filterInput, filterTarget, filterRegex);
185+
cursor = Math.min(cursor, Math.max(0, newRows.length - 1));
186+
scrollOffset = Math.min(scrollOffset, cursor);
180187
scheduleStatsUpdate();
181188
} else if (key === ANSI_ARROW_LEFT) {
182189
// ← — move cursor left
@@ -203,19 +210,22 @@ export async function runInteractive(
203210
filterCursor = newPos;
204211
const newRows = buildRows(groups, filterInput, filterTarget, filterRegex);
205212
cursor = Math.min(cursor, Math.max(0, newRows.length - 1));
213+
scrollOffset = Math.min(scrollOffset, cursor);
206214
scheduleStatsUpdate();
207215
} else if ((key === "\x7f" || key === "\b") && filterCursor > 0) {
208216
// Backspace — delete char before cursor
209217
filterInput = filterInput.slice(0, filterCursor - 1) + filterInput.slice(filterCursor);
210218
filterCursor--;
211-
const newRows = buildRows(groups, filterInput, filterTarget, filterRegex);
212-
cursor = Math.min(cursor, Math.max(0, newRows.length - 1));
219+
const newRows2 = buildRows(groups, filterInput, filterTarget, filterRegex);
220+
cursor = Math.min(cursor, Math.max(0, newRows2.length - 1));
221+
scrollOffset = Math.min(scrollOffset, cursor);
213222
scheduleStatsUpdate();
214223
} else if (key === KEY_DELETE && filterCursor < filterInput.length) {
215224
// Del — delete char at cursor
216225
filterInput = filterInput.slice(0, filterCursor) + filterInput.slice(filterCursor + 1);
217-
const newRows = buildRows(groups, filterInput, filterTarget, filterRegex);
218-
cursor = Math.min(cursor, Math.max(0, newRows.length - 1));
226+
const newRows3 = buildRows(groups, filterInput, filterTarget, filterRegex);
227+
cursor = Math.min(cursor, Math.max(0, newRows3.length - 1));
228+
scrollOffset = Math.min(scrollOffset, cursor);
219229
scheduleStatsUpdate();
220230
} else if (key === KEY_SHIFT_TAB) {
221231
// Shift+Tab — cycle filter target (path → content → repo → path)
@@ -243,6 +253,7 @@ export async function runInteractive(
243253
filterCursor += printable.length;
244254
const newRows = buildRows(groups, filterInput, filterTarget, filterRegex);
245255
cursor = Math.min(cursor, Math.max(0, newRows.length - 1));
256+
scrollOffset = Math.min(scrollOffset, cursor);
246257
scheduleStatsUpdate();
247258
}
248259
}

0 commit comments

Comments
 (0)