fix(ui): pad viewport rows to prevent iTerm2 ghost lines on scroll#1240
Open
Juoper wants to merge 1 commit into
Open
fix(ui): pad viewport rows to prevent iTerm2 ghost lines on scroll#1240Juoper wants to merge 1 commit into
Juoper wants to merge 1 commit into
Conversation
Pad each frame line to full terminal cell width so incremental redraw overwrites stale glyphs when scrolling the session list, without using tea.ClearScreen.
📝 WalkthroughWalkthroughThis PR adds terminal cell-width adjustment helpers and applies them throughout viewport clamping logic to ensure consistent line widths for emoji and keycap rendering. The changes prevent glyph leftovers during incremental redraws by enforcing exact cell-width padding and truncation. ChangesCell-width consistency in viewport rendering
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/ui/cellwidth.go`:
- Around line 30-58: Tests currently don't exercise the clampViewToViewport →
fitCellWidth path for RemoteSession rendering; add a test that sets up
Home.remoteSessions with one or more RemoteSession rows, calls Home.View(), and
asserts the output lines produced by fitCellWidth/fitLinesToWidth are correctly
padded or truncated to the expected cell width (use cellWidth expectations for
multibyte/emoji cases), or if coverage cannot be added, add a t.Skipf in the
existing RemoteSession-related tests explaining why RemoteSession frames can't
be validated here; refer to Home.View(), Home.remoteSessions,
clampViewToViewport, fitCellWidth and fitLinesToWidth when locating code to
modify.
In `@internal/ui/issue937_keycap_test.go`:
- Around line 280-296: This test (Test_clampViewToViewport_PadsEveryRow)
exercises clampViewToViewport but doesn't cover the RemoteSession path; either
add a parallel subtest that runs under a RemoteSession context (e.g., initialize
or mock RemoteSession and call clampViewToViewport the same way) to assert
identical padding behavior, or add an explicit t.Skip with a short justification
at the top of Test_clampViewToViewport_PadsEveryRow explaining why RemoteSession
isn't applicable; ensure you reference the RemoteSession initializer/mocker used
elsewhere in internal/ui tests, keep assertions identical (cellWidth check), and
name the subtest to indicate RemoteSession coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 17702318-a442-4957-a8b6-cd7d85dae62e
📒 Files selected for processing (3)
internal/ui/cellwidth.gointernal/ui/home.gointernal/ui/issue937_keycap_test.go
Comment on lines
+30
to
+58
| // fitCellWidth returns s truncated or space-padded so it occupies exactly width | ||
| // terminal cells. Used by clampViewToViewport so each frame overwrites the full | ||
| // row and incremental redraw does not leave ghost characters (issue #607) — | ||
| // avoids tea.ClearScreen / flicker on iTerm2. | ||
| func fitCellWidth(s string, width int) string { | ||
| if width <= 0 { | ||
| return "" | ||
| } | ||
| w := cellWidth(s) | ||
| if w > width { | ||
| return cellTruncate(s, width, "") | ||
| } | ||
| if w < width { | ||
| return s + strings.Repeat(" ", width-w) | ||
| } | ||
| return s | ||
| } | ||
|
|
||
| // fitLinesToWidth applies fitCellWidth to each line of a multi-line string. | ||
| func fitLinesToWidth(content string, width int) string { | ||
| if width <= 0 || content == "" { | ||
| return content | ||
| } | ||
| lines := strings.Split(content, "\n") | ||
| for i := range lines { | ||
| lines[i] = fitCellWidth(lines[i], width) | ||
| } | ||
| return strings.Join(lines, "\n") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== PR files touching internal/ui or cmd/agent-deck =="
git diff --name-only origin/main...HEAD | rg -n '^(internal/ui|cmd/agent-deck)/.*\.go$' || true
echo
echo "== Candidate tests mentioning clamp/viewport/width helpers =="
rg -n --type go -C2 'fitCellWidth|fitLinesToWidth|clampViewToViewport|ensureExactWidth' internal/ui cmd/agent-deck || true
echo
echo "== RemoteSession coverage in tests =="
rg -n --type go -C2 'RemoteSession' internal/ui cmd/agent-deck || true
echo
echo "== Explicit t.Skip rationale mentions (RemoteSession / why) =="
rg -n --type go -C2 't\.Skip\(|Skipf\(' internal/ui cmd/agent-deck || trueRepository: asheshgoplani/agent-deck
Length of output: 50439
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== clampViewToViewport / ensureExactWidth / fitCellWidth usage in tests (w/ file+line) =="
rg -n --type go 'clampViewToViewport\(|ensureExactWidth\(|fitCellWidth\(|fitLinesToWidth\(' internal/ui cmd/agent-deck -g'*_test.go' || true
echo
echo "== Tests that mention RemoteSession AND clamp/width helpers =="
rg -n --type go 'RemoteSession.*(clampViewToViewport|ensureExactWidth|fitCellWidth|fitLinesToWidth)|\(clampViewToViewport|ensureExactWidth|fitCellWidth|fitLinesToWidth\).*RemoteSession' internal/ui -g'*_test.go' || true
echo
echo "== RemoteSession mentioned in tests that render Home.View/frames =="
rg -n --type go 'RemoteSession' internal/ui -g'*_test.go' | head -n 200
echo
rg -n --type go 'home\.View\(|View\(\)' internal/ui -g'*_test.go' || true
echo
rg -n --type go 'RemoteSession.*home\.View|home\.View.*RemoteSession' internal/ui -g'*_test.go' || true
echo
echo "== t.Skip / Skipf occurrences in internal/ui and cmd/agent-deck =="
rg -n --type go 't\.Skip\(|Skipf\(' internal/ui cmd/agent-deck -g'*_test.go' || trueRepository: asheshgoplani/agent-deck
Length of output: 29080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
internal/ui/issue953_counter_test.go
internal/ui/issue1101_remote_preview_test.go
internal/ui/issue1091_remote_render_test.go
internal/ui/issue1112_remote_waiting_status_test.go
internal/ui/issue1100_shift_enter_remote_test.go
)
for f in "${files[@]}"; do
echo "================ $f ================"
nl -ba "$f" | sed -n '1,220p'
echo
done
echo "== Search within these files for width/clamp helpers =="
for f in "${files[@]}"; do
echo "-- $f"
rg -n --type go 'clampViewToViewport|ensureExactWidth|fitCellWidth|fitLinesToWidth' "$f" || true
doneRepository: asheshgoplani/agent-deck
Length of output: 201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
internal/ui/issue953_counter_test.go
internal/ui/issue1101_remote_preview_test.go
internal/ui/issue1091_remote_render_test.go
internal/ui/issue1112_remote_waiting_status_test.go
internal/ui/issue1100_shift_enter_remote_test.go
)
for f in "${files[@]}"; do
echo "================ $f (first 220 lines) ================"
stat "$f" | cat
cat -n "$f" | head -n 220
echo
done
echo "== Search within these files for width/clamp helpers =="
for f in "${files[@]}"; do
echo "-- $f"
rg -n --type go 'clampViewToViewport|ensureExactWidth|fitCellWidth|fitLinesToWidth' "$f" || true
echo
doneRepository: asheshgoplani/agent-deck
Length of output: 221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
internal/ui/issue953_counter_test.go
internal/ui/issue1101_remote_preview_test.go
internal/ui/issue1091_remote_render_test.go
internal/ui/issue1112_remote_waiting_status_test.go
internal/ui/issue1100_shift_enter_remote_test.go
)
for f in "${files[@]}"; do
echo "================ $f ================"
echo "-- RemoteSession / remoteSessions occurrences --"
rg -n --type go 'RemoteSession|remoteSessions' "$f" || true
echo "-- Width/clamp helper occurrences --"
rg -n --type go 'clampViewToViewport|ensureExactWidth|fitCellWidth|fitLinesToWidth' "$f" || true
echo "-- Home.View() occurrences --"
rg -n --type go 'home\.View\(' "$f" || true
echo "-- Any View() call occurrences --"
rg -n --type go '\bView\(\)' "$f" || true
echo
doneRepository: asheshgoplani/agent-deck
Length of output: 3966
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== issue953_counter_test.go relevant section =="
sed -n '1,210p' internal/ui/issue953_counter_test.go
echo
echo "== Home.View() implementation snippet (should call clampViewToViewport) =="
rg -n --type go 'func \(.*\) View\(' internal/ui/home.go | head -n 20
# Print around the clampViewToViewport call site we saw earlier.
rg -n --type go 'return clampViewToViewport\(' internal/ui/home.go
# Show a small window around the clampViewToViewport return.
clamp_line=$(rg -n --type go 'return clampViewToViewport\(' internal/ui/home.go | head -n 1 | cut -d: -f1)
start=$((clamp_line-20))
end=$((clamp_line+20))
sed -n "${start},${end}p" internal/ui/home.goRepository: asheshgoplani/agent-deck
Length of output: 8136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Files that mention RemoteSession / RemoteSessionInfo
rg -l --type go 'RemoteSession' internal/ui cmd/agent-deck -g'*_test.go' > /tmp/remote_files.txt || true
# Files that call home.View()
rg -l --type go 'home\.View\(' internal/ui -g'*_test.go' > /tmp/home_view_files.txt || true
echo "== RemoteSession files =="
wc -l /tmp/remote_files.txt; head -n 50 /tmp/remote_files.txt || true
echo "== home.View() files =="
wc -l /tmp/home_view_files.txt; head -n 50 /tmp/home_view_files.txt || true
# Intersection
comm -12 <(sort -u /tmp/remote_files.txt) <(sort -u /tmp/home_view_files.txt) | sed -n '1,200p'
echo "== Files with RemoteSession AND home.View() (count) =="
comm -12 <(sort -u /tmp/remote_files.txt) <(sort -u /tmp/home_view_files.txt) | wc -l
# Also check for any direct View() calls on a Home instance with RemoteSession in the same file.
echo "== Files with RemoteSession AND Home.View() anywhere (home.View or .View() on Home var) =="
rg -l --type go '(RemoteSession.*|.*RemoteSession)\s*\n.*\.View\(' internal/ui -g'*_test.go' || trueRepository: asheshgoplani/agent-deck
Length of output: 827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
remote_files=/tmp/remote_files.txt
home_view_files=/tmp/home_view_files.txt
# Files that mention RemoteSession
rg -l --type go 'RemoteSession' internal/ui cmd/agent-deck -g'*_test.go' > "$remote_files" || true
# Files that call home.View()
rg -l --type go 'home\.View\(' internal/ui -g'*_test.go' > "$home_view_files" || true
echo "== Intersection (RemoteSession files ∩ home.View() files) =="
# grep -Fxf expects one file list in pattern; use grep with -x for exact lines.
grep -Fxf "$remote_files" "$home_view_files" || true
echo
echo "== Quick check: in those intersection files, any t.Skip rationale mentioning RemoteSession? =="
intersection="$(grep -Fxf "$remote_files" "$home_view_files" || true)"
if [ -n "$intersection" ]; then
for f in $intersection; do
echo "-- $f"
rg -n --type go 't\.Skip\(|Skipf\(' "$f" || true
done
else
echo "No intersection files found."
fiRepository: asheshgoplani/agent-deck
Length of output: 355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== issue953_counter_test.go: home.View() call sites =="
rg -n --type go 'home\.View\(' internal/ui/issue953_counter_test.go
echo
echo "== issue953_counter_test.go: nearby remoteSessions / RemoteSession setup near home.View() =="
# show a window around the home.View() call
view_line=$(rg -n --type go 'home\.View\(' internal/ui/issue953_counter_test.go | head -n 1 | cut -d: -f1)
start=$((view_line-60)); end=$((view_line+60))
sed -n "${start},${end}p" internal/ui/issue953_counter_test.go
echo
echo "== home_test.go: home.View() call sites =="
rg -n --type go 'home\.View\(' internal/ui/home_test.go
echo
echo "== home_test.go: check remote setup near each home.View() occurrence =="
# For each View() line, print a small context and look for remoteSessions / RemoteSession / ItemTypeRemoteSession
rg -n --type go 'home\.View\(' internal/ui/home_test.go | while read -r line; do
ln=$(echo "$line" | cut -d: -f1)
echo "---- home_test.go:$ln ----"
sed -n "$((ln-50)),$((ln+50))p" internal/ui/home_test.go | rg -n 'remoteSessions|RemoteSession|ItemTypeRemoteSession|remotePreviewCacheKey' || true
done
echo
echo "== home_test.go: any Home.View tests that explicitly set flatItems with RemoteSession just before View() =="
rg -n --type go 'ItemTypeRemoteSession|RemoteSessionInfo|remoteSessions' internal/ui/home_test.go -g'*_test.go' | head -n 80Repository: asheshgoplani/agent-deck
Length of output: 6780
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show context around remote flatItems assignments and the next View() usage.
ranges=(
"1260,1320"
"1310,1380"
"1360,1425"
"1410,1475"
"1460,1525"
"2940,3020"
)
for r in "${ranges[@]}"; do
echo "==== internal/ui/home_test.go lines $r ===="
sed -n "${r}p" internal/ui/home_test.go | rg -n 'RemoteSession|RemoteSessionInfo|ItemTypeRemoteSession|flatItems|home\.View\(' || true
done
echo
echo "== List all home.View() occurrences with nearby line window in home_test.go =="
rg -n --type go 'home\.View\(' internal/ui/home_test.go | while read -r line; do
ln=$(echo "$line" | cut -d: -f1)
echo "-- home.View() at $ln --"
sed -n "$((ln-20)),$((ln+20))p" internal/ui/home_test.go | rg -n 'RemoteSession|remoteSessions|ItemTypeRemoteSession|flatItems' || true
doneRepository: asheshgoplani/agent-deck
Length of output: 4112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the blocks around each home.View() call and also any nearby remote flatItems assignments.
view_lines=(195 238 1755 1784 1842 1874 2351)
for ln in "${view_lines[@]}"; do
start=$((ln-120)); end=$((ln+120))
echo "================ home_test.go around home.View() at $ln ($start-$end) ================"
sed -n "${start},${end}p" internal/ui/home_test.go | rg -n 'RemoteSessionInfo|ItemTypeRemoteSession|RemoteSession:|flatItems|remote|Home\.View|home\.View|previewCache|render' || true
done
echo
echo "== List remote flatItems assignments in home_test.go =="
rg -n 'home\.flatItems\s*=\s*\[\]session\.Item\{\{Type:\s*session\.ItemTypeRemoteSession' internal/ui/home_test.go
echo
echo "== List all home.flatItems assignments in home_test.go (near those view calls) =="
# Only print flatItems assignments that are within 600 lines of any View() call we care about.
# Build a set of candidate line numbers by grepping for View() and then checking nearby flatItems lines.
rg -n --type go 'home\.View\(' internal/ui/home_test.go | awk -F: '{print $1}' > /tmp/view_lines.txt
python3 - <<'PY'
import re
views=[int(x.strip()) for x in open('/tmp/view_lines.txt') if x.strip()]
flat=[]
with open('internal/ui/home_test.go','r',encoding='utf-8') as f:
for i,line in enumerate(f, start=1):
if re.search(r'home\.flatItems\s*=', line):
# keep if close to any view line
if any(abs(i-v)<=600 for v in views):
flat.append(i)
for i in sorted(set(flat)):
print(i)
PYRepository: asheshgoplani/agent-deck
Length of output: 6127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re, pathlib
def analyze(path):
text = pathlib.Path(path).read_text(encoding='utf-8')
lines = text.splitlines()
# Find home.View() call sites
view_idxs = [i for i,l in enumerate(lines) if re.search(r'\bhome\.View\(\)', l)]
print(f"\n== {path} home.View() call sites: {len(view_idxs)} ==")
for i in view_idxs:
# Find enclosing func Test... by scanning backwards to last func start
start = None
for j in range(i, -1, -1):
if re.match(r'^\s*func\s+Test[A-Za-z0-9_]*\s*\(.*\)\s*\{', lines[j]):
start = j
break
if start is None:
print(f"- View at line {i+1}: no enclosing Test func found")
continue
# Brace count forward to find end of that func
brace = 0
end = None
for j in range(start, len(lines)):
brace += lines[j].count('{')
brace -= lines[j].count('}')
if j>start and brace<=0:
end = j
break
if end is None:
end = min(len(lines)-1, start+400)
block = "\n".join(lines[start:end+1])
has_remote = bool(re.search(r'\bRemoteSession\b|remoteSessionsMu\.|remoteSessions\b|ItemTypeRemoteSession', block))
has_skip_remote = bool(re.search(r't\.Skipf?\(|Skipf?\(', block) and re.search(r'RemoteSession|remoteSessions', block))
print(f"- View at line {i+1} inside func at line {start+1}-{end+1}: remote-related={has_remote}, skip-remote-rationale={has_skip_remote}")
# Print a tiny window with relevant markers if any
if has_remote:
# show up to 15 lines around first matching marker in the block
m = re.search(r'(\bRemoteSession\b|remoteSessionsMu\.|remoteSessions\b|ItemTypeRemoteSession)', block)
# find its approximate line in file
# We'll just print the first 80 lines of the block that contain any remote markers.
for k in range(start, end+1):
if re.search(r'\bRemoteSession\b|remoteSessionsMu\.|remoteSessions\b|ItemTypeRemoteSession', lines[k]):
a=max(start, k-3); b=min(end, k+5)
snippet="\n".join(f"{t+1}:{lines[t]}" for t in range(a,b+1))
print(" snippet:\n"+snippet)
break
for p in [
"internal/ui/issue953_counter_test.go",
"internal/ui/home_test.go",
]:
analyze(p)
PYRepository: asheshgoplani/agent-deck
Length of output: 995
Add RemoteSession-based coverage for the clampViewToViewport/fitCellWidth path
- The
Home.View()rendering path clamps viaclampViewToViewport→fitCellWidth, but the existing tests that exercise width/padding (e.g.,internal/ui/issue937_keycap_test.go) don’t coversession.RemoteSessionInfo/RemoteSession rendering. - In the RemoteSession-related files that call
home.View()(internal/ui/issue953_counter_test.go,internal/ui/home_test.go), thehome.View()assertions run with non-RemoteSession snapshots (RemoteSession setup happens in other tests that don’t callhome.View()), so RemoteSession frames aren’t validated for the width clamping behavior. - No
t.Skip/Skipfrationale exists for why RemoteSession isn’t covered.
Add a test that populates Home.remoteSessions with RemoteSession rows and asserts the resulting Home.View() output is correctly cell-width padded/truncated (or add an explicit t.Skip with rationale).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/ui/cellwidth.go` around lines 30 - 58, Tests currently don't
exercise the clampViewToViewport → fitCellWidth path for RemoteSession
rendering; add a test that sets up Home.remoteSessions with one or more
RemoteSession rows, calls Home.View(), and asserts the output lines produced by
fitCellWidth/fitLinesToWidth are correctly padded or truncated to the expected
cell width (use cellWidth expectations for multibyte/emoji cases), or if
coverage cannot be added, add a t.Skipf in the existing RemoteSession-related
tests explaining why RemoteSession frames can't be validated here; refer to
Home.View(), Home.remoteSessions, clampViewToViewport, fitCellWidth and
fitLinesToWidth when locating code to modify.
Comment on lines
+280
to
+296
| // Test_clampViewToViewport_PadsEveryRow ensures the final frame pads narrow lines. | ||
| func Test_clampViewToViewport_PadsEveryRow(t *testing.T) { | ||
| const width, height = 30, 3 | ||
| in := strings.Join([]string{ | ||
| strings.Repeat("X", width), | ||
| "short", | ||
| "", | ||
| }, "\n") | ||
| out := clampViewToViewport(in, width, height) | ||
| lines := strings.Split(out, "\n") | ||
| if len(lines) != height { | ||
| t.Fatalf("want %d lines, got %d", height, len(lines)) | ||
| } | ||
| if got := cellWidth(lines[1]); got != width { | ||
| t.Fatalf("line 1 cellWidth = %d; want %d (must pad, not only truncate)", got, width) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add RemoteSession coverage (or explicit t.Skip) for this TUI behavior change.
This regression test validates padding behavior, but it does not verify the same behavior under RemoteSession conditions, and there is no explicit t.Skip explaining why that path is excluded. Please add a RemoteSession variant (or document a justified skip) to satisfy the TUI test requirement.
As per coding guidelines, "If a PR changes TUI behavior in internal/ui/ or cmd/agent-deck/, verify it also handles RemoteSession (or has an explicit t.Skip documenting why)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/ui/issue937_keycap_test.go` around lines 280 - 296, This test
(Test_clampViewToViewport_PadsEveryRow) exercises clampViewToViewport but
doesn't cover the RemoteSession path; either add a parallel subtest that runs
under a RemoteSession context (e.g., initialize or mock RemoteSession and call
clampViewToViewport the same way) to assert identical padding behavior, or add
an explicit t.Skip with a short justification at the top of
Test_clampViewToViewport_PadsEveryRow explaining why RemoteSession isn't
applicable; ensure you reference the RemoteSession initializer/mocker used
elsewhere in internal/ui tests, keep assertions identical (cellWidth check), and
name the subtest to indicate RemoteSession coverage.
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
1 participant
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.
Pad each frame line to full terminal cell width so incremental redraw overwrites stale glyphs when scrolling the session list, without using tea.ClearScreen.
Summary by CodeRabbit
Bug Fixes
Tests