Repository navigation
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add Linux-specific render_html_to_png using headless_chrome crate - Implement visual stability probing (fonts, images, mermaid readiness) - Support MDV_CHROME_PATH env var for custom Chrome/Chromium path - Ensure workspace cleanup on both success and error paths Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rnings on Linux Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… case Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Relative base_dir (e.g. ".") caused create_workspace to produce a relative workspace path, resulting in an invalid file:// URL that Chrome could not navigate to. fs::canonicalize resolves this to an absolute path before any downstream use. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdded Linux-specific HTML→PNG rendering using headless Chrome, gated Linux compilation for snapshot path helpers, added Linux-only Cargo dependencies, and extended terminal detection to recognize Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Workspace as Workspace (create/cleanup)
participant FS as FileSystem
participant ChromeFinder as ChromeFinder (MDV_CHROME_PATH / PATH)
participant Chrome as HeadlessChrome
Caller->>Workspace: create_workspace(run_id)
Workspace->>FS: write("document.html", html)
Caller->>ChromeFinder: find_chrome_binary()
ChromeFinder->>Chrome: launch(binary, args with file access)
Chrome->>Chrome: open_tab("file://.../document.html")
Chrome->>Chrome: evaluate VISUAL_STABILITY_JS (probe loop)
Chrome-->>ChromeFinder: report stable_height or timeout
Chrome->>Chrome: set_viewport_height(stable_height)
Chrome->>FS: capture_full_page_png() -> png_bytes
Caller->>Workspace: cleanup_workspace(run_id)
Caller-->>Caller: return SnapshotResult(png_bytes)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 4
🤖 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/io/webkit_snapshot/mod.rs`:
- Around line 179-184: The code currently builds file_url by concatenating
"file://" and a UTF-8 stringified html_path, which breaks on characters like
`#/`?/% and fails on non-UTF-8 paths; replace that logic in the block producing
file_url by constructing a proper file:// URL from the Path (html_path) using a
file-path-aware URL constructor (e.g., Url::from_file_path or an equivalent
path_to_file_url helper) and propagate errors with the same context message;
ensure you call .to_string() on the resulting Url for use where file_url is
needed and add the necessary use/import for url::Url if not already present.
- Around line 121-139: The logic currently marks remote images as non-blocking
(isRemoteAsset -> blocking: !remote) causing imagesReady to ignore
network-loaded images; change the probe to treat remote images as blocking so
the readiness check waits for their load state (modify the mapping that builds
images to set blocking true for network sources or remove the remote-based
branch) and/or update the imagesReady predicate in the imagesReady computation
to evaluate remote images the same as local ones (use
image.complete/currentSrc/naturalWidth checks regardless of isRemoteAsset),
referencing the isRemoteAsset function, the images mapping, and the imagesReady
closure.
- Around line 193-197: The command-line args for the WebKit snapshot renderer
include the overly permissive "--disable-web-security" flag; remove that entry
from the args vector so only "--allow-file-access-from-files" (and the other
flags like "--force-device-scale-factor=1") are passed. Locate the args(vec![
... ]) call in src/io/webkit_snapshot/mod.rs and delete the
std::ffi::OsStr::new("--disable-web-security") element (ensuring the surrounding
commas and vector formatting remain valid).
In `@src/ui/terminal/mod.rs`:
- Around line 85-88: The bailout message in src/app/mod.rs that tells users
“interactive mode requires Ghostty or Kitty” is stale because
is_supported_terminal now accepts "bcon"; update that message to include "bcon"
(e.g., "Ghostty, bcon, or Kitty") or, better, derive the supported-names list
programmatically and reuse the logic from is_supported_terminal to avoid
drifting; change the string in the bailout/deprecation/usage output to match the
new supported set so users see accurate guidance.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89c73b3e-0397-49db-b49c-cbebcac3d610
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomldocs/superpowers/plans/2026-03-30-linux-render-html-to-png.mddocs/superpowers/specs/2026-03-30-linux-render-html-to-png-design.mdsrc/io/webkit_snapshot/mod.rssrc/io/webkit_snapshot/paths.rssrc/ui/terminal/mod.rssrc/ui/terminal/tests.rs
- Use url::Url::from_file_path() for proper file URL construction - Make remote images blocking in JS visual stability probe - Remove --disable-web-security Chrome flag - Update error message to include bcon terminal Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/io/webkit_snapshot/mod.rs (1)
260-275: Consider explicit error instead of defaulting ready flags totrue.If the JS probe returns malformed JSON (missing keys), the current code defaults
fonts_ready,images_ready, andmermaids_readytotrue, which could cause premature screenshots. Since theVISUAL_STABILITY_JSis controlled code that should always return valid structure, this is low-risk, but logging a warning when defaults are used would aid debugging rendering issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/io/webkit_snapshot/mod.rs` around lines 260 - 275, The ready flags (fonts_ready, images_ready, mermaids_ready) are being defaulted to true silently when payload keys are missing or malformed; change the logic to detect when payload.get(...).and_then(...).is_none() and, instead of silently using unwrap_or(true), emit a warning (e.g., using the project's logging facility like warn! or process_logger.warn) that includes which key fell back to the default and the offending payload (or a brief snippet), then set the flag to true; reference the payload variable and the fonts_ready/images_ready/mermaids_ready bindings as the locations to update, and note VISUAL_STABILITY_JS as the producer of the JSON in the log message for context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/io/webkit_snapshot/mod.rs`:
- Around line 260-275: The ready flags (fonts_ready, images_ready,
mermaids_ready) are being defaulted to true silently when payload keys are
missing or malformed; change the logic to detect when
payload.get(...).and_then(...).is_none() and, instead of silently using
unwrap_or(true), emit a warning (e.g., using the project's logging facility like
warn! or process_logger.warn) that includes which key fell back to the default
and the offending payload (or a brief snippet), then set the flag to true;
reference the payload variable and the fonts_ready/images_ready/mermaids_ready
bindings as the locations to update, and note VISUAL_STABILITY_JS as the
producer of the JSON in the log message for context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 974c2829-7676-4b38-b879-2a18433711ef
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlsrc/app/mod.rssrc/io/webkit_snapshot/mod.rs
✅ Files skipped from review due to trivial changes (2)
- src/app/mod.rs
- Cargo.toml
If the visual stability probe returns unexpected JSON, defaulting to false (not ready) is safer than true — the probe loop will simply retry until timeout rather than taking a premature screenshot. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/io/webkit_snapshot/mod.rs (1)
150-150: Consider not swallowing workspace cleanup failures.At Line 150,
let _ = cleanup_workspace(&workspace);hides filesystem cleanup errors, which can accumulate stale.mdv-webkitdirs over time. Recommend at least surfacing this failure (warning or propagated context).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/io/webkit_snapshot/mod.rs` at line 150, The workspace cleanup call currently swallows errors by using `let _ = cleanup_workspace(&workspace);`; change this to handle the Result from `cleanup_workspace` instead of ignoring it—either propagate the error (use `?` from `function` that returns Result) or log the failure (e.g., call your logger with a warning/error including the `workspace` path and the returned error) so filesystem cleanup failures for `.mdv-webkit` are visible; update the call site in the function where `cleanup_workspace` is invoked and ensure the enclosing function signature is adjusted if you choose to propagate the error.
🤖 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/io/webkit_snapshot/mod.rs`:
- Around line 251-259: The current parsing of the visual-stability probe (the
json_str extraction and serde_json::from_str into payload) should not propagate
errors; instead treat missing or malformed probe responses as "not ready" so the
stability loop can continue. Replace the .context(...) error paths around
remote.value.as_ref().and_then(|v| v.as_str()) and
serde_json::from_str(json_str) with non-failing handling: if the string is
missing or parsing fails, log/debug the issue and return the probe-not-ready
result (e.g., Ok(None) or an explicit NotReady variant) rather than returning an
Err, updating the function that consumes json_str/payload so callers continue
retrying until timeout. Ensure you reference the visual-stability probe handling
code where json_str and payload are used so the control-flow change is applied
consistently.
- Around line 212-214: The Linux snapshot branch currently returns
SnapshotDiagnostics::default() which drops all probe data; replace that default
with a SnapshotDiagnostics instance populated from the Linux probe variables
collected earlier in this function (mirror the macOS return path), e.g. build
SnapshotDiagnostics with the same fields (probe timings, CPU/GPU/temperature/hid
probe results or whatever probe_* variables are in scope) instead of
SnapshotDiagnostics::default(), and ensure the constructed diagnostics are
included in the SnapshotResult returned by this code path (update any unit tests
that assert diagnostics if present).
---
Nitpick comments:
In `@src/io/webkit_snapshot/mod.rs`:
- Line 150: The workspace cleanup call currently swallows errors by using `let _
= cleanup_workspace(&workspace);`; change this to handle the Result from
`cleanup_workspace` instead of ignoring it—either propagate the error (use `?`
from `function` that returns Result) or log the failure (e.g., call your logger
with a warning/error including the `workspace` path and the returned error) so
filesystem cleanup failures for `.mdv-webkit` are visible; update the call site
in the function where `cleanup_workspace` is invoked and ensure the enclosing
function signature is adjusted if you choose to propagate the error.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b31897d1-fc6b-4327-a74d-1a0ea2e4980e
📒 Files selected for processing (1)
src/io/webkit_snapshot/mod.rs
Visual stability probe errors (JS evaluation, JSON parsing) now cause the loop to sleep and retry rather than immediately failing the entire render_html_to_png call. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
Re: diagnostics data nitpick — this is an intentional design choice. If diagnostics become useful in the future (e.g., for a |
Return fonts_ready, images_ready, and mermaids_ready from the visual stability probe instead of always returning default (empty) diagnostics. This aligns Linux behavior with macOS and avoids silent drift in SnapshotResult. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/io/webkit_snapshot/mod.rs`:
- Around line 189-198: The code uses probe.height (from await_visual_stability)
directly to compute snapshot_height and pass it into tab.set_bounds, which can
cause OOM/hangs for huge pages; clamp or reject excessive heights before calling
set_bounds by introducing a safe maximum (e.g., MAX_SNAPSHOT_HEIGHT_PX) and then
replace snapshot_height = probe.height.ceil().max(1.0) with logic that bounds it
(or returns an error if probe.height exceeds a hard limit). Update the resizing
call that uses Bounds::Normal and tab.set_bounds to use the clamped value (or
bail out with a clear error) so await_visual_stability/probe.height cannot drive
unbounded viewport sizes.
- Around line 274-280: The probe loop currently treats all errors the same and
can return the fallback as Ok even if no probe ever succeeded; add a boolean
flag (e.g. probe_succeeded = false) initialized before the loop, set
probe_succeeded = true whenever probe_result matches Ok(v) and you update
last_height, and after exhausting the retry budget check probe_succeeded and
call bail! with an explanatory error if it is still false; also update the
fallback branch that currently returns Ok based on last_height to instead
require probe_succeeded (or bail) so a completely failed probe sequence does not
produce a 1px/false-success render. Ensure you reference and update the existing
probe_result handling, last_height updates, PROBE_INTERVAL loop, and the code
path that previously returned the fallback Ok.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dcc716b6-3151-4c85-ae42-afd3fbd99695
📒 Files selected for processing (1)
src/io/webkit_snapshot/mod.rs
- Clamp computed document height to 32000px to prevent OOM from pathological HTML inputs - Track whether any probe succeeded; bail with an error message if all probes fail instead of returning a 1px screenshot as success Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
Hi @posaune0423, just checking in — are there any concerns or changes needed on this PR? Happy to address feedback if anything comes up. |
Run `make ci` to pass fmt-check, clippy, and tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
Sorry, I forgot to run |
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/io/webkit_snapshot/mod.rs`:
- Around line 306-313: The current comment incorrectly claims the fallback
Ok(StabilityProbe { ... }) is unreachable; update it to explain that this path
is reachable when earlier probes succeed but a subsequent probe fails (the loop
exits without returning via the earlier Ok branch), so we return the last known
good height with conservative readiness flags. Mention the relevant symbols: the
loop that returns Ok(...) on successful probe, the Err(_) branch that can break
the loop, and the returned StabilityProbe with fields last_height, fonts_ready,
images_ready, mermaids_ready; keep the new comment short and precise about this
specific edge case.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c2fb25c-f537-4a6f-bc12-ed2713d1ad79
📒 Files selected for processing (1)
src/io/webkit_snapshot/mod.rs
The fallback path is reachable when earlier probes succeed but the final probe fails, not unreachable as the comment stated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
|
@kay-ws thank you for your contribution. I will have a look at the code later! |
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
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.
Summary
Enable Graphic Mode on Linux by implementing
render_html_to_pngusing theheadless_chromecrate. This replaces thebail!("only supported on macOS")stub with a working Chrome-based screenshot pipeline, allowing Linux users with Kitty Graphics Protocol-capable terminals to see browser-quality Markdown rendering.render_html_to_pngfor Linux using headless Chrome (CDP)bconterminal to the supported terminal listbase_dir = ".") viafs::canonicalizeArchitecture
The existing pipeline is preserved — only the
render_html_to_pngstep is replaced:Chrome Binary Resolution
MDV_CHROME_PATHenvironment variableheadless_chrome::browser::default_executable()(searchesCHROMEenv var, then PATH forgoogle-chrome-stable,chromium,chromium-browser, etc.)No automatic Chrome download — users must have Chrome/Chromium installed.
Visual Stability Probing
Ported from the macOS Swift implementation with matching constants:
MAX_PROBESPROBE_INTERVALHEIGHT_TOLERANCESTABLE_COUNT_REQUIREDJavaScript evaluates
fontsReady,imagesReady,mermaidsReady, andheight. Screenshot is taken when all visuals are ready and height is stable for 2 consecutive probes, or after 40 probes (timeout).Platform cfg Gates
cfg(target_os = "macos")headless_chromedepcfg(target_os = "linux")cfg(not(any(macos, linux)))→ bailFiles Changed
Cargo.tomlheadless_chromeascfg(target_os = "linux")dependencysrc/io/webkit_snapshot/mod.rssrc/io/webkit_snapshot/paths.rssrc/ui/terminal/mod.rsbcontois_supported_terminal()src/ui/terminal/tests.rsNo changes to macOS code paths. No new files.
Test Plan
cargo check— compiles without warningscargo test— all 122 tests pass (4 new terminal detection tests)./README.md(relative path) — images load correctly--no-mermaidoption — worksmmdc— renders with placeholderMDV_CHROME_PATHoverride — worksTest Environment
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores