Bun.markdown.ansi: show image URL and cap Kitty image width#29131
Bun.markdown.ansi: show image URL and cap Kitty image width#29131robobun wants to merge 18 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughKitty Graphics emission limited to local PNGs; non-PNG images fall back to alt-text/URL or OSC-8 hyperlinks. Added theme Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 Prompt for all review comments with AI agents
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.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f6e7db2f-0a77-425a-b244-1a56d8b1d6e7
📥 Commits
Reviewing files that changed from the base of the PR and between d81b0ca and 32f18abecb3d20f78b2b18d3e24f769fa33a453e.
📒 Files selected for processing (4)
packages/bun-types/bun.d.tssrc/bun.js/api/MarkdownObject.zigsrc/md/ansi_renderer.zigtest/regression/issue/29118.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/regression/issue/29118.test.ts (2)
2-12:⚠️ Potential issue | 🟡 MinorTrim the top banner to the single issue URL comment.
Lines 2-12 are mostly issue/PR context prose; keep only Line 1 (
// https://github.com/oven-sh/bun/issues/29118) and retain inline comments only where they explain test-signal rationale.Based on learnings: in
test/regression/issue/, the standard is a single-line GitHub issue URL comment; avoid extra inline bug-context prose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/29118.test.ts` around lines 2 - 12, The top comment block in test/regression/issue/29118.test.ts contains extra prose; trim it to the single-line GitHub issue URL comment (“// https://github.com/oven-sh/bun/issues/29118”) and remove the surrounding explanatory lines from the banner, preserving only any inline comments that are directly used as test-signal rationale; locate the header comment at the top of the file and reduce it accordingly.
199-199:⚠️ Potential issue | 🟡 MinorStrengthen the empty-
srcguard assertion.Line 199 only rejects
" ()". Reject bare()so variants likealt()are also caught.Proposed fix
- expect(out).not.toContain(" ()"); + expect(out).not.toContain("()");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/29118.test.ts` at line 199, The test currently only rejects the string " ()" but should reject bare parentheses; update the assertion that checks the output variable out (the expect(out)... line in the test) to assert not.toContain("()") instead of not.toContain(" ()") so bare "()" occurrences (e.g., alt()) are caught as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/regression/issue/29118.test.ts`:
- Around line 2-12: The top comment block in test/regression/issue/29118.test.ts
contains extra prose; trim it to the single-line GitHub issue URL comment (“//
https://github.com/oven-sh/bun/issues/29118”) and remove the surrounding
explanatory lines from the banner, preserving only any inline comments that are
directly used as test-signal rationale; locate the header comment at the top of
the file and reduce it accordingly.
- Line 199: The test currently only rejects the string " ()" but should reject
bare parentheses; update the assertion that checks the output variable out (the
expect(out)... line in the test) to assert not.toContain("()") instead of
not.toContain(" ()") so bare "()" occurrences (e.g., alt()) are caught as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b38b72f-d69b-4dbf-8da1-d1b4d80eaf45
📥 Commits
Reviewing files that changed from the base of the PR and between 32f18abecb3d20f78b2b18d3e24f769fa33a453e and 002d1d7c5a160a2a13bca904f90d50478bab2f9e.
📒 Files selected for processing (1)
test/regression/issue/29118.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
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.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 6b58422a-ce77-4835-838a-9738c3c71e4e
📥 Commits
Reviewing files that changed from the base of the PR and between 002d1d7c5a160a2a13bca904f90d50478bab2f9e and 3f77240551f65b4bd1c1c24f3be9a8ab34ee7d0a.
📒 Files selected for processing (2)
src/md/ansi_renderer.zigtest/regression/issue/29118.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
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.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 2fe1a45a-826d-4b77-8184-ebbab6d109f4
📥 Commits
Reviewing files that changed from the base of the PR and between 3f77240551f65b4bd1c1c24f3be9a8ab34ee7d0a and 0b327a3657453d1121f13732235eddaaa34b1319.
📒 Files selected for processing (2)
src/md/ansi_renderer.zigtest/regression/issue/29118.test.ts
robobun
force-pushed
the
farm/92232157/md-image-url-fallback
branch
from
f5d038c to
073ad9b
Compare
There was a problem hiding this comment.
LGTM — all bugs found in prior review rounds have been addressed.
Extended reasoning...
Overview
This PR adds three image-rendering improvements to Bun.markdown.ansi: URL display in the fallback path, PNG signature verification before Kitty transmission, and a column-width cap via c=<cols> in the APC header. It also threads a cwd option through the JS API. Changes touch src/md/ansi_renderer.zig, src/bun.js/api/MarkdownObject.zig, src/cli/run_command.zig, type definitions, and a new regression test file.
Security risks
No security concerns. The code reads local files (PNG signature check) and writes terminal escape sequences. The explicit guards against dumping data: URI payloads (case-insensitive per RFC 3986) prevent accidental megabyte base64 output. No user-controlled input reaches shell execution or SQL.
Level of scrutiny
This is rendering/display code, not production-critical. The main risk surface is subtle state-machine bugs in the ANSI renderer (block stack, last_was_newline, in_cell, heading_level) and edge cases in URI scheme matching. Multiple rounds of review found and fixed exactly these kinds of issues.
Other factors
After several rounds of review, all bugs I flagged were fixed: the in_cell and heading_level guards on the URL-parens fallback, kittyColumnBudget using self.col instead of currentIndent(), all data:/http:/file: scheme checks made case-insensitive, the isPngFile PATH_MAX guard corrected to >=, the empty-payload guard in extractPngDataUrlBase64, and the MIME type/encoding checks made case-insensitive. The only remaining issue from the bug hunter is pre-existing and does not block this PR. 294 lines of new regression tests cover the key paths.
robobun
force-pushed
the
farm/92232157/md-image-url-fallback
branch
from
bf5117b to
2835149
Compare
There was a problem hiding this comment.
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.
Extended reasoning...
Overview
This PR touches the markdown→ANSI renderer (src/md/ansi_renderer.zig, ~200 lines net), the JS binding (src/runtime/api/MarkdownObject.zig), the CLI prefetch path (src/cli/run_command.zig), the public type surface (packages/bun-types/bun.d.ts), one snapshot, and a 334-line regression test. It introduces several new helpers (readPngDims, parsePngDims, parsePngDimsFromBase64, writeKittyApcHeader, kittyColumnBudget, endsWithCaseInsensitiveAscii) and a new cwd field on the public AnsiTheme interface.
Security risks
Low. The new file-read in readPngDims opens an already-resolved absolute path read-only, peeks 24 bytes, and closes — gated behind the existing resolveLocalImagePath stat check and the user opting into kittyGraphics: true. No shell execution, no network, no path-traversal surface beyond what already existed. The cwd option only feeds image_base_dir, which was already settable internally.
Level of scrutiny
Medium. This is not security- or correctness-critical infrastructure, but it:
- Adds a documented public API option (
cwd), which is a one-way door. - Encodes a heuristic constant (
ASSUMED_CELL_PX = 16) that determines when images get force-scaled vs. rendered natively — a UX judgment call a maintainer should ratify. - Went through ~10 rounds of review-and-fix on this branch (table/heading guards, case-insensitive scheme matching, PATH_MAX off-by-one, empty-payload data URIs, and the Kitty
c=exact-vs-max semantics), which signals the change was non-trivial to get right.
Other factors
All prior inline comments (mine and CodeRabbit's) are resolved, the latest bug-hunt pass on 785e70a is clean, and test coverage for the new behavior is thorough (20 targeted assertions including edge cases for tables, headings, inline-prefix images, empty payloads, and case-insensitive MIME). The remaining reason not to auto-approve is scope, not correctness.
There was a problem hiding this comment.
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.
Extended reasoning...
Overview
Touches src/md/ansi_renderer.zig (~200 new lines: PNG signature + IHDR parsing, partial base64 decode, conditional Kitty c= emission, URL-parens fallback with in_cell/heading/link_depth/data-URI guards), src/runtime/api/MarkdownObject.zig (new cwd theme option), src/cli/run_command.zig (case-insensitive http(s) scheme), bun.d.ts, a snapshot update, and a 334-line regression test.
Security risks
None identified. readPngDims opens user-supplied paths read-only, but only behind an explicit kittyGraphics: true opt-in and after the pre-existing resolveLocalImagePath stat() gate. No injection, auth, or data-exposure surface.
Level of scrutiny
Medium. This is a rendering/UX feature, not a hot path or security boundary, but it went through ~10 review iterations with real bugs caught each round (table-cell guard, heading guard, self.col vs indent, four separate case-insensitivity gaps, MAX_PATH_BYTES off-by-one, empty-payload APC, and the c= upscaling regression). The most recent fix (785e70a) added a meaningful chunk of new logic — parsePngDims, readPngDims, parsePngDimsFromBase64, and the ASSUMED_CELL_PX threshold — in response to my last comment, and that code hasn't had independent review yet.
Other factors
Two design decisions warrant human sign-off: (1) the new public cwd?: string field on AnsiTheme, and (2) ASSUMED_CELL_PX = 16 — the comment notes real cells are 6–12 px, so images in the budget*6 .. budget*16 px range will still render at native size and may overflow on narrow-cell fonts; that's a deliberate under-cap tradeoff someone should ack. CI is green except for an unrelated Windows http-close flake. All prior reviewer comments (mine and CodeRabbit's) are resolved.
robobun
force-pushed
the
farm/92232157/md-image-url-fallback
branch
from
c17e715 to
98d246d
Compare
There was a problem hiding this comment.
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.
Extended reasoning...
Overview
This PR touches six files: ~250 lines of logic in src/md/ansi_renderer.zig (PNG IHDR parsing, Kitty APC header emission with conditional c= scaling, URL-parens fallback for images, case-insensitive URI scheme handling), ~50 lines in src/runtime/api/MarkdownObject.zig (new cwd theme option with relative-path absolutization), a small case-insensitivity fix in src/runtime/cli/run_command.zig, the AnsiTheme type definition in bun.d.ts, one snapshot update, and a 360-line regression test suite.
Security risks
None identified. The new file I/O (readPngDims) opens files read-only with a length-guarded stack buffer and only reads 24 bytes; the path comes from markdown the caller already chose to render. The cwd option is absolutized before use and only feeds into stat()/open() for read — no write, exec, or traversal beyond what the existing resolveLocalImagePath already did with process cwd.
Level of scrutiny
Medium. This is not a simple/mechanical change: it adds a new public API field (AnsiTheme.cwd), introduces binary parsing of PNG headers and base64 prefixes, and encodes a heuristic (ASSUMED_CELL_PX = 16) that governs when Kitty images get downscaled vs. rendered native-size. The fallback-rendering guards (in_cell, heading_level, link_depth, is_data_url) interact with several layout subsystems.
Other factors
The PR has been through ~10 rounds of automated review; every flagged issue (in_cell/heading guards, self.col vs indent, case-insensitive scheme/MIME, empty-payload data URI, MAX_PATH_BYTES off-by-one, Kitty c= upscaling semantics, relative-cwd absolutization) was fixed and has a corresponding regression test. The current bug-hunting pass found nothing. CI has been retriggered for unrelated infra flakes only. Given the new public API surface and the scaling heuristic being a judgment call, a human should give the final approval rather than auto-merging.
|
Diff is green — the only red on the latest run (#54503) is two SIGKILLs on the same Ubuntu 25.04 x64 runner ( Ready for a maintainer to merge or kick CI once the infra settles. |
robobun
force-pushed
the
farm/92232157/md-image-url-fallback
branch
from
fc874f7 to
6f963c8
Compare
|
CI build #56618 has one Diff-relevant lanes are green. Ready for a maintainer to merge or kick the one Darwin 14 aarch64 lane. |
robobun
force-pushed
the
farm/92232157/md-image-url-fallback
branch
from
8e0d295 to
9cb2eb7
Compare
|
Rebased clean onto latest main (9cb2eb7). CI build #58013 red is unrelated flake only:
My regression suite ( |
There was a problem hiding this comment.
No bugs found and all prior review feedback is resolved, but since this adds a public API option (cwd on AnsiTheme) and bakes in the ASSUMED_CELL_PX = 16 Kitty-scaling heuristic across both the Zig and Rust renderers, I'd like a maintainer to sign off on the API surface and the scaling design rather than auto-approving.
Extended reasoning...
Overview
This PR fixes three image-rendering behaviors in Bun.markdown.ansi(): (1) shows the image URL in dim parens when OSC 8 hyperlinks are off, (2) verifies the PNG signature + IHDR before handing a local file to Kitty's t=f,f=100 path so JPEG/GIF/WebP fall through to the text fallback, and (3) emits Kitty's c=<cols> only when the PNG's IHDR width exceeds budget * ASSUMED_CELL_PX so small icons aren't upscaled. It also adds a new cwd?: string option to AnsiTheme for resolving relative image paths, with relative values absolutized against the process cwd. Changes span 9 files (~500 net lines): dual ansi_renderer.{zig,rs} implementations, dual MarkdownObject.{zig,rs} API plumbing, a small case-insensitive scheme fix in run_command.{zig,rs}, bun.d.ts types, a snapshot update, and a 20-case regression test.
Security risks
Low. The new file I/O (readPngDims) is read-only, opens with O_RDONLY, reads a fixed 24 bytes, and is bounded by the existing resolveLocalImagePath stat check; the >= MAX_PATH_BYTES guard prevents the off-by-one in bun.path.z. The base64 IHDR decode is bounded to 32 input chars / 24 output bytes. The cwd option is caller-supplied and only used to resolve image paths for an opt-in terminal rendering feature — no new injection or traversal surface beyond what resolveLocalImagePath already exposed via process cwd.
Level of scrutiny
Medium-high. While the blast radius is limited to markdown terminal rendering (not a hot path, not security-critical), the PR (a) extends a public Bun API with a new option that needs to be supported going forward, (b) introduces a hard-coded heuristic constant (ASSUMED_CELL_PX = 16) whose value materially affects when images get scaled vs. rendered native-size, and (c) duplicates non-trivial logic across Zig and Rust ports that must stay in sync. These are exactly the kinds of design choices a maintainer should ratify.
Other factors
The PR has been through ~12 rounds of bot review (CodeRabbit + prior claude[bot] runs) and every flagged issue — table-cell/heading guards, self.col vs indent budget, case-insensitive scheme/MIME matching, empty-payload data URIs, MAX_PATH_BYTES off-by-one, relative-cwd absolutization, the c= upscaling regression, and an out-of-scope bake-codegen ride-along — has been addressed with follow-up commits and regression tests. The current bug-hunting pass found nothing new. CI is green on diff-relevant lanes per the author's last status comment. Test coverage for the new behavior is thorough (20 targeted assertions in test/regression/issue/29118.test.ts). Given the clean state, this is ready for a human merge decision; I'm deferring rather than approving solely because of the new API surface and the embedded design heuristic.
|
Update on build #58013: a third failure surfaced as more lanes finished — Full red set on this build, all on baseline/asan lanes and none touching markdown/ANSI/image/Kitty:
My diff touches no transpiler/install/TLS/http code. Regression suite + markdown-entrypoint tests pass locally and on every lane they ran. Needs a maintainer to merge or re-kick the baseline lanes. |
Three fallback fixes for image rendering: 1. Show the image URL in dim parens after the alt text when OSC 8 hyperlinks aren't being emitted. Previously users saw just the camera icon with no way to reach the source, matching the link-fallback style from leaveSpan(.a). Skipped for data: URIs and when inside an enclosing link span. 2. Verify the PNG signature before transmitting a file via Kitty's t=f / f=100 path. JPEG, GIF, and WebP files on disk used to get sent anyway and the terminal would show a broken-image indicator. They now fall through to the URL-label fallback. 3. Include c=<cols> in the Kitty APC header so large-dimension images scale to the current column budget instead of overflowing the screen. The budget is theme.columns minus the active block indent; c= is omitted when columns is 0 (wrapping disabled) so the terminal renders at native size. Also adds a `cwd` option to Bun.markdown.ansi() so tests can point image-base-dir at a temp directory. Fixes #29118
- kittyColumnBudget: cap at 1 cell when indent consumes all columns so deeply-indented layouts still get c=<cols> scaling instead of falling back to native-size rendering - trim test file banner to the single issue URL - tighten empty-src assertion to reject bare () as well as ' ()'
Two bugs caught in review:
1. kittyColumnBudget() was subtracting only currentIndent() from
theme.columns. For an inline image preceded by text on the same
line (e.g. "prefix "), self.col has already advanced
past the indent — so the APC was advertising a budget larger than
the remaining line width and the image overflowed to the right of
the terminal. Use max(self.col, currentIndent()) instead.
2. The URL-parens fallback ("📷 alt (url)") was missing the in_cell /
heading_level guards that the Kitty path already had. Inside a
table cell, the URL text went into table_cell_buf and flushTable
counted it against the column width, blowing the table layout past
theme.columns. Same hazard in a heading — heading_buf widths feed
the underline row. Gate the fallback on !in_cell && heading_level
== 0 to match the Kitty rendering gate.
Regression tests:
- inline image after text emits c=30 (not c=40) in a 40-column render
- image in a table cell renders inside the column budget and the URL
string stays out of the output
- image in a heading: same
`` used to slip the lowercase-only startsWith check in is_data_url and fall into the URL-fallback / OSC 8 path, dumping the full base64 payload into the output. Match case-insensitively per RFC 3986 §3.1. Regression test added.
Two review nits from claude[bot]: 1. `extractPngDataUrlBase64` and `resolveLocalImagePath`'s early-return guard still used case-sensitive `startsWith` on `data:`/`http(s):`. `is_data_url` in emitImage was already case-insensitive from f5d038c, but a `DATA:image/png;…` URI would silently miss the direct Kitty transmit fast path and burn a decode+join+stat cycle before returning null. Switched all three prefix checks to `startsWithCaseInsensitiveAscii` for consistency with RFC 3986 §3.1. 2. `isPngFile`'s length guard used `> bun.MAX_PATH_BYTES`, so a path exactly MAX_PATH_BYTES long would fall through and cause `bun.path.z` to write its NUL terminator one byte past the end of the [4096]u8 stack buffer. Real paths never hit this on Linux because PATH_MAX includes the NUL, but guard defensively with `>= bun.MAX_PATH_BYTES`.
MIME type and its parameters are ASCII case-insensitive per RFC 2045 §5.1 / RFC 2046 §5.1, so `data:image/PNG;BASE64,…` is a valid spelling of the lowercase form. The scheme check was already case-insensitive (f5d038c), but the `;base64` and `image/png` checks inside extractPngDataUrlBase64 were still byte-exact, so an uppercase MIME slipped into the alt-text fallback instead of going through the Kitty direct-transmit path. Regression test added.
Two follow-ups: 1. The pre-existing `renders images as alt text with link` snapshot in test/cli/run/__snapshots__/markdown-entrypoint still had the old camera-only output. With the URL-parens fallback now active for hyperlink-off renders (default in non-TTY), the expected output includes the dim `(url)` suffix. Updating the snapshot to match. 2. The `file://` prefix strip and `localhost` sub-check in `resolveLocalImagePath` were still case-sensitive even though the adjacent `http(s):`/`data:` reject list was updated in 073ad9b. Switched to `startsWithCaseInsensitiveAscii`/`eqlCaseInsensitiveASCII` for consistency with RFC 3986 §3.1.
Three review nits: 1. `data:image/png;base64,` with nothing after the comma passed every guard in `extractPngDataUrlBase64` and returned a non-null zero- length slice. Zig's `?[]const u8` treats empty slices as non-null, so `emitKittyImageDirect` was emitting a malformed empty APC and returning early — no camera, no alt, no URL fallback. Route empty payloads to the fallback with an explicit `payload.len == 0` guard. 2. The `remote_image_paths` map lookup in `emitImage` still gated on case-sensitive `startsWith(src, "http://")` even though the rest of the PR switched to case-insensitive matching. Swapped to `startsWithCaseInsensitiveAscii` to match. 3. The corresponding pre-scan filter in `prefetchRemoteImages` (`src/cli/run_command.zig`) used `hasPrefixComptime`, so uppercase schemes were never even downloaded — the renderer lookup and the downloader must agree on the same case-insensitive filter or the map entry can never be found. Mirrored the same fix there.
Per the Kitty Graphics Protocol, `c=<cols>` is the EXACT number of cells to display over — Kitty enlarges images smaller than `c` cells just as readily as it shrinks larger ones. The previous unconditional `c=<column_budget>` was upscaling small icons (favicons, badges, the 8x8 test PNG) to fill the entire column budget, a regression vs. pre-PR native-size rendering. Fix: parse the PNG IHDR to get pixel width, and only emit `c=` when the image would definitely overflow at native size. Use a conservative ASSUMED_CELL_PX=16 as the cell-width upper bound so we only cap when width_px > budget * 16 (well above any realistic monospace font cell width). Small images render at native size, large images get scaled down. - Replace `isPngFile()` with `readPngDims()` which reads sig + IHDR and returns width/height, null for non-PNG. - Add `parsePngDimsFromBase64()` for the data:image/png path — decodes just the first 32 base64 chars (24 raw bytes) to extract IHDR without scanning the whole payload. - Thread `width_px` through `emitKittyImageFile` / `emitKittyImageDirect` / `writeKittyApcHeader`. - Update tests: small PNG (8x8) tests now assert NO `c=` is emitted; new wide-PNG fixture (fabricated 2000x100 header) exercises the cap path.
…ub 502, unrelated)
…; test runner cleanup race, unrelated)
`bun.path.joinAbsString` assumes its `cwd` argument is already
absolute (and on Windows `_joinAbsStringBufWindows` asserts
`isAbsoluteWindows(cwd)` in debug builds). The JS API was
storing `theme.cwd` verbatim into `theme.image_base_dir`, so
a relative value like `{ cwd: "./assets" }` resolved to
`/assets/img.png` on POSIX (stat fails, silent alt-text
fallback) and panicked on Windows debug.
Resolve relative values against the process cwd before storing,
mirroring the pattern already used in `src/cli/run_command.zig`'s
`renderMarkdownFileAndExit`. Regression test added.
…est.ts buntmp, 1-of-8 shard flake, unrelated)
Port the full set of Bun.markdown.ansi image rendering fixes to src/md/ansi_renderer.rs, src/runtime/api/MarkdownObject.rs, and src/runtime/cli/run_command.rs. The Rust rewrite in 23427db (May 14) replaced the Zig renderer, so the original Zig edits on this branch no longer shipped in built binaries. Ports: - URL-parens fallback when hyperlinks are off (guards on link_depth, in_cell, heading_level; skip data: URIs) - Case-insensitive data:image/png;base64 scheme/MIME matching, empty payload guard - Case-insensitive file:// and http(s):// scheme matching for resolve_local_image_path and prefetch_remote_images - PNG signature verification before Kitty transmission; non-PNG falls through to the label - Kitty c=<cols> only emitted when the PNG's IHDR width would overflow the remaining column budget (c= is EXACT width per Kitty spec, not a max cap) - Bun.markdown.ansi(text, {cwd}) absolutizes relative cwd against the process cwd before passing to image path resolution Also fixes a pre-existing bake-codegen.ts bug: OVERLAY_CSS define needed JSON.stringify (raw CSS was passed through, failing the bundler's JSON-value check in release builds).
The JSON.stringify wrapper was added earlier on this branch when the
JSON lexer still raised on a leading `*` (raw minified CSS starts
with `*{...}`). #30679 on main added an auto-quote fallback so the
bake `define` value no longer needs to be pre-quoted, making this
hunk redundant and out of scope for the markdown PR.
The test/bundler/bun-build-api.test.ts auto-quote test still uses
hardcoded fixture strings, not bake-codegen's output, so coverage is
unaffected.
robobun
force-pushed
the
farm/92232157/md-image-url-fallback
branch
from
9cb2eb7 to
bcf9f63
Compare
There was a problem hiding this comment.
No issues found on this pass. Deferring because this adds a new public API option (cwd on AnsiTheme) and bakes in a rendering heuristic (ASSUMED_CELL_PX = 16) across parallel Zig/Rust implementations — those design choices are worth a maintainer's sign-off.
Extended reasoning...
Overview
This PR touches 9 files to fix three image-rendering behaviors in Bun.markdown.ansi(): (1) showing the image URL in dim parens when OSC 8 hyperlinks are off, (2) verifying PNG signature + IHDR before sending a file to Kitty (non-PNGs fall through to the text fallback), and (3) emitting Kitty's c=<cols> only when the PNG's IHDR width would overflow the remaining line budget. It also adds a new cwd?: string option to Bun.markdown.AnsiTheme for resolving relative image paths, with absolutization against the process cwd. Changes are mirrored across the Zig (src/md/ansi_renderer.zig, src/runtime/api/MarkdownObject.zig, src/runtime/cli/run_command.zig) and Rust (src/md/ansi_renderer.rs, src/runtime/api/MarkdownObject.rs, src/runtime/cli/run_command.rs) implementations, plus a 360-line regression test and a snapshot update.
Security risks
None significant. The new file I/O in readPngDims opens a user-resolved path read-only and reads at most 24 bytes — no different in privilege from the pre-existing stat() call in resolveLocalImagePath. No shell, network, auth, or escape-sequence injection surface is introduced; APC/OSC payloads are base64 or pass-through of the user's own markdown src, same as before.
Level of scrutiny
Medium. The renderer is user-facing terminal output (cosmetic blast radius), not a correctness-critical path like the bundler or runtime. However, this PR is not mechanical: it introduces a new public API field on a documented type, hand-rolls PNG IHDR parsing in two languages, and encodes a heuristic constant (ASSUMED_CELL_PX = 16) whose value directly determines when images get scaled. Those are design decisions a maintainer should ratify rather than have land via bot approval.
Other factors
The PR has been through ~12 rounds of bot review and every prior inline finding (table-cell/heading guards, self.col vs indent budget, case-insensitive scheme/MIME matching, empty-payload data URIs, relative-cwd absolutization, the c= upscaling regression) is addressed and resolved in the timeline. Test coverage is thorough (20 targeted tests in test/regression/issue/29118.test.ts plus a snapshot). CI is reported green on diff-relevant lanes with only unrelated infra flakes. The remaining reason to defer is purely scope/design sign-off, not code quality.
1 participant
Summary
Closes #29118. Three fallback fixes for how
Bun.markdown.ansi()handles images.1. Show the image URL in the fallback path
Before this change,
rendered as just📷 altwith no way to reach the source when OSC 8 hyperlinks were off. Now the URL is shown in dim parens after the alt text, matching the link-fallback style already used byleaveSpan(.a):Skipped for
data:URIs (megabyte base64 payloads would dominate the output), when the image is inside an enclosing link span (the outer link already shows its own URL), and inside table cells / headings (their width machinery would count the URL against cell / underline size).2. Verify the PNG signature before handing a file to Kitty
emitImagewas happily sending any regular file on disk through Kitty'st=f,f=100(PNG) transmission. For a.jpg/.gif/.webpthe terminal would decode the bytes as PNG, fail, and render the broken-image indicator with no caption. Fix: read the PNG signature + IHDR before callingemitKittyImageFile. Non-PNGs fall through to the URL-label fallback from (1).3. Scale Kitty image width only when it would overflow
The APC sequence now includes
c=<cols>when (and only when) the image's pixel width exceeds what the remaining line can hold at native size. Per the Kitty Graphics Protocol spec,c=<cols>is the exact number of cells to display over — Kitty enlarges images smaller thanccells just as readily as it shrinks larger ones. So emittingc=unconditionally would upscale small icons (favicons, badges) to fill the column budget, a regression vs. pre-PR native-size rendering. The fix is to parse the PNG IHDR and comparewidth_pxagainstbudget * ASSUMED_CELL_PX(16, a conservative upper bound on cell width) — only emitc=<budget>when the image definitely would overflow.c=is also omitted whencolumns: 0so wrapping-disabled callers always get native-size rendering.For the
t=ddata-URL path the first 24 bytes of raw PNG are decoded out of the base64 payload (~32 base64 chars) to get the IHDR without scanning the whole payload.API addition
Bun.markdown.ansi(text, theme)now accepts acwdoption to set the base directory used to resolve relative imagesrcpaths. Defaults to the process cwd (unchanged behaviour). Added toAnsiThemeinbun.d.ts.Verification
bun run zig:check— clean.test/regression/issue/29118.test.tscovers: URL in parens, no OSC 8 withouthyperlinks:true,c=only for wide images, PNG signature check rejecting JPEG, nested link handling, data URI skip (case-insensitive scheme + MIME), colors-off fallback,data:…;base64,empty payload, inline-image remaining-line budget, table cell / heading guards.