Bun.markdown.ansi: show image URL and cap Kitty image width - #29131

Actionable comments posted: 3

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/md/ansi_renderer.zig`:
- Around line 1905-1910: The kittyColumnBudget() helper currently returns 0 when
indent >= self.theme.columns which causes writeKittyApcHeader to omit the "c="
width cap; change kittyColumnBudget so that when self.theme.columns > 0 it
returns at least 1 (e.g., return 1 instead of 0 when indent >= columns) so the
"c=" parameter remains present and enforces a minimal cap even in
narrow/deeply-indented layouts; update the function kittyColumnBudget (and any
callers like writeKittyApcHeader if necessary) to rely on a minimum budget of 1
rather than 0.

In `@test/regression/issue/29118.test.ts`:
- Around line 2-12: The leading multi-line issue-summary banner in
test/regression/issue/29118.test.ts (the comment that starts with
"Bun.markdown.ansi() image-rendering improvements:") should be trimmed to a
single-line GitHub issue URL comment on the first line; remove the remaining
explanatory lines in that banner and keep only concise comments that document
test design rationale or specific fixtures/assertions as needed.
- Around line 194-199: The test "image without a src still works (doesn't crash,
doesn't print URL)" currently only rejects the string " ()" which misses other
empty-paren forms; update the assertion to check for bare parentheses by
replacing the negative assertion expect(out).not.toContain(" ()") with
expect(out).not.toContain("()") so the output variable out produced by
Bun.markdown.ansi("![alt]()\n", { colors: true, hyperlinks: false }) is verified
to never contain an empty-URL suffix; keep the existing assertions for "alt"
intact.

Actionable comments posted: 1

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/md/ansi_renderer.zig`:
- Around line 1909-1913: The kittyColumnBudget function currently bases the APC
width only on self.theme.columns minus currentIndent(), which ignores visible
text already on the current line and lets images overflow; update
kittyColumnBudget in AnsiRenderer to compute the remaining line width as
theme.columns minus the current column position (not just the indent) — e.g.,
subtract the current cursor/column (use an existing method like currentColumn()
or compute visibleTextWidthOnLine()) in addition to currentIndent(), then clamp
with `@max`(1, remaining) so the returned u32 is at least 1; keep the function
name kittyColumnBudget and the theme.columns usage but replace the single
subtract with a subtraction of the full consumed width.

Actionable comments posted: 1

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/md/ansi_renderer.zig`:
- Around line 1855-1857: The guard for data: URLs only checks lowercase "data:"
so uppercase/mixed-case URIs still pass; update the is_data_url logic (where
is_data_url is defined using bun.strings.startsWith(src.?, "data:")) to perform
a case-insensitive check by normalizing src (or comparing a lowercased prefix)
before startsWith, and apply the same change to the other instances referenced
around the other block (the section noted around lines 1882-1899) so any
variation like "DATA:" or "Data:" is treated as a data URL and suppressed
consistently.

LGTM — all bugs found in prior review rounds have been addressed.

No further issues found after 785e70a — the IHDR-gated c= emission resolves the upscaling regression. Deferring to a human for final sign-off since this adds a public API option (cwd on AnsiTheme) and a hardcoded heuristic (ASSUMED_CELL_PX = 16) that are worth a maintainer's eye.

Thanks for the 785e70a fix — the IHDR-gated c= emission looks right. No further issues from me, but deferring to a human for the new cwd API option and the ASSUMED_CELL_PX = 16 heuristic, since both are judgment calls worth a second pair of eyes.

No new issues found and all prior review feedback has been addressed — deferring to a human for final sign-off on the new cwd API option and the ASSUMED_CELL_PX scaling heuristic.

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/md/ansi_renderer.rs:2930-2932 — The explicit let _ = file.close(); and its comment are redundant/misleading: bun_sys::File already closes on Drop (src/sys/file.rs:17-24, documented in src/CLAUDE.md), so the fd is closed on every path out regardless. File::close(self)'s only documented benefit over Drop is that "the syscall result is observable" — but let _ = discards that. Just delete both lines and let RAII do it (matching src/CLAUDE.md's own idiom // file closes on Drop.); at minimum drop the comment, since "we must be sure it runs on every path out" wrongly implies Drop doesn't cover it.

  Extended reasoning...

  What the issue is

  read_png_dims (src/md/ansi_renderer.rs:2930-2932) contains:

  let read_result = file.read_all(&mut buf);
  // Always close — File::close consumes self, so we must be sure it runs
  // on every path out.
  let _ = file.close();
  match read_result {
      bun_sys::Result::Ok(amt) if amt >= 24 => parse_png_dims(&buf),
      _ => None,
  }

  The comment claims the explicit .close() is needed to ensure the fd is released "on every path out." That is factually wrong for bun_sys::File, and the explicit call gains nothing because its return is discarded.

  Why the comment is wrong

  bun_sys::File owns its fd and closes it in Drop:

  // src/sys/file.rs:17-24
  impl Drop for File {
      #[inline]
      fn drop(&mut self) {
          if self.handle != Fd::INVALID && !self.handle.is_stdio() {
              let _ = close(self.handle);
          }
      }
  }

  This is documented at src/CLAUDE.md — bun_sys::File (owns the fd; closes on Drop) — and the crate's own canonical usage example there ends with the comment // file closes on Drop. with no explicit close call. The project convention (CLAUDE.md rule 8) is explicit: "Prefer RAII (Drop) over manual cleanup."

  Why the explicit .close() gains nothing here

  File::close(self) (src/sys/file.rs:313-321) is documented as "Close now. Equivalent to dropping self but the syscall result is observable." Its sole distinguishing feature is that it returns the close(2) result — but this call site immediately discards it with let _ =. So the ONLY reason the API exists to prefer .close() over Drop is thrown away. The match on line 2933 is the last statement in the function, so file would drop at exactly the same point in execution anyway; there is no timing benefit either. And there is no double-close risk (.close() consumes self via into_raw(), disarming Drop), so the call is harmless — it is just dead weight.

  Why the comment is the actionable problem

  REVIEW.md: "Only comment what the code cannot say. One line. Never restate what the code does." This comment does worse than restate — it misstates. A reader coming to this file learns "bun_sys::File doesn't close on Drop, you must manually ensure .close() on every path" — the opposite of the truth. That is exactly the kind of misdirection REVIEW.md's "verify semantics empirically, never from names or intuition" rule tries to prevent readers from having to do. Existing callers elsewhere in the tree (e.g. src/install/migration.rs:44 carries // file closes on Drop) rely on the RAII behavior, so this comment also disagrees with established convention.

  Step-by-step proof the fd closes without the explicit call

  Delete lines 2930-2932. Trace both paths out of the match:

  1. bun_sys::Result::Ok(amt) if amt >= 24 → parse_png_dims(&buf) returns; file (a local owned by this stack frame) goes out of scope; Drop::drop runs; self.handle != Fd::INVALID (open just succeeded) and !is_stdio() (a regular file we opened) → close(self.handle) fires.
  2. _ => None → identical: file goes out of scope at the closing brace; same Drop path.
  3. Early returns before file exists (lines 2917, 2923) never opened an fd.

  There is no path on which the fd survives function return.

  Impact

  None functionally — no fd leak, no double-close, no UB. This is purely comment-accuracy + a redundant call on new code introduced by this PR, hence nit.

  Fix

  Delete both lines (comment + let _ = file.close();) and let file drop at end-of-scope — matching src/CLAUDE.md's own example. If keeping the explicit close is preferred for symmetry with some other site, at minimum delete the two-line comment: it is what actively misleads.

• 🟡 src/md/ansi_renderer.rs:2115-2142 — format_c_hint hand-rolls a 25-line BufWriter struct + core::fmt::Write impl to format one u32 into a stack buffer, but bun_core::fmt::itoa(&mut ItoaBuf, n) -> &[u8] already does exactly this and is what the sibling renderer uses (src/md/html_renderer.rs:597, src/md/helpers.rs:476). The whole helper collapses to self.write_raw_no_color(b",c="); self.write_raw_no_color(bun_core::fmt::itoa(&mut bun_core::fmt::ItoaBuf::new(), budget)); inlined at the single call site in write_kitty_apc_header, which also drops the let _ = write!(...) discarded-result.

  Extended reasoning...

  What the issue is

  format_c_hint (src/md/ansi_renderer.rs:2118-2143) defines an inline BufWriter<'a> struct with a core::fmt::Write impl, constructs it over a caller-provided [u8; 16], calls write!(w, ",c={}", budget) with the result discarded via let _ =, and returns the written slice. This is 25 lines of bespoke machinery whose only job is to format a single u32 into a small stack buffer.

  The codebase already has a helper for exactly this: bun_core::fmt::itoa(&mut ItoaBuf, n) -> &[u8] (src/bun_core/fmt.rs:3203, with ItoaBuf re-exported at :3198). It's the tree-wide convention for base-10 integer formatting into a stack buffer, used at 15+ call sites — including two in the same crate: src/md/html_renderer.rs:597 (self.write(bun_core::fmt::itoa(&mut buf, value))) and src/md/helpers.rs:476. So the sibling HTML renderer already does this the canonical way; format_c_hint is the odd one out.

  Why REVIEW.md flags this

  REVIEW.md's Code style & idioms section is explicit: "In runtime native code, grep for the in-tree helper before hand-writing anything. File I/O, paths, strings, hashing, formatting … Being the only file touching a raw primitive is a red flag." Grep confirms this is the only place in the tree hand-rolling a core::fmt::Write impl to format an integer into a stack buffer. src/CLAUDE.md also lists bun_core::fmt alongside bun_core::strings as the canonical toolkit. Additionally, bun_alloc::SliceCursor already provides a core::fmt::Write impl over &mut [u8], so even the write!-based approach wouldn't need a hand-rolled struct.

  Why this is a nit, not normal

  The code is functionally correct: u32::MAX is 10 digits, plus ",c=" = 13 bytes < 16, so the buffer never overflows and the discarded write! result is always Ok. There is no user-visible behavior difference. This is purely a convention/code-quality finding on new code introduced by this PR — merging as-is causes no failure, so per the severity rubric it does not justify blocking merge.

  How to fix

  Delete format_c_hint entirely and inline at the single call site in write_kitty_apc_header (line 2107-2111):

  if budget > 0 && width_px > budget.saturating_mul(ASSUMED_CELL_PX) {
      self.write_raw_no_color(b",c=");
      let mut buf = bun_core::fmt::ItoaBuf::new();
      self.write_raw_no_color(bun_core::fmt::itoa(&mut buf, budget));
  }

  This drops ~25 lines, removes the discarded-result pattern, and matches the sibling renderer at html_renderer.rs:595-598 exactly.

  Step-by-step proof

  Input: [](./wide.png) with { kittyGraphics: true, columns: 40 } and a 2000px-wide PNG.

  1. write_kitty_apc_header(b"t=f", 2000) is called; budget = 40, 2000 > 40*16 = 640, so the c= branch runs.
  2. Current code: allocates [0u8; 16] on the stack, constructs BufWriter { buf, pos: 0 }, dispatches through the core::fmt::Write vtable via write!(w, ",c={}", 40) → w.write_str(",c=") copies 3 bytes, then core::fmt's integer formatter calls w.write_str("40") copies 2 bytes; pos = 5; returns &buf[..5] = b",c=40"; caller writes it via write_raw_no_color.
  3. Proposed code: write_raw_no_color(b",c=") writes 3 bytes; ItoaBuf::new() is a const uninit 40-byte array; itoa(&mut buf, 40u32) returns b"40"; write_raw_no_color writes 2 bytes.
  4. Output is byte-identical: \x1b_Ga=T,t=f,f=100,q=2,c=40;. The test at test/regression/issue/29118.test.ts:159 (expect(out).toContain("\x1b_Ga=T,t=f,f=100,q=2,c=40;")) and the c=30 inline-prefix test at :291 continue to pass unchanged.

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/md/ansi_renderer.rs:2073-2097 — format_c_hint hand-rolls a 26-line core::fmt::Write impl over a stack buffer to format ,c=<u32> — but this file already uses bun_core::fmt::buf_print (line 380) for exactly this, and sibling md files use bun_core::fmt::itoa. The whole helper collapses to bun_core::fmt::buf_print(buf, format_args!(",c={budget}")) or two write_raw_no_color calls (b",c=" + itoa) inline in write_kitty_apc_header.

  Extended reasoning...

  What the issue is

  The new format_c_hint helper (src/md/ansi_renderer.rs:2073-2097) defines a bespoke local BufWriter struct with a hand-written core::fmt::Write impl, then calls write!(w, ",c={}", budget) — 26 lines total — to format one u32 into a 16-byte stack buffer:

  fn format_c_hint(buf: &mut [u8; 16], budget: u32) -> &[u8] {
      use core::fmt::Write as _;
      struct BufWriter<'a> { buf: &'a mut [u8], pos: usize }
      impl<'a> core::fmt::Write for BufWriter<'a> {
          fn write_str(&mut self, s: &str) -> core::fmt::Result { ... }
      }
      let mut w = BufWriter { buf: &mut buf[..], pos: 0 };
      let _ = write!(w, ",c={}", budget);
      &buf[..w.pos]
  }

  Why this diverges from convention

  REVIEW.md is explicit: "grep for the in-tree helper before hand-writing anything … bun_core strings/fmt/Output … Being the only file touching a raw primitive is a red flag" and "Match the exact file's local conventions." Both apply here:

  1. This same file already uses the in-tree helper. ansi_renderer.rs:380 does bun_core::fmt::buf_print(&mut num_buf, format_args!("{num}. ")) for the ordered-list marker — a SliceCursor-based core::fmt::Write over a caller stack buffer, i.e. precisely what BufWriter reinvents.
  2. Sibling files in the same crate use itoa for u32→ASCII. src/md/html_renderer.rs:596-597 has let mut buf = bun_core::fmt::ItoaBuf::new(); self.write(bun_core::fmt::itoa(&mut buf, value)); and src/md/helpers.rs:475-476 does the same. bun_core::fmt::itoa (fmt.rs:3203) is the documented core::fmt-free integer-to-ASCII path.

  So format_c_hint is new code that hand-rolls a helper the file itself already imports and uses ~1700 lines above, and that the crate uses in two sibling files.

  How to fix

  Either delete format_c_hint entirely and inline into write_kitty_apc_header:

  if budget > 0 && width_px > budget.saturating_mul(ASSUMED_CELL_PX) {
      self.write_raw_no_color(b",c=");
      let mut buf = bun_core::fmt::ItoaBuf::new();
      self.write_raw_no_color(bun_core::fmt::itoa(&mut buf, budget));
  }

  …or, if you want to keep the single-write shape, replace the body with the same helper line 380 uses:

  let s = bun_core::fmt::buf_print(&mut buf[..], format_args!(",c={budget}")).unwrap_or(b"");
  self.write_raw_no_color(s);

  Both drop the bespoke BufWriter struct + core::fmt::Write impl and align with the file-family convention.

  Impact / why this is a nit

  There is no functional bug. The hand-rolled version is correct: 16 bytes comfortably fits ",c=" (3) + u32::MAX (10 digits) = 13 bytes max, so the Err(core::fmt::Error) branch is unreachable and the discarded write! result is fine. Output is byte-identical to what buf_print/itoa would produce. This is purely a code-quality / convention finding on new code introduced by the PR — nice to fix but not merge-blocking.

  Step-by-step proof

  Take budget = 40:

  1. Current: BufWriter::write_str receives ",c=" then "40", copies to buf[0..3] and buf[3..5], returns &buf[..5] = b",c=40".
  2. bun_core::fmt::buf_print(&mut buf, format_args!(",c={}", 40)): internal SliceCursor receives the same two fragments, writes to buf[0..5], returns Ok(&buf[..5]) = b",c=40".
  3. itoa variant: writes b",c=" then itoa(&mut ItoaBuf::new(), 40) = b"40" — same bytes on the wire.

  All three produce identical APC output; the only difference is 26 lines of bespoke type vs. one line calling the documented in-tree helper the file already uses.

I reviewed this revision and the automated bug-hunt found no issues — all prior nits (path-buffer pool, bare Ok/Err arms, strings:: alias, test-file placement, stderr assertions, and the pooled-scratch-buffer comment wording) are addressed. Because the PR adds new public API surface (cwd on AnsiTheme) and ~250 lines of renderer/file-I/O logic (PNG IHDR parsing, the ASSUMED_CELL_PX=16 scaling heuristic, path-buffer join guards), a maintainer sign-off on those design choices would still be worthwhile.

What was reviewed:

• URL-parens fallback: checked the in_cell / heading_level / link / is_data_url guards match the sibling link-fallback and Kitty-gate conditions; snapshot update is consistent.
• read_png_dims / parse_png_dims_from_base64: bounds are checked before slicing; bun_sys::File closes on Drop; over-long paths return None before resolve_path::z.
• cwd handling in MarkdownObject.rs: to_slice result is copied into an owned Vec/pooled buffer before render, so no borrow outlives the JS string; over-long input degrades to the process-cwd fallback via _checked join.
• Case-insensitive scheme/MIME checks applied consistently across emitImage, resolve_local_image_path, extract_png_data_url_base64, and the CLI prefetch filter.