Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughThe CLI adds screenshot quality and size flags plus two heap snapshot commands. Routing, screenshot execution, heap snapshot capture, offline heap inspection, and direct dispatch were updated to support the new flows. ChangesScreenshot and heap snapshot feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new features for screencast recording, heap snapshotting, and advanced screenshot options such as quality, max width, and max height. However, several critical issues were identified in the review: the screencast feature is currently non-functional due to invalid CDP parameters and a lack of frame-handling logic; the heap snapshot parser is prone to out-of-bounds panics on malformed files; the heap profiler can be left enabled on error, causing resource leaks; and screenshot downscaling lacks validation for negative or zero dimensions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![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
🧹 Nitpick comments (1)
src/commands/memory.rs (1)
66-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the narrative scratch comments.
These comments describe a thought process ("Let's implement...", "Actually, V8...") rather than why the code is structured this way.
As per coding guidelines: "Comments should explain why, not what".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/memory.rs` around lines 66 - 71, The comments in the snapshot parsing section are narrative scratch notes rather than explanatory documentation; remove the “Let’s implement…” and “Actually…” style text near the heap snapshot parser in memory.rs. Keep only comments that explain the reason for the chosen approach, and if needed reword them to describe why the parser is structured that way around the snapshot parsing logic and dominator reconstruction.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/memory.rs`:
- Around line 62-121: `get_heapsnapshot_dominators` does not follow the
command-function contract and its behavior does not match its name. Update this
command to accept `&mut CdpClient`, `session_id`, and `OutputFormat` like the
other functions in `src/commands/memory.rs`, and route any CDP/session work
through that signature instead of the current file-only flow. Then either
implement the actual dominator/retained-size computation or rename the
function/output to reflect that it only performs node lookup; the current
`retainedSize` value in `get_heapsnapshot_dominators` is hard-coded to unknown.
If you keep the parsing logic, extract the heap snapshot parsing and node lookup
into a pure helper so it can be tested independently, and add tests for that
helper.
- Around line 22-53: The heap snapshot logic in memory::snapshot_result is
broken because it waits for HeapProfiler.takeHeapSnapshot to finish before
reading streamed HeapProfiler.addHeapSnapshotChunk events, and it also disables
reportProgress while still relying on HeapProfiler.reportHeapSnapshotProgress to
exit. Update the HeapProfiler.takeHeapSnapshot call to use progress
appropriately and restructure the flow so chunk events are consumed via the
client’s event handling instead of client.read_text() after send_to_target
returns; in particular, handle addHeapSnapshotChunk as events arrive and
terminate based on the correct completion signal for takeHeapSnapshot rather
than waiting on a progress event that will never be emitted.
In `@src/lib.rs`:
- Around line 933-947: `GetHeapSnapshotDominators` is being dispatched through
the page-attached `run_direct` flow even though
`commands::memory::get_heapsnapshot_dominators` only needs a local file. Move
this command onto the early browser-independent path used for non-session
commands so it bypasses `client.resolve_page`, `client.attach_to_target`, and
`Page.enable`, and can run without a live tab. Apply the same routing fix in the
daemon executor path as well, using the
`Commands::GetHeapSnapshotDominators`/`get_heapsnapshot_dominators` handling as
the entry point.
---
Nitpick comments:
In `@src/commands/memory.rs`:
- Around line 66-71: The comments in the snapshot parsing section are narrative
scratch notes rather than explanatory documentation; remove the “Let’s
implement…” and “Actually…” style text near the heap snapshot parser in
memory.rs. Keep only comments that explain the reason for the chosen approach,
and if needed reword them to describe why the parser is structured that way
around the snapshot parsing logic and dominator reconstruction.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 960bf501-d587-4ee3-93da-98b76232b4f0
📒 Files selected for processing (5)
src/commands/executor.rssrc/commands/memory.rssrc/commands/mod.rssrc/commands/screenshot.rssrc/lib.rs
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces memory profiling capabilities by adding commands to take heap snapshots (take-heapsnapshot) and inspect specific nodes within them (inspect-heapsnapshot-node). It also enhances the screenshot command with options for compression quality and downscaling limits (max_width and max_height). The review feedback highlights critical improvements: executing the synchronous heap snapshot parser within a blocking task to prevent blocking the async executor, buffering non-snapshot CDP events during streaming to avoid losing them, and clamping the screenshot quality parameter to a maximum of 100 to prevent CDP errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces heap snapshot capabilities, adding commands to take a heap snapshot (take-heapsnapshot) and inspect a specific node from a local snapshot (inspect-heapsnapshot-node). It also enhances the screenshot command with support for quality compression and downscaling (max_width and max_height). Feedback on the changes highlights a performance and safety issue in the heap snapshot parser: parsing large JSON snapshots into a generic serde_json::Value can lead to high memory usage or OOM crashes, and an empty node_fields array could trigger an infinite loop. It is recommended to deserialize into a strongly-typed struct and validate the node size.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces heap snapshotting capabilities, including taking heap snapshots and inspecting specific nodes from local snapshot files. It also enhances the screenshot command with support for compression quality and downscaling options (max_width and max_height). The feedback identifies a potential out-of-bounds panic when parsing malformed heap snapshots, an unreachable match arm in the CLI execution flow, and a potential event buffer pollution issue caused by unhandled heap profiler progress events.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces memory profiling capabilities by adding the take-heapsnapshot and inspect-heapsnapshot-node commands, implemented in a new memory module. It also enhances the screenshot command with options for compression quality and downscaling dimensions (max_width and max_height). Feedback on the changes suggests escaping the node name when formatting the output as text in inspect_heapsnapshot_node to ensure the generated CSV remains valid and parseable if the name contains commas, quotes, or newlines.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/commands/screenshot.rs (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the sizing comments explain intent, not the next statement.
These comments mostly restate the code; either remove them or explain why the command uses evaluated page dimensions before setting a custom clip. As per coding guidelines: “Comments should explain why, not what.”
Also applies to: 54-54, 73-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/screenshot.rs` at line 30, The sizing comments in the screenshot command are too descriptive of the next statement and should be rewritten or removed. Update the comments around the viewport/document dimension logic in `screenshot` to explain why `evaluate` is used before setting a custom clip, or drop them if they add no intent. Apply the same cleanup to the other matching comments in the same command so the documentation explains the reasoning, not the mechanics.Source: Coding guidelines
src/commands/memory.rs (2)
151-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract node output formatting into a testable helper.
The CSV escaping and structured-output conversion are pure formatting logic but are embedded in the command body and not directly tested. Move this into a small helper and add cases for commas, quotes, and newlines. As per coding guidelines: “Pure conversion and formatting logic should be extracted into testable functions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/memory.rs` around lines 151 - 171, The formatting logic in the memory command is embedded directly in the command body, making it hard to test; extract the CSV escaping and structured output creation from the `format.is_text()` branch in `memory.rs` into a small helper used by the `nodeId/nodeName/selfSize/retainedSize` output path. Then add unit tests for that helper covering names with commas, quotes, and newlines, and verify both text and structured formatting behaviors without relying on the command flow itself.Source: Coding guidelines
15-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd context to user-visible I/O and parsing failures.
Bare
?here can surface ambiguous errors for bad output paths, write failures, or invalid heap snapshot files. Wrap these with descriptive context. As per coding guidelines: “Error handling should use anyhow::Result with descriptive messages.”🛠️ Proposed fix
-use anyhow::{anyhow, bail, Result}; +use anyhow::{anyhow, bail, Context, Result}; @@ - let mut file = tokio::fs::File::create(output).await?; + let mut file = tokio::fs::File::create(output) + .await + .with_context(|| format!("Failed to create heap snapshot output file: {output}"))?; @@ - file.write_all(chunk.as_bytes()).await?; + file.write_all(chunk.as_bytes()) + .await + .context("Failed to write heap snapshot chunk")?; @@ - let file = File::open(file_path)?; + let file = File::open(file_path) + .with_context(|| format!("Failed to open heap snapshot file: {file_path}"))?; let reader = BufReader::new(file); - let val: HeapSnapshot = serde_json::from_reader(reader)?; + let val: HeapSnapshot = serde_json::from_reader(reader) + .with_context(|| format!("Failed to parse heap snapshot JSON: {file_path}"))?;Also applies to: 47-47, 91-93
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/memory.rs` around lines 15 - 18, The memory command is propagating bare errors from file I/O and heap snapshot handling, so wrap the failing operations with descriptive anyhow context instead of using plain ?; update the logic around the memory command flow (including the File::create path and the heap snapshot parsing/write steps referenced by the other noted spots) to report whether the failure came from creating the output file, writing snapshot data, or parsing an invalid snapshot. Use the existing memory command function and related helper calls so the user-visible error messages clearly say what operation failed and on which input.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/memory.rs`:
- Around line 10-14: The take_heapsnapshot command currently hardcodes a text
success response and cannot honor structured output. Update take_heapsnapshot to
accept OutputFormat alongside &mut CdpClient, session_id, and output, then
thread req.format() through the dispatcher/executor call so the command can emit
the correct response format. Make sure the function signature and call sites
follow the command pattern used elsewhere, with OutputFormat passed explicitly
into take_heapsnapshot.
In `@src/commands/screenshot.rs`:
- Around line 60-68: The downscaled viewport screenshot path in the screenshot
capture logic is resetting the clip origin to the document top-left instead of
the current viewport, so update the code around the viewport metrics and clip
construction to also read scrollX and scrollY alongside width and height. Then
pass those scroll offsets into the clip used when full_page is false and
downscaling is enabled, ensuring the visible area is captured in the screenshot
flow in screenshot.rs.
---
Nitpick comments:
In `@src/commands/memory.rs`:
- Around line 151-171: The formatting logic in the memory command is embedded
directly in the command body, making it hard to test; extract the CSV escaping
and structured output creation from the `format.is_text()` branch in `memory.rs`
into a small helper used by the `nodeId/nodeName/selfSize/retainedSize` output
path. Then add unit tests for that helper covering names with commas, quotes,
and newlines, and verify both text and structured formatting behaviors without
relying on the command flow itself.
- Around line 15-18: The memory command is propagating bare errors from file I/O
and heap snapshot handling, so wrap the failing operations with descriptive
anyhow context instead of using plain ?; update the logic around the memory
command flow (including the File::create path and the heap snapshot
parsing/write steps referenced by the other noted spots) to report whether the
failure came from creating the output file, writing snapshot data, or parsing an
invalid snapshot. Use the existing memory command function and related helper
calls so the user-visible error messages clearly say what operation failed and
on which input.
In `@src/commands/screenshot.rs`:
- Line 30: The sizing comments in the screenshot command are too descriptive of
the next statement and should be rewritten or removed. Update the comments
around the viewport/document dimension logic in `screenshot` to explain why
`evaluate` is used before setting a custom clip, or drop them if they add no
intent. Apply the same cleanup to the other matching comments in the same
command so the documentation explains the reasoning, not the mechanics.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd780feb-5e74-4c7e-95d1-7f1e28c5723f
📒 Files selected for processing (5)
src/commands/executor.rssrc/commands/memory.rssrc/commands/mod.rssrc/commands/screenshot.rssrc/lib.rs
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces memory profiling capabilities with two new commands, take-heapsnapshot and inspect-heapsnapshot-node, allowing users to capture and analyze heap snapshots. Additionally, it enhances the screenshot command by adding options for compression quality and downscaling (max_width and max_height). The review feedback suggests improving I/O performance during heap snapshot generation by buffering file writes with BufWriter, and preventing potential CDP errors by normalizing the screenshot format string to lowercase before performing case-sensitive checks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces heap snapshot capabilities with two new commands, take-heapsnapshot and inspect-heapsnapshot-node, and extends the screenshot command with options for quality compression and downscaling (max_width and max_height). Feedback highlights two issues: first, using document-relative scroll offsets for viewport screenshot clipping coordinates can cause incorrect clipping or blank images when scrolled; second, pushing concurrent events directly to client.events during a heap snapshot bypasses event routing and can lead to unbounded memory growth.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/commands/screenshot.rs (2)
57-76: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the extra metrics round trip for normal viewport screenshots.
When
full_pageis false and no max dimensions are set,src_w,src_h, and scroll offsets are never used because noclipis emitted. Gate thisRuntime.evaluatebehindmax_width.is_some() || max_height.is_some()to keep the default screenshot path single-call.Proposed fix
- } else { + } else if max_width.is_some() || max_height.is_some() { let metrics = client .send_to_target( session_id,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/screenshot.rs` around lines 57 - 76, The normal viewport screenshot path in screenshot.rs is doing an unnecessary Runtime.evaluate round trip even though src_w, src_h, and scroll_x/scroll_y are unused when no clip is emitted. Update the branching around the client.send_to_target call in the screenshot logic so this metrics fetch only runs when max_width.is_some() || max_height.is_some(), keeping the default full_page false path to a single call while preserving the existing clip behavior when max dimensions are requested.
78-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the clip-scale calculation into a pure tested helper.
This downscale math is pure conversion logic and now carries edge cases around zero, negative, and one-sided limits. Moving it into a small helper makes those cases testable without CDP setup. As per coding guidelines: "Pure conversion and formatting logic should be extracted into testable functions".
Proposed refactor
+fn compute_clip_scale( + src_w: f64, + src_h: f64, + max_width: Option<f64>, + max_height: Option<f64>, +) -> f64 { + let width_scale = match max_width { + Some(max_w) if max_w > 0.0 && src_w > 0.0 => (max_w / src_w).min(1.0), + _ => 1.0, + }; + let height_scale = match max_height { + Some(max_h) if max_h > 0.0 && src_h > 0.0 => (max_h / src_h).min(1.0), + _ => 1.0, + }; + width_scale.min(height_scale) +} + - let mut clip_scale = 1.0; - if max_width.is_some() || max_height.is_some() { - let width_scale = match max_width { - Some(max_w) if max_w > 0.0 && src_w > 0.0 => (max_w / src_w).min(1.0), - _ => 1.0, - }; - let height_scale = match max_height { - Some(max_h) if max_h > 0.0 && src_h > 0.0 => (max_h / src_h).min(1.0), - _ => 1.0, - }; - clip_scale = width_scale.min(height_scale); - } + let clip_scale = compute_clip_scale(src_w, src_h, max_width, max_height);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/screenshot.rs` around lines 78 - 90, The clip-scale downscaling logic inside screenshot handling is pure conversion code and should be extracted from the screenshot command flow into a small helper so it can be unit tested independently. Move the max-width/max-height scale calculation out of the screenshot path in screenshot::command logic into a dedicated pure function with a clear name, then add tests covering zero, negative, one-sided, and both-dimensions cases to validate the helper’s behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/memory.rs`:
- Around line 31-44: The heap snapshot flow in `memory.rs` is using an unbounded
raw `read_text()` loop, which can hang the daemon and bypass the centralized CDP
path. Move this logic into a `CdpClient` helper that uses `send_to_target()` for
the command, owns the raw read/stream handling, preserves unrelated frames, and
enforces an idle timeout while collecting chunks. Keep the existing
`send_raw_no_wait`/`read_text` behavior out of the command handler and route the
streaming through the helper instead.
---
Nitpick comments:
In `@src/commands/screenshot.rs`:
- Around line 57-76: The normal viewport screenshot path in screenshot.rs is
doing an unnecessary Runtime.evaluate round trip even though src_w, src_h, and
scroll_x/scroll_y are unused when no clip is emitted. Update the branching
around the client.send_to_target call in the screenshot logic so this metrics
fetch only runs when max_width.is_some() || max_height.is_some(), keeping the
default full_page false path to a single call while preserving the existing clip
behavior when max dimensions are requested.
- Around line 78-90: The clip-scale downscaling logic inside screenshot handling
is pure conversion code and should be extracted from the screenshot command flow
into a small helper so it can be unit tested independently. Move the
max-width/max-height scale calculation out of the screenshot path in
screenshot::command logic into a dedicated pure function with a clear name, then
add tests covering zero, negative, one-sided, and both-dimensions cases to
validate the helper’s behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f5ba365-1894-42a1-8466-a7ae1ac99bd6
📒 Files selected for processing (5)
src/commands/executor.rssrc/commands/memory.rssrc/commands/mod.rssrc/commands/screenshot.rssrc/lib.rs
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces heap snapshot capabilities with two new commands, take-heapsnapshot and inspect-heapsnapshot-node, and extends the screenshot command with options for quality compression and downscaling (max_width and max_height). Feedback on these changes includes setting reportProgress to false during snapshot capture to eliminate redundant progress events, using a safer JavaScript expression to prevent potential TypeErrors when retrieving document dimensions on null elements, and considering a streaming JSON parser to mitigate memory overhead and OOM risks when deserializing large heap snapshots.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Code Review
This pull request introduces memory profiling commands (take-heapsnapshot and inspect-heapsnapshot-node) to capture and analyze heap snapshots, and enhances the screenshot command with new options for compression quality and downscaling (max_width and max_height). Feedback suggests replacing the fragile Runtime.evaluate calls with the native Page.getLayoutMetrics CDP command to reliably retrieve viewport and document dimensions without executing JavaScript.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/memory.rs`:
- Around line 166-167: The string lookup in memory snapshot parsing is masking
malformed data by falling back to "unknown" in the name extraction path. Update
the logic around name_str_idx and the name resolution in the memory command so
that an out-of-bounds or missing string index fails the inspection instead of
using a placeholder, and propagate an error from the relevant parsing flow in
src/commands/memory.rs to make corrupt snapshots surface as failures.
- Around line 19-23: The heap snapshot export in memory::command currently
creates the final output file up front, which can leave a partial or empty file
if later CDP streaming or parsing fails. Update the logic around the
BufWriter/File::create path to write into a temporary file in the same
directory, complete the snapshot work there, and only rename the temp file to
the final output after snapshot_result succeeds. Keep the fix localized to the
snapshot-writing flow so the final output path is only touched on success.
In `@src/commands/screenshot.rs`:
- Around line 54-80: The screenshot flow in the command handling logic should
add descriptive anyhow context around the metric collection and file-writing
steps so these failures are distinguishable from capture failures. Update the
code around the Runtime.evaluate / send_to_target calls and the screenshot
output write path to wrap errors with Context messages that mention whether the
failure happened while reading dimensions or writing the screenshot file, and
import anyhow::Context if it is not already available.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e8478046-aea1-465f-adac-40edd8755317
📒 Files selected for processing (6)
src/cdp.rssrc/commands/executor.rssrc/commands/memory.rssrc/commands/mod.rssrc/commands/screenshot.rssrc/lib.rs
* expose `CdpClient::push_event` for controlled event routing and buffering * update heap snapshot handling to use `push_event` for capped and categorized event storage * simplify screenshot metric retrieval by fetching dimensions only when needed * remove scroll offset usage and standardize clip origin to (0,0) for correctness * extract `clip_scale_factor` helper for clearer downscaling logic * add comprehensive unit tests for clip scaling edge cases * improve readability and maintainability of screenshot capture flow
* disable `reportHeapSnapshotProgress` to avoid unnecessary event handling * remove redundant progress-event branch in memory command * improve screenshot dimension fallback using optional chaining and viewport defaults
…nshot commands * write heap snapshots to temporary file and atomically rename on success to avoid corrupt outputs * clean up partial temp files on failure * add buffered writes with explicit flush and improved error context * validate snapshot string indices and return explicit corruption errors * switch screenshot metrics to `Page.getLayoutMetrics` for better accuracy and non-DOM support * simplify dimension retrieval logic and remove JS evaluation dependency * add detailed error context for CDP calls and file writes
…handling * use unique PID-based temp files for heap snapshots to avoid collisions and ensure atomic rename * clean up temp files on failure and prevent corrupt output artifacts * extract `find_node_in_snapshot` for pure schema validation and unit testing * add overflow-safe index handling and improved corruption errors * introduce offline `inspect_heapsnapshot_node` (no Chrome required) with CLI early intercept * remove daemon dependency for offline inspection command * absolutize file paths before sending to daemon to fix CWD inconsistencies
…output format for node details
|
/gemini review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Code Review
This pull request introduces heap snapshot capabilities, including a take-heapsnapshot command to stream V8 heap snapshots via CDP and an offline inspect-heapsnapshot-node command to parse local .heapsnapshot files. It also enhances the screenshot command with new options for quality, maximum width, and maximum height, switching to Page.getLayoutMetrics for layout dimensions. Feedback on the changes highlights a potential issue in screenshot dimension parsing where non-positive layout metrics (such as 0.0) could bypass the default fallback and cause Chrome to fail, with a suggestion to filter for strictly positive values.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 135: The README entry for read-page --output <path> uses lowercase
“markdown” in user-facing text; update this copy to “Markdown” for consistency
with the rest of the documentation. Locate the table row containing read-page
--output <path> and adjust the displayed description only, keeping the rest of
the wording unchanged.
In `@src/lib.rs`:
- Around line 385-395: The path resolution flow in absolutize_path and
build_request is swallowing CWD lookup failures via unwrap_or_default(), which
can hide an unresolved CLI working directory. Change absolutize_path to return
anyhow::Result<String> with a descriptive error when current_dir() cannot be
obtained, then make build_request return anyhow::Result and propagate that
failure through its path insertions into the JSON payload. Update the call site
to use let request = build_request(&cli)?; and ensure all callers handle errors
via ? instead of assuming a fallback path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 945f7c21-d8fb-40d8-8252-ca9f23eac539
📒 Files selected for processing (9)
AGENTS.mdREADME.mdsrc/cdp.rssrc/commands/executor.rssrc/commands/memory.rssrc/commands/mod.rssrc/commands/screenshot.rssrc/lib.rstests/telemetry_tests.rs
…ns for screenshots
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces features for capturing V8 heap snapshots (take-heapsnapshot) and inspecting snapshot nodes offline (inspect-heapsnapshot-node), alongside screenshot enhancements such as quality adjustment and downscaling. Review feedback highlights two key improvements: correcting screenshot clipping coordinates to use viewport scroll offsets (pageX/pageY) to prevent incorrect captures on scrolled pages, and removing the offline-only inspect-heapsnapshot-node command from the daemon's browser-level commands list to avoid potential panics.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces features for capturing V8 heap snapshots (take-heapsnapshot) and inspecting heap snapshot nodes offline (inspect-heapsnapshot-node). It also enhances the screenshot command with quality and downscaling options (max_width, max_height) using Page.getLayoutMetrics instead of executing JavaScript. Additionally, relative paths are now resolved to absolute paths before being sent to the daemon. The review feedback suggests implementing a Drop guard for the temporary heap snapshot file to ensure reliable cleanup upon future cancellation or panics, which would also simplify the error handling path.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces heap snapshot functionality and enhances screenshot capabilities. It adds a take-heapsnapshot command to stream V8 heap snapshots via CDP, and an offline inspect-heapsnapshot-node command to parse local .heapsnapshot files. The screenshot command is updated to support compression quality and downscaling dimensions (--max-width and --max-height) using Page.getLayoutMetrics. Additionally, relative file paths are now resolved to absolute paths before being sent to the daemon. No review comments were provided, so there is no feedback to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/commands/screenshot.rs (1)
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAlign the command handler signature with the command contract.
Line 18 now takes
ScreenshotOptions, but the command handler still omitsOutputFormat; update the signature and call sites to include it, even as_output_formatif this command intentionally ignores formatted output.As per coding guidelines, "
src/commands/*.rs: Each command should be implemented as a pure async function that takes&mut CdpClient,session_id,OutputFormat, and command-specific arguments."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/screenshot.rs` around lines 18 - 22, The take_screenshot command handler signature does not match the command contract because it still accepts only ScreenshotOptions and omits OutputFormat. Update take_screenshot to take OutputFormat as required by the src/commands/*.rs convention, and adjust every caller to pass it through; if the screenshot command does not use formatting, accept it as _output_format to preserve the contract while ignoring it.Source: Coding guidelines
tests/telemetry_tests.rs (1)
3-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlease colocate this smoke test with
src/telemetry.rs.This compile-only export check extends
tests/telemetry_tests.rsinstead of the in-file#[cfg(test)] mod testsused elsewhere, so it keeps drifting away from the repo’s test layout convention. As per coding guidelines, “Tests should live in#[cfg(test)] mod testswithin the same file.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/telemetry_tests.rs` around lines 3 - 7, The smoke test for telemetry export accessibility is in the wrong place and should be moved into the same-file test module pattern used elsewhere. Relocate the compile-only check that references chrome_devtools_cli::telemetry and init_logger_once into a #[cfg(test)] mod tests inside src/telemetry.rs, keeping the same assertion that the module is publicly exported and accessible.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/memory.rs`:
- Around line 101-117: The temp-file handle from the heap snapshot writer is
still alive when `tokio::fs::rename` runs, which can block the move on Windows.
In the `memory` command flow around the buffered writer `file`, explicitly
release the handle after `file.flush().await` and before the rename by dropping
`file`, so the atomic move of `temp_path` to `output_path` can succeed reliably.
In `@src/commands/screenshot.rs`:
- Around line 44-47: The screenshot parameter handling currently sets
Page.captureScreenshot quality for any non-PNG format, which includes WebP and
can be inconsistent. Update the logic in the screenshot command so the quality
field is only added when the requested format is JPEG, and keep the existing
clamping behavior in the same quality-setting branch.
- Around line 74-88: The screenshot clip calculation is using deprecated
`contentSize`/`layoutViewport` metrics instead of CSS layout metrics, which can
produce incorrect CSS-pixel coordinates on HiDPI or emulated devices. Update the
screenshot capture logic to read the CSS layout metrics for the full-page and
viewport paths, and use those values consistently for `src_w`, `src_h`,
`scroll_x`, and `scroll_y`. Keep the existing bounds/filtering behavior and the
`max_width`/`max_height` scaling logic aligned with the new CSS-pixel-based
values in the screenshot command implementation.
---
Nitpick comments:
In `@src/commands/screenshot.rs`:
- Around line 18-22: The take_screenshot command handler signature does not
match the command contract because it still accepts only ScreenshotOptions and
omits OutputFormat. Update take_screenshot to take OutputFormat as required by
the src/commands/*.rs convention, and adjust every caller to pass it through; if
the screenshot command does not use formatting, accept it as _output_format to
preserve the contract while ignoring it.
In `@tests/telemetry_tests.rs`:
- Around line 3-7: The smoke test for telemetry export accessibility is in the
wrong place and should be moved into the same-file test module pattern used
elsewhere. Relocate the compile-only check that references
chrome_devtools_cli::telemetry and init_logger_once into a #[cfg(test)] mod
tests inside src/telemetry.rs, keeping the same assertion that the module is
publicly exported and accessible.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5b7d9bd-bce7-49ac-9c05-3a3a9bb70b6e
📒 Files selected for processing (9)
AGENTS.mdREADME.mdsrc/cdp.rssrc/commands/executor.rssrc/commands/memory.rssrc/commands/mod.rssrc/commands/screenshot.rssrc/lib.rstests/telemetry_tests.rs
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces V8 heap snapshot capabilities to the CLI, adding a take-heapsnapshot command to stream heap snapshots via CDP and an offline inspect-heapsnapshot-node command to inspect node details from a local .heapsnapshot file. Additionally, it enhances the screenshot command with new options for compression quality (--quality) and downscaling (--max-width, --max-height), refactoring the layout metric retrieval to use Page.getLayoutMetrics instead of JS evaluation. Path resolution has also been improved to absolutize relative paths on the CLI side before sending requests to the daemon. Since there are no review comments provided, I have no further feedback to offer on these changes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1 participant
Summary by CodeRabbit
take-heapsnapshotto write.heapsnapshotfiles andinspect-heapsnapshot-nodeto inspect a node from a local snapshot (CSV or JSON).screenshotwith optional--quality,--max-width, and--max-heightfor controlled downscaling.