Analyzed image escape sequences and rendering flow

Summary

In Progress

Understand
Reviewed image rendering flow and escape sequences.
Understand
Understand
Explained single-row versus multi-row images.
Understand
Understand
Detailed VT code sequences emitted by image render.
Understand
Plan
Discussed optimization for image line detection.
Plan

You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/1091 i dont understand how this can happen at all

For each PR URL, do the following in order:

Read the PR page in full. Include description, all comments, all commits, and all changed files.
Identify any linked issues referenced in the PR body, comments, commit messages, or cross links. Read each issue in full, including all comments.
Analyze the PR diff. Read all relevant code files in full with no truncation from the current main branch and compare against the diff. Do not fetch PR file blobs unless a file is missing on main or the diff context is insufficient. Include related code paths that are not in the diff but are required to validate behavior.
Check for a changelog entry in the relevant packages/*/CHANGELOG.md files. Report whether an entry exists. If missing, state that a changelog entry is required before merge and that you will add it if the user decides to merge. Follow the changelog format rules in AGENTS.md. Verify:
- Entry uses correct section (### Breaking Changes, ### Added, ### Fixed, etc.)
- External contributions include PR link and author: Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))
- Breaking changes are in ### Breaking Changes, not just ### Fixed
Check if packages/coding-agent/README.md, packages/coding-agent/docs/.md, packages/coding-agent/examples/**/.md require modification. This is usually the case when existing features have been changed, or new features have been added.
Provide a structured review with these sections:
- Good: solid choices or improvements
- Bad: concrete issues, regressions, missing tests, or risks
- Ugly: subtle or high impact problems
Add Questions or Assumptions if anything is unclear.
Add Change summary and Tests.

Output format per PR: PR: Changelog:

... Good:
... Bad:
... Ugly:
... Questions or Assumptions:
... Change summary:
... Tests:
...

If no issues are found, say so under Bad and Ugly.

Let me check where image escape sequences could come from and understand the flow better:

Now I understand the bug! Let me verify the scenarios more clearly:

PR: https://github.com/badlogic/pi-mono/pull/1091

Changelog:

Entry exists: Yes, in packages/tui/CHANGELOG.md under ## [Unreleased] > ### Fixed
Format correct: Yes, internal fix format used (no external contributor attribution needed since this is from @zedrdave, an external contributor). Needs correction: Should include PR link and author attribution per AGENTS.md rules for external contributions.

Good:

The fix is correct: using includes() instead of startsWith() properly detects image escape sequences anywhere in a line
Removes dead code (imageEscapePrefix variable and getImageEscapePrefix() function) that cached based on terminal capabilities
New implementation is simpler and no longer depends on terminal capabilities detection
Comprehensive test coverage: 31 new tests for isImageLine() with regression, negative, and edge cases
Tests cover both Kitty and iTerm2 protocols

Bad:

PR description misidentifies the root cause: The description claims the bug is about "terminal doesn't support images" causing getImageEscapePrefix() to return null. While this is a bug path, it's not the primary crash scenario.

The actual root cause: When Image.render() outputs multi-row images, it prepends a cursor-up escape sequence:

typescript

const moveUp = result.rows > 1 ? `\x1b[${result.rows - 1}A` : "";lines.push(moveUp + result.sequence);

const moveUp = result.rows > 1 ? `\x1b[${result.rows - 1}A` : "";lines.push(moveUp + result.sequence);

This creates lines like \x1b[2A\x1b]1337;File=... where the image sequence is NOT at the start, so startsWith() fails even on terminals that fully support images.

Changelog entry needs external contributor attribution: should be Fixed X ([#1091](https://github.com/badlogic/pi-mono/pull/1091) by [@zedrdave](https://github.com/zedrdave))

Ugly:

The regression tests simulate the "terminal without image support" scenario but don't test the actual cursor-up prefix scenario which is the more common trigger in practice. A test like this would be valuable:

typescript

it("detects image with cursor-up prefix (multi-row image)", async () => {  const { isImageLine } = await import("../src/terminal-image.js");  // This is what Image.render() actually produces for multi-row images  const line = "\x1b[3A\x1b]1337;File=inline=1:base64data==\x07";  assert.strictEqual(isImageLine(line), true);});

it("detects image with cursor-up prefix (multi-row image)", async () => {  const { isImageLine } = await import("../src/terminal-image.js");  // This is what Image.render() actually produces for multi-row images  const line = "\x1b[3A\x1b]1337;File=inline=1:base64data==\x07";  assert.strictEqual(isImageLine(line), true);});

Questions or Assumptions:

How does a line with image escape sequences get rendered when the terminal doesn't support images? The Image component returns fallback text (not escape sequences) when caps.images is null. The PR description suggests this is the crash scenario, but the code path doesn't seem to produce image escape sequences in that case. The cursor-up prefix scenario (multi-row images on supported terminals) seems like the actual bug trigger.

Change summary:

isImageLine() changed from capability-aware startsWith() to capability-agnostic includes() for both Kitty (\x1b_G) and iTerm2 (\x1b]1337;File=) sequences
Removed unused imageEscapePrefix cache and getImageEscapePrefix() function
Added 31 new tests across 2 new test files

Tests:

New file: packages/tui/test/terminal-image.test.ts (19 tests)
New file: packages/tui/test/bug-regression-isimageline-startswith-bug.test.ts (12 tests)
Coverage includes: both protocols, various positions in line, long lines, negative cases, mixed content

Your question "how can this happen at all": The bug happens primarily because multi-row images have a cursor-up prefix (\x1b[XA) before the actual image escape sequence. The Image component does this to position the cursor correctly before outputting the image data. So startsWith() fails even on fully image-capable terminals because the line starts with \x1b[3A (or similar), not the image protocol prefix.

there was a pr merge from a user called can, guess we need to revert that

Let me check PR #1084 from @can1357:

Found it. PR #1084 from @can1357 introduced the bug. Here's what happened:

Before #1084 (working code in tui.ts):

typescript

private containsImage(line: string): boolean {    return line.includes("\x1b_G") || line.includes("\x1b]1337;File=");}

private containsImage(line: string): boolean {    return line.includes("\x1b_G") || line.includes("\x1b]1337;File=");}

After #1084 (broken code in terminal-image.ts):

typescript

export function isImageLine(line: string): boolean {    const prefix = getImageEscapePrefix();    return prefix !== null && line.startsWith(prefix);}

export function isImageLine(line: string): boolean {    const prefix = getImageEscapePrefix();    return prefix !== null && line.startsWith(prefix);}

The performance optimization changed includes() to startsWith(), which broke detection for:

Multi-row images (have cursor-up prefix \x1b[XA before the image sequence)
Terminals without image support (prefix is null, always returns false)

You don't need a full revert of #1084. PR #1091 is the correct targeted fix:

Keeps isImageLine() in terminal-image.ts (good organization)
Changes back to includes() (fixes the bug)
Removes the capability-based caching (removes the null prefix issue)
Keeps the other #1084 optimizations (box cache, in-place mutation)

The irony: @can1357's optimization traded correctness for performance. The startsWith() check is faster than includes(), but it doesn't work when the image escape sequence isn't at position 0.

Should I merge #1091 with the changelog attribution fix?

yes

then do a patch release

Done. PR #1091 merged and released as v0.50.5.

Summary:

Merged fix from @zedrdave that changes isImageLine() from startsWith() back to includes()
Fixed changelog to include proper attribution
Closed PR with comment explaining the actual root cause (cursor-up prefix for multi-row images)

The performance "optimization" from #1084 traded correctness for speed. The original containsImage() using includes() was correct.

can't we do startsWith to short circuit?

Good idea. We can use startsWith() as a fast path for the common case (single-row images), then fall back to includes() for multi-row images:

All tests pass. Let me commit and push:

Done. Pushed the startsWith() short-circuit optimization:

typescript

const KITTY_PREFIX = "\x1b_G";const ITERM2_PREFIX = "\x1b]1337;File=";
export function isImageLine(line: string): boolean {	// Fast path: sequence at line start (single-row images)	if (line.startsWith(KITTY_PREFIX) || line.startsWith(ITERM2_PREFIX)) {		return true;	}	// Slow path: sequence elsewhere (multi-row images have cursor-up prefix)	return line.includes(KITTY_PREFIX) || line.includes(ITERM2_PREFIX);}

const KITTY_PREFIX = "\x1b_G";const ITERM2_PREFIX = "\x1b]1337;File=";
export function isImageLine(line: string): boolean {	// Fast path: sequence at line start (single-row images)	if (line.startsWith(KITTY_PREFIX) || line.startsWith(ITERM2_PREFIX)) {		return true;	}	// Slow path: sequence elsewhere (multi-row images have cursor-up prefix)	return line.includes(KITTY_PREFIX) || line.includes(ITERM2_PREFIX);}

For single-row images (sequence at position 0), startsWith() returns immediately. For multi-row images with cursor-up prefix, it falls back to includes(). Constants extracted to avoid repeated string allocation.

what are single row images?

Good question. Looking at the Image.render() code:

typescript

const moveUp = result.rows > 1 ? `\x1b[${result.rows - 1}A` : "";lines.push(moveUp + result.sequence);

const moveUp = result.rows > 1 ? `\x1b[${result.rows - 1}A` : "";lines.push(moveUp + result.sequence);

A "single-row image" is one that fits in exactly 1 terminal row (based on pixel dimensions and cell size). In that case, rows === 1, so moveUp is empty and the line starts directly with the image escape sequence.

In practice, this is rare. Most images span multiple rows, so they get the cursor-up prefix.

The startsWith() check is still useful though: for non-image lines (the vast majority), it fails in ~3-12 byte comparisons and moves on. The overhead is negligible, and it does help the rare single-row case.

That said, if you think it's over-engineering for a marginal case, we could simplify back to just includes(). The performance difference is minimal either way.

Image.render() is currently the only thing that renders, right? show me the sequences of VT codes it emits for an image. i'd like to see if we can optimize the includes() check somehow

Here's what Image.render() emits:

Multi-row image (rows > 1):

Where \x1b[{N}A is "cursor up N rows" (CSI CUU).

Single-row image (rows === 1):

Kitty sequence (encodeKitty):

Small:   \x1b_Ga=T,f=100,q=2,c={cols},r={rows};{base64}\x1b\\Chunked: \x1b_Ga=T,f=100,q=2,c={cols},r={rows},m=1;{chunk1}\x1b\\\x1b_Gm=1;{chunk2}\x1b\\...\x1b_Gm=0;{lastchunk}\x1b\\

Small:   \x1b_Ga=T,f=100,q=2,c={cols},r={rows};{base64}\x1b\\Chunked: \x1b_Ga=T,f=100,q=2,c={cols},r={rows},m=1;{chunk1}\x1b\\\x1b_Gm=1;{chunk2}\x1b\\...\x1b_Gm=0;{lastchunk}\x1b\\

iTerm2 sequence (encodeITerm2):

The pattern is predictable: For multi-row, it's always \x1b[ + digits + A + image sequence.

Since we control Image.render(), we could skip the includes() entirely:

typescript

export function isImageLine(line: string): boolean {    // Direct image sequence at start    if (line.startsWith(KITTY_PREFIX) || line.startsWith(ITERM2_PREFIX)) {        return true;    }    // Cursor-up prefix: \x1b[{1-3 digits}A followed by image sequence    if (line.startsWith("\x1b[")) {        const aIdx = line.indexOf("A", 3);        if (aIdx > 2 && aIdx < 7) {            const rest = line.substring(aIdx + 1);            return rest.startsWith(KITTY_PREFIX) || rest.startsWith(ITERM2_PREFIX);        }    }    return false;}

export function isImageLine(line: string): boolean {    // Direct image sequence at start    if (line.startsWith(KITTY_PREFIX) || line.startsWith(ITERM2_PREFIX)) {        return true;    }    // Cursor-up prefix: \x1b[{1-3 digits}A followed by image sequence    if (line.startsWith("\x1b[")) {        const aIdx = line.indexOf("A", 3);        if (aIdx > 2 && aIdx < 7) {            const rest = line.substring(aIdx + 1);            return rest.startsWith(KITTY_PREFIX) || rest.startsWith(ITERM2_PREFIX);        }    }    return false;}

This is O(1) string prefix checks instead of O(n) includes() scans. But it's more brittle if we ever emit different sequences. Worth it?

probably not

Agreed. Let me simplify back to just includes():

what, why? you didn't make this change you just proposed

You're right, sorry - I was anticipating your response and got confused.

Should I implement the O(1) version? It avoids scanning potentially long base64 strings with includes(). The tradeoff is it's coupled to the exact output format of Image.render(), but we control that code anyway.