Fix TUI header scrolling off-screen due to viewport overflow#106
Merged
Fix TUI header scrolling off-screen due to viewport overflow#106
Conversation
Multiple root causes all sharing the same symptom (the title line pushed above the top of the terminal when scrolling): 1. getViewportHeight() stickyHeaderLines condition (tui.ts) The original condition `scrollOffset > 0 && cursor >= scrollOffset` fired on repo rows too, over-reserving 1 line and shrinking the viewport by 1. Fix: mirror the exact 3-condition check used in renderGroups (extract type + findIndex for repo row + repoRowIndex < scrollOffset). 2. Hints line wrapping (render.ts) The hints string (~158 visible chars) wrapped on any terminal < 159 cols, producing 2 physical lines while HEADER_LINES=4 assumed 1. Fix: clip HINTS_TEXT to termWidth before rendering. 3. Section label wrapping (render.ts) Long team names like 'squad-architecture-and-platform-…' made the '── <label> ' line exceed termWidth → wrap → title pushed off. Fix: clip label to max(4, termWidth - 4) chars. 4. Fragment lines wrapping (render.ts + render/highlight.ts) MAX_LINE_CHARS=120 + 6-char indent = 126 visible chars max. On terminals < 127 cols fragments wrapped, underestimating the physical line count. Fix: pass fragmentMaxChars = termWidth-6-1 to highlightFragment() so every fragment line fits in 1 terminal row. 5. All remaining header/content lines clipped (render.ts) Added clipAnsi() helper that clips ANSI-aware strings to a visible character budget. Applied to: title line, summary line, sticky repo line, repo row name, extract path line, and filter-bar lines. 6. Section row budget guard missing (render.ts) Section rows (2 lines: blank + label) were pushed to lines[] before checking whether they fit in the remaining viewport. When the cursor was on a folded repo at the bottom of the viewport and the next row was a section, usedLines exceeded viewportHeight by 2 → title scrolled off. Unfolding the repo consumed the remaining space before the section was reached, hiding the bug. Fix: add the same 'if (usedLines + 2 > viewportHeight && usedLines > 0) break' guard that repo/extract rows already had. Fixes #105
|
Coverage after merging fix/tui-title-scrolls-off-viewport into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a TUI rendering/scrolling bug where the header/title could scroll off-screen due to the rendered output exceeding the terminal viewport by 1+ physical lines, especially on narrow terminals or around section boundaries.
Changes:
- Aligns
tui.tsviewport height calculations with the exact sticky-header rendering condition used byrenderGroups. - Adds ANSI-aware clipping and width-based truncation across header, section, repo, extract, and fragment rendering to prevent terminal line wrapping.
- Adds regression tests covering narrow
termWidthscenarios and section-boundary viewport budgeting.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/tui.ts | Updates viewport-height logic to match sticky-header rendering conditions for correct scroll calculations. |
| src/render/highlight.ts | Extends highlightFragment with an optional maxLineChars to support width-aware truncation. |
| src/render.ts | Introduces clipAnsi and applies clipping/guards to prevent wrapped lines and viewport overflow. |
| src/render.test.ts | Adds/updates regression tests for narrow-width clipping and section boundary overflow cases. |
…#105) Three follow-up fixes addressing review comments on PR #106 plus two new viewport layout bugs: ── Review comment fixes ──────────────────────────────────────────────────── 1. render.ts: fragmentMaxChars floor lowered from Math.max(20,…) to Math.max(1,…) so very narrow terminals never hit an artificial 20-char minimum that still exceeded the available width. 2. render.ts: extract path prefix width corrected for inactive rows — was subtracting PATH_INDENT twice, producing a 1-char-too-short clip width. 3. tui.ts: getViewportHeight replaced rows.findIndex() O(n) scan with a lazily-rebuilt Map<repoIndex,rowIndex> (getRepoRowIndexMap) for O(1) lookup per call. ── Section-first-viewport double-blank (footer pushed off screen) ────────── Section rows embedded a leading \n in their rendered string. When a section was the first row in the viewport (usedLines===0), the hints header already ends with \n and lines.join("\n") adds a second \n separator before the section's own \n → 2 blank lines → 1 physical line over budget → the footer position indicator (↕ row X of Y) was pushed below the terminal bottom. Fix (render.ts): remove the embedded \n from the section string; emit an empty string as blank separator only when usedLines > 0; use sectionCost = usedLines === 0 ? 1 : 2. Mirror the same conditional cost in isCursorVisible (rows.ts) so viewport accounting stays consistent. ── Footer floating above the bottom when viewport is sparse ─────────────── The footer was appended immediately after the last rendered item. When the viewport was not full (few results, repos folded, cursor near the end), the footer floated up instead of staying at the bottom of the terminal. Fix (render.ts): push (viewportHeight - usedLines) blank lines before the footer so the total output is always exactly termHeight lines. ── Blank gap between last content row and footer ────────────────────────── After fold/unfold, filter change, or navigating near the end, the rows visible from scrollOffset to the end could occupy fewer than viewportHeight lines — leaving visible padding above the footer even though rows above scrollOffset could fill it. Fix (rows.ts): new pure function normalizeScrollOffset() decreases scrollOffset as long as prepending the next row above still fits within viewportHeight (same section-cost rules as renderGroups). Called in redraw() (tui.ts) before every renderGroups call, covering all triggers (navigation, fold/unfold, filter) without duplicating logic in each handler. Regression tests added for all four cases.
|
Coverage after merging fix/tui-title-scrolls-off-viewport into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage after merging fix/tui-title-scrolls-off-viewport into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…izeScrollOffset fixed-point Four follow-up fixes from code review on PR #106: 1. clipAnsi: replace \x1b[0m (full SGR reset) with \x1b[22;39m (reset bold + fg only). A full reset clears any background set by the caller, so renderActiveLine's dark-purple bg was wiped mid-line on narrow terminals when the repo name or file path was clipped. 2. Section label width: change Math.max(4, termWidth - SECTION_FIXED) to Math.max(0, …). The old floor of 4 could exceed the actual available width on very narrow terminals, forcing the section header line wider than termWidth and re-introducing wrapping. When maxLabelChars === 0 the section is counted toward usedLines but not pushed to output. 3. Extract path width: change Math.max(10, …) to Math.max(1, …). Same reasoning as above — the floor of 10 could exceed the available width on very narrow terminals. 4. normalizeScrollOffset fixed-point loop: replace single-pass call with a loop that iterates until (scrollOffset, viewportHeight) stabilises. getViewportHeight depends on scrollOffset (sticky-header condition), so decreasing scrollOffset can toggle the sticky header, changing viewportHeight by 1 and causing a single pass to stop 1 row early.
|
Coverage after merging fix/tui-title-scrolls-off-viewport into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…, repo row floor Four follow-up fixes from code review on PR #106: 1. clipAnsi docstring: updated to document the partial SGR reset (\x1b[22;39m) and explain why the background is deliberately preserved. Added note about code-point iteration to match the new implementation. 2. clipAnsi surrogate pairs: the loop now advances i by codePointAt() width (2 UTF-16 units for non-BMP code points, 1 otherwise) instead of always i++. This prevents clipping in the middle of a surrogate pair when maxVisible falls between the two halves of an emoji (e.g. the 🔍 prefix used in filter lines). 3. Section row maxLabelChars === 0: instead of incrementing usedLines without pushing to lines[], the code now pushes blank placeholder strings (same count as sectionCost) to keep lines[] in sync with usedLines. Without this, the footer-padding loop adds too few blank lines and the output is sectionCost lines shorter than termHeight. 4. Repo row maxLeftVisible floor: Math.max(4, …) → Math.max(1, …). The old floor of 4 could exceed the available width on very narrow terminals (termWidth < 4 + countLen + barAdjust), reintroducing wrapping.
|
Coverage after merging fix/tui-title-scrolls-off-viewport into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
shouze
force-pushed
the
fix/tui-title-scrolls-off-viewport
branch
from
241456a to
3d824ac
Compare
|
Coverage after merging fix/tui-title-scrolls-off-viewport into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage after merging fix/tui-title-scrolls-off-viewport into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
The TUI title line (
github-code-search <query> in <org>) occasionally scrolled above the top of the terminal when navigating the results list. Six independent root causes were found — each capable of pushing the rendered output beyondtermHeightby exactly 1 line.Root causes & fixes
1.
getViewportHeight()sticky-header condition too broad (tui.ts)The original condition
scrollOffset > 0 && cursor >= scrollOffsetover-reserved 1 line for the sticky repo header even onreporows (where no sticky header is rendered). Fix: mirror the exact 3-condition check fromrenderGroups(extracttype +findIndexfor the repo row +repoRowIndex < scrollOffset).2. Hints line wrapping (
render.ts)The hints string (
← / → fold/unfold … q quit) is ~158 visible chars. On any terminal < 159 cols it wrapped to 2 physical lines, butHEADER_LINES = 4assumed 1. Fix: clipHINTS_TEXTtotermWidthbefore rendering.3. Section label wrapping (
render.ts)Long team names like
squad-architecture-and-platform-…made the── <label>line exceedtermWidth→ wrap → extra line. Fix: clip the label tomax(4, termWidth − 4)chars.4. Fragment lines wrapping (
render.ts+render/highlight.ts)MAX_LINE_CHARS = 120+ 6-char indent = 126 visible chars. On terminals < 127 cols, fragments wrapped.rowTerminalLines()counted 1 logical line per fragment regardless, so the viewport budget underestimated the physical line count. Fix: passfragmentMaxChars = termWidth − 6 − 1tohighlightFragment()(new optional parameter) so every rendered line fits in exactly 1 terminal row.5. All remaining header/content lines unclipped (
render.ts)Added
clipAnsi(str, maxVisible)— an ANSI-aware clip helper that counts visible characters correctly and injects\x1b[0mat the cut point. Applied to: title line, summary line, sticky repo line, repo row (repo name clipped totermWidth − countLen − barAdjust), extract path line, and filter-bar lines.6. Section row budget guard missing (
render.ts)Section rows (blank separator + label = 2 lines) were pushed to
lines[]before checking whether they fit in the remaining viewport. Repo/extract rows already had the guard. Without it: cursor on a folded repo near the bottom of the viewport, next row is a section → usedLines exceeded viewportHeight by 2 → title scrolled off. Unfolding the repo consumed the remaining space before the section was reached, which explained the "folded: title gone / unfolded: title back" behaviour.Steps to reproduce (before the fix)
bun github-code-search.ts --org fulll "@fulll/aws-extra" --group-by-team-prefix squad-,chapter-Steps to verify (after the fix)
bun github-code-search.ts --org fulll "@fulll/aws-extra" --group-by-team-prefix squad-,chapter-Navigate freely with ↑/↓, fold/unfold repos with ←/→, scroll through section boundaries — the title line stays visible at all times.
Test coverage
6 regression tests added to
src/render.test.ts(558 tests total, 0 fail):Closes #105