Skip to content

fix(terminal): repaint the viewport after an inline image is added - #29713

Merged
nwparker merged 1 commit into
mainfrom
fix/inline-image-partial-repaint
Oct 16, 2026
Merged

nwparker merged 1 commit into
mainfrom
fix/inline-image-partial-repaint

Conversation

@nwparker

@nwparker nwparker commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​65 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​65
Prod 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​67 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​67

ELI5

Inline terminal images (iTerm2, SIXEL, Kitty) sometimes showed up as a thin 4-pixel strip instead of the full picture. The image was decoded and stored correctly, but only its last row ever got drawn. Orca now redraws the terminal once right after an image is added, so the whole picture is painted.

What Changed

  • Before: with GPU acceleration off (xterm's DOM renderer), an image whose rows were written by a write that Orca's per-write repair didn't cover only got repainted on the row the cursor sat on afterwards. Orca's repair covers only the rows a write touched, and only for writes it flags (for example, writes containing \r). The decoders add images asynchronously, sometimes after their write has already been parsed. The image's other rows were left to xterm's frame-debounced refresh. In a hidden or throttled window that frame effectively never ran, so the image stayed a strip until something else repainted the grid. The real WSL CI lane (headful, hidden window, GPU off) caught this about 15% of the time.
  • After: a new terminal-image-added-repaint.ts subscribes to the image addon's public onImageAdded event. Once per burst of added images (coalesced in a microtask), it requests one synchronous repaint of the viewport, using the same _core.refresh(start, end, true) Orca's foreground repair already uses. pane-inline-images.ts wires it up when the addon attaches and disposes it when the addon detaches.

Why

Evidence, from instrumented dispatch runs of windows-wsl-e2e.yml:

  • Failing state: in the failing state the addon's storage is correct. All 45 tiles per image are in the right cells, the bitmaps are valid, and the cell size is unchanged. The image canvas holds only the 4-pixel strips, and a forced full refresh paints the images completely.
  • Render trace: an IIP image added at rows 3–5 got a debounced refreshRows 3-5, but no render ever covered rows 3–4. Every render that happened was Orca's synchronous per-write repair: 5-6, 6-7, and so on. Across 10 seconds, xterm's own frame-debounced renders ran only 3 times. The same pattern held for the SIXEL and Kitty images.
  • Before and after: without the fix, 24 of 160 GPU-off WSL runs failed. With the fix, two 20-run soaks passed 40 of 40.

Alternatives considered:

  • Widening Orca's per-write repair to cover image writes. That means detecting image sequences in the output scan and still misses the asynchronous decode, which finishes after the write.
  • Patching the vendored addon to request its own refresh. That means editing minified lib/ output in a dependency patch.
  • Making the test force a repaint. That would hide a real painting gap in background windows.

The event-based repaint is small, uses a public addon API, and does one viewport repaint per burst of images, not per write. A hidden pane (xterm's render pause) still does no work: xterm just marks it for a full refresh when it becomes visible.

Linked Issue

N/A (found while root-causing the real WSL terminal CI lane flake; follows #29633)

Visual Proof

Failing CI screenshot: red, green and blue images show as 4-pixel strips. After a forced full refresh of the same terminal, all three render fully. Both are in the windows-wsl-terminal-traces artifact of run 38907618010.

Testing

  • I manually tested these changes locally: not on a desktop, because of background-launch rules. I validated on the Windows WSL runner instead (two dispatch soaks, 40 of 40 GPU-off runs).
  • Automated tests added/updated, or explained why not below: terminal-image-added-repaint.test.ts covers coalescing, the sync repaint range, the fallback without _core, disposal (including an already-queued repaint), and addon builds without the event.

AI Disclosure

Review

Shared terminal rendering code, so this is left open for owner review.

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Platform: renderer-only. It's not specific to any platform and applies to both the WebGL and DOM renderers.
  • Remote and wire compatibility: SSH and remote terminals render through the same pane code, so they're covered too. There's no wire or protocol change.
  • Performance: one synchronous viewport repaint per microtask burst of images. Image adds are rare next to text writes. Continuous Kitty animations cost one repaint per frame, which is the cost of showing those frames anyway.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@coderabbitai

coderabbitai Bot commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: eace0f79-792d-476d-97bf-872294c79dea
📥 Commits

Reviewing files that changed from the base of the PR and between eada95e and 678dd0f.

📒 Files selected for processing: 3
  • src/renderer/src/lib/pane-manager/pane-inline-images.ts
  • src/renderer/src/lib/pane-manager/terminal-image-added-repaint.test.ts
  • src/renderer/src/lib/pane-manager/terminal-image-added-repaint.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

The change adds a helper that coalesces image-added events into asynchronous terminal viewport repaints. Pane attachment starts the handling, and pane detachment disposes it. Tests cover event coalescing, refresh fallbacks, disposal, and addons without image-added events.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 678dd

The change repaints the terminal viewport after an inline image is added, which fixes the leftover image strip. No merge-blocking risk is evident, and the change is covered by focused tests.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Description check Warning The description explains the user impact, mechanism, rationale, alternatives, testing, compatibility, performance, and checklist status. However, the required Linked Issue section is marked N/A even t… Replace "N/A" in Linked Issue with the actual issue reference, such as "Fixes #29633", if that is the issue addressed by this PR.
Docstring Coverage Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: repainting the terminal viewport after an inline image is added.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the user impact, mechanism, rationale, alternatives, testing, compatibility, performance, and checklist status. However, the required Linked Issue section is marked N/A even though the template requires an actual issue link.

Resolution

Replace "N/A" in Linked Issue with the actual issue reference, such as "Fixes #29633", if that is the issue addressed by this PR.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nwparker
nwparker merged commit 54bb885 into main Oct 16, 2026
30 checks passed
@s546126 s546126 mentioned this pull request Nov 4, 2026
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant