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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds an offline ChangesCompare Heapsnapshots Feature
Daemon Connection and Kill Handling
Formatting Reflows
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![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 a new offline command, compare-heapsnapshots, which allows users to compare two .heapsnapshot files and view per-class differences in added/deleted nodes and size deltas without requiring a Chrome connection. It implements the necessary parsing, aggregation, diffing, and formatting logic, along with comprehensive unit tests. The feedback highlights a critical issue where sorting the diffs is non-deterministic for classes with identical size deltas due to the randomized iteration order of HashSet. This can cause the index used to query detailed class views to change across CLI invocations. To resolve this, a deterministic tie-breaker sorting by class name is recommended.
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib.rs (1)
251-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer auto-derived
shortfor consistency.Elsewhere in this enum (e.g.
InspectHeapSnapshotNode,outputfields) short flags are auto-derived via#[arg(long, short)]rather than explicitly pinned. Since'b'/'c'are exactly the auto-derived values here, using the same convention would be more consistent.♻️ Proposed consistency tweak
/// Path to the base (earlier) .heapsnapshot file - #[arg(long, short = 'b')] + #[arg(long, short)] base: String, /// Path to the current (later) .heapsnapshot file - #[arg(long, short = 'c')] + #[arg(long, short)] current: String,🤖 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/lib.rs` around lines 251 - 256, The `base` and `current` fields in this enum use explicit short flags even though they match clap’s auto-derived values. Update the `#[arg(...)]` attributes on these fields to use the same `#[arg(long, short)]` convention used by `InspectHeapSnapshotNode` and the `output` fields so the enum stays consistent.
🤖 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`:
- Line 399: The `diffs.sort_by` ordering in `memory.rs` is only comparing
`size_delta`, so ties can reorder differently across runs because
`all_names.into_iter()` comes from a randomized `HashSet`. Update the sorting in
the summary path to use a deterministic secondary key from the class identity
(for example the class name or another stable field from the `diff` item) so the
row order is reproducible across separate invocations and `--class-index`
matches the displayed summary index.
---
Nitpick comments:
In `@src/lib.rs`:
- Around line 251-256: The `base` and `current` fields in this enum use explicit
short flags even though they match clap’s auto-derived values. Update the
`#[arg(...)]` attributes on these fields to use the same `#[arg(long, short)]`
convention used by `InspectHeapSnapshotNode` and the `output` fields so the enum
stays consistent.
🪄 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: b6d31b44-cc7b-4a10-a3ce-ddb0812f72d6
📒 Files selected for processing (3)
src/commands/executor.rssrc/commands/memory.rssrc/lib.rs
|
/gemini review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new offline command, compare-heapsnapshots, which compares two .heapsnapshot files to report per-class added and deleted nodes without requiring a Chrome connection. It includes helper functions for parsing, aggregating, diffing, and formatting the snapshot data, along with comprehensive unit tests. The reviewer suggested two performance optimizations: first, avoiding redundant string cloning in build_class_aggregates by only cloning when inserting a new class name; second, optimizing diff_snapshots to eliminate an intermediate HashSet allocation and reduce map lookups by iterating over the maps directly.
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.
🧹 Nitpick comments (2)
src/commands/memory.rs (2)
406-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
csv_escapeinformat_node_detailsto avoid divergent escaping.This is a verbatim copy of the escaping logic already inlined in
format_node_details(Lines 226-230). Having two copies invites future drift. Route the existing call through this helper.♻️ Proposed fix
- let escaped_name = if name.contains(',') || name.contains('"') || name.contains('\n') || name.contains('\r') { - format!("\"{}\"", name.replace('"', "\"\"")) - } else { - name.to_string() - }; + let escaped_name = csv_escape(name);🤖 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 406 - 415, `format_node_details` currently duplicates the CSV escaping logic that `csv_escape` already provides, which can lead to drift between the two paths. Update `format_node_details` to call `csv_escape` for class/node name escaping instead of keeping its own inline implementation, and leave `csv_escape` as the single shared helper for this behavior.
355-397: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDetail node ordering is non-deterministic across runs.
added_ids/deleted_idsare populated by iteratingcur.nodes/base_a.nodes, which areHashMaps seeded with a per-process random state. The summary sort is now stable (Line 399), but for a class with multiple added/removed nodes, the--class-indexdetail view rows (format_class_diff_detail) will appear in a different order on each separate invocation. Add a deterministic key so detail output is reproducible.🔧 Proposed fix: sort node lists by id
let added_count = added_ids.len(); let removed_count = deleted_ids.len(); if added_count == 0 && removed_count == 0 { return None; } + // Sort by id so the per-class detail view is reproducible across + // separate `--class-index` invocations (HashMap order is randomized). + let mut added: Vec<(u64, u64)> = added_ids.into_iter().zip(added_self_sizes).collect(); + added.sort_unstable_by_key(|&(id, _)| id); + let (added_ids, added_self_sizes): (Vec<u64>, Vec<u64>) = added.into_iter().unzip(); + let mut deleted: Vec<(u64, u64)> = deleted_ids.into_iter().zip(deleted_self_sizes).collect(); + deleted.sort_unstable_by_key(|&(id, _)| id); + let (deleted_ids, deleted_self_sizes): (Vec<u64>, Vec<u64>) = deleted.into_iter().unzip(); let count_delta = added_count as i64 - removed_count as i64;🤖 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 355 - 397, Make the per-class detail rows deterministic in `memory.rs` by ensuring `HeapSnapshotClassDiff` stores `added_ids` and `deleted_ids` in a stable order before `format_class_diff_detail` renders them. The current loops that populate these vectors from `cur_agg` and `base_agg` iterate `HashMap` entries, so sort the collected node ids (and keep `added_self_sizes` / `deleted_self_sizes` aligned with them) using a consistent key such as the node id or a derived deterministic tuple. This should be done in the aggregation logic that builds `HeapSnapshotClassDiff`, not in the formatter, so `--class-index` output is reproducible across runs.
🤖 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.
Nitpick comments:
In `@src/commands/memory.rs`:
- Around line 406-415: `format_node_details` currently duplicates the CSV
escaping logic that `csv_escape` already provides, which can lead to drift
between the two paths. Update `format_node_details` to call `csv_escape` for
class/node name escaping instead of keeping its own inline implementation, and
leave `csv_escape` as the single shared helper for this behavior.
- Around line 355-397: Make the per-class detail rows deterministic in
`memory.rs` by ensuring `HeapSnapshotClassDiff` stores `added_ids` and
`deleted_ids` in a stable order before `format_class_diff_detail` renders them.
The current loops that populate these vectors from `cur_agg` and `base_agg`
iterate `HashMap` entries, so sort the collected node ids (and keep
`added_self_sizes` / `deleted_self_sizes` aligned with them) using a consistent
key such as the node id or a derived deterministic tuple. This should be done in
the aggregation logic that builds `HeapSnapshotClassDiff`, not in the formatter,
so `--class-index` output is reproducible across runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b406a0b9-24f9-4db0-ba3b-4a8e6cd7310c
📒 Files selected for processing (3)
src/commands/executor.rssrc/commands/memory.rssrc/lib.rs
- Reformatted argument lists and chained method calls for better clarity in `executor.rs`, `memory.rs`, `read_page.rs`, `screenshot.rs`, `third_party.rs`, and `lib.rs`. - Enhanced error handling messages in `lib.rs` for clearer debugging. - Updated tests in `read_page.rs`, `screenshot.rs`, and `lib.rs` for improved readability and consistency in assertions. - Removed unnecessary line breaks and adjusted formatting in various files to adhere to style guidelines.
|
/gemini review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new offline command, compare-heapsnapshots, which compares two local .heapsnapshot files and reports per-class added or deleted nodes without requiring a Chrome connection. It also includes extensive code formatting and style cleanups across several files. The review feedback highlights two important performance and memory optimizations in src/commands/memory.rs: dropping the raw HeapSnapshot ASTs immediately after aggregation to prevent potential Out-Of-Memory (OOM) errors on large snapshots, and pre-allocating vector capacities during the diffing process to minimize reallocations.
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.
🧹 Nitpick comments (1)
src/commands/memory.rs (1)
333-339: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider the
EntryAPI to simplify the insert-or-update branch.The
get_mut/else-insert pattern duplicates theagg.nodes.insert(id, self_size)logic across both branches.entry(...).or_insert_with(...)collapses this to one line without behavior change.♻️ Proposed refactor
- if let Some(agg) = aggregates.get_mut(name_ref) { - agg.nodes.insert(id, self_size); - } else { - let mut agg = ClassAggregate::new(); - agg.nodes.insert(id, self_size); - aggregates.insert(name_ref.clone(), agg); - } + aggregates + .entry(name_ref.clone()) + .or_insert_with(ClassAggregate::new) + .nodes + .insert(id, self_size);🤖 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 333 - 339, The insert-or-update logic in the aggregate handling duplicates the same node insertion in both branches. Refactor the `aggregates` update in `memory.rs` to use the `Entry` API on `name_ref` so `ClassAggregate::new()` is only created when missing and `agg.nodes.insert(id, self_size)` happens in one place. Keep the existing behavior unchanged while simplifying the branch around the `get_mut`/insert path.
🤖 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.
Nitpick comments:
In `@src/commands/memory.rs`:
- Around line 333-339: The insert-or-update logic in the aggregate handling
duplicates the same node insertion in both branches. Refactor the `aggregates`
update in `memory.rs` to use the `Entry` API on `name_ref` so
`ClassAggregate::new()` is only created when missing and `agg.nodes.insert(id,
self_size)` happens in one place. Keep the existing behavior unchanged while
simplifying the branch around the `get_mut`/insert path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 36e8f73a-3cf5-4170-be03-af9aa790539c
📒 Files selected for processing (7)
src/commands/evaluate.rssrc/commands/executor.rssrc/commands/memory.rssrc/commands/read_page.rssrc/commands/screenshot.rssrc/commands/third_party.rssrc/lib.rs
* preallocate added/deleted vectors using upper-bound capacities to avoid reallocations * compute base length once to size deletion buffers efficiently * scope snapshot parsing to drop raw data immediately after building aggregates, lowering peak memory usage during diff
|
/gemini review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/commands/memory.rs (1)
316-342: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
entry()to avoid double hashmap lookups in the hot loop.
get_mutfollowed byinsertperforms two lookups per node;entry()does it in one. This loop runs once per node in the entire snapshot, so for large snapshots this adds up.♻️ Proposed refactor
- if let Some(agg) = aggregates.get_mut(name_ref) { - agg.nodes.insert(id, self_size); - } else { - let mut agg = ClassAggregate::new(); - agg.nodes.insert(id, self_size); - aggregates.insert(name_ref.clone(), agg); - } + aggregates + .entry(name_ref.clone()) + .or_insert_with(ClassAggregate::new) + .nodes + .insert(id, self_size);🤖 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 316 - 342, The node aggregation loop in `src/commands/memory.rs` is doing two HashMap lookups per class name via `aggregates.get_mut()` followed by `insert`; switch this hot path to `HashMap::entry()` in the `while current_idx + node_size <= nodes.len()` loop so each `name_ref` is handled with a single lookup. Update the `ClassAggregate` insertion logic to use the entry API while preserving the current `ClassAggregate::new()` initialization and `agg.nodes.insert(id, self_size)` behavior.
🤖 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.
Nitpick comments:
In `@src/commands/memory.rs`:
- Around line 316-342: The node aggregation loop in `src/commands/memory.rs` is
doing two HashMap lookups per class name via `aggregates.get_mut()` followed by
`insert`; switch this hot path to `HashMap::entry()` in the `while current_idx +
node_size <= nodes.len()` loop so each `name_ref` is handled with a single
lookup. Update the `ClassAggregate` insertion logic to use the entry API while
preserving the current `ClassAggregate::new()` initialization and
`agg.nodes.insert(id, self_size)` behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 90584542-39aa-4394-8831-6c213c013471
📒 Files selected for processing (7)
src/commands/evaluate.rssrc/commands/executor.rssrc/commands/memory.rssrc/commands/read_page.rssrc/commands/screenshot.rssrc/commands/third_party.rssrc/lib.rs
…n termination * add connection timeout for Chrome WebSocket handshake with actionable error messaging * document failure handling and agent guidance in AGENTS.md and SKILL.md * introduce guarded `kill-daemon` command with `--force` flag and TTY-based confirmation/refusal logic * prevent non-interactive agents from killing daemon without explicit force * add unit-testable decision logic for daemon termination behavior * update CLI argument parsing and command handling for new `kill-daemon` behavior
|
/gemini review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new offline command compare-heapsnapshots to diff two .heapsnapshot files and report per-class added/deleted nodes, complete with text/JSON formatting and unit tests. It also implements a connection timeout for the Chrome WebSocket handshake to prevent indefinite hangs, and adds a safety guard to the kill-daemon command to refuse non-interactive execution unless --force is specified. The review feedback suggests a valuable performance optimization in the heap snapshot diffing logic to avoid allocating and sorting detailed node vectors when only the summary view is requested.
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 (1)
src/commands/memory.rs (1)
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant “what” comment.
Line 57 only restates the next CDP call; either drop it or replace it with the reason this enable step must precede streaming. As per coding guidelines,
src/**/*.rs: Rust code 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 57 - 59, The comment before the HeapProfiler enable call is redundant and only restates what `client.send_to_target(session_id, "HeapProfiler.enable", json!({}))` already shows. Remove that “what” comment or replace it with a brief “why” explanation for `send_to_target` in `memory.rs`, focusing on why HeapProfiler must be enabled before starting streaming.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 652-658: The out-of-range classIndex validation in the memory
command is currently using a generic anyhow error, which will surface as an
unspecified CLI code. Update the invalid index path in the class selection logic
to return a typed CliError with ErrorCode::InvalidInput instead of anyhow!, so
the error boundary preserves the stable user-input error code; use the existing
classIndex/diffs lookup in memory command to locate the change.
- Around line 316-343: The flat node parsing in the memory snapshot aggregation
loop currently stops at the last complete record and silently ignores any
trailing partial data. Update the logic around the aggregates-building loop in
the memory command to validate that nodes.len() is evenly divisible by node_size
before iterating, and return an error for malformed snapshots instead of relying
on the current_idx + node_size <= nodes.len() check. Keep the fix localized to
the class aggregation path that uses ClassAggregate and the node offsets so
truncated arrays are rejected early.
In `@src/lib.rs`:
- Around line 874-876: The match on kill-daemon handling is partially moving cli
by destructuring cli.command, which breaks later uses of cli.command and
build_request(&cli) in the same flow. Update the Commands::KillDaemon branch to
borrow cli.command instead of taking ownership, and dereference the force field
inside the pattern so the existing cli value remains available for the later
matches and request-building logic.
---
Nitpick comments:
In `@src/commands/memory.rs`:
- Around line 57-59: The comment before the HeapProfiler enable call is
redundant and only restates what `client.send_to_target(session_id,
"HeapProfiler.enable", json!({}))` already shows. Remove that “what” comment or
replace it with a brief “why” explanation for `send_to_target` in `memory.rs`,
focusing on why HeapProfiler must be enabled before starting streaming.
🪄 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: e4a715b7-0eb2-443f-bd39-be280522faec
📒 Files selected for processing (10)
AGENTS.mdskill/chrome-devtools/SKILL.mdsrc/cdp.rssrc/commands/evaluate.rssrc/commands/executor.rssrc/commands/memory.rssrc/commands/read_page.rssrc/commands/screenshot.rssrc/commands/third_party.rssrc/lib.rs
* enforce nodes array length is multiple of node\_size to detect truncated/malformed snapshots * improve error message for corrupted heap snapshots * replace anyhow error with typed CliError (InvalidInput) for invalid classIndex * update tests to cover truncated nodes case and validate error typing * clarify HeapProfiler.enable requirement before taking heap snapshot
|
/gemini review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Code Review
This pull request introduces an offline compare-heapsnapshots command to diff heap snapshots, implements a connection timeout for the CDP client, and guards the kill-daemon command to prevent non-interactive callers from executing it without --force. It also updates documentation and adds comprehensive unit tests. Feedback on the changes suggests replacing is_multiple_of with the standard modulo operator to prevent MSRV issues, utilizing the more idiomatic entry API to avoid double lookups, and refactoring parallel vectors into a single vector of structs or tuples to simplify data handling.
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.
♻️ Duplicate comments (1)
src/lib.rs (1)
874-876: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winBorrow
cli.commandbefore the kill-daemon decision.Line 874 still moves
cli.command, but the function later uses&cli.commandand&cli. Match by reference and dereferenceforce.Proposed fix
- if let Commands::KillDaemon { force } = cli.command { + if let Commands::KillDaemon { force } = &cli.command { use std::io::IsTerminal; - match kill_daemon_decision(force, std::io::stdin().is_terminal()) { + match kill_daemon_decision(*force, std::io::stdin().is_terminal()) {🤖 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/lib.rs` around lines 874 - 876, The kill-daemon branch in lib.rs is moving cli.command, which conflicts with later uses of &cli.command and &cli in the same function. Update the match on Commands::KillDaemon in the kill_daemon_decision flow to borrow cli.command by reference instead of moving it, and dereference force when passing it onward so the rest of the code can still use cli and cli.command safely.
🤖 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.
Duplicate comments:
In `@src/lib.rs`:
- Around line 874-876: The kill-daemon branch in lib.rs is moving cli.command,
which conflicts with later uses of &cli.command and &cli in the same function.
Update the match on Commands::KillDaemon in the kill_daemon_decision flow to
borrow cli.command by reference instead of moving it, and dereference force when
passing it onward so the rest of the code can still use cli and cli.command
safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2603a71f-dd25-4b8f-9581-6a366f5ccecf
📒 Files selected for processing (10)
AGENTS.mdskill/chrome-devtools/SKILL.mdsrc/cdp.rssrc/commands/evaluate.rssrc/commands/executor.rssrc/commands/memory.rssrc/commands/read_page.rssrc/commands/screenshot.rssrc/commands/third_party.rssrc/lib.rs
…ot diff structure
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an offline compare-heapsnapshots command to diff heap snapshots, adds a timeout to Chrome WebSocket connections to prevent indefinite hangs, and implements a safety guard on kill-daemon to prevent automated agents from killing active connections without --force. Feedback on these changes includes optimizing the heap snapshot parser by avoiding redundant string allocations during node iteration, replacing is_multiple_of with the standard modulo operator % to maintain backward compatibility with older Rust versions, and caching the connection timeout duration in a local variable to avoid duplicate environment variable reads.
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.
…gregates function
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new offline command compare-heapsnapshots to diff two .heapsnapshot files, along with corresponding documentation and tests. It also adds a connection timeout to the Chrome WebSocket handshake to prevent indefinite hangs, and guards the kill-daemon command to prevent automated agents from executing it non-interactively without the --force flag. The review feedback suggests optimizing the diff_snapshots function by passing the base and current aggregates by value rather than by reference, which avoids unnecessary string cloning and simplifies the lookup logic.
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.
…ment passing and reducing unnecessary lookups
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
1 participant
Summary by CodeRabbit
New Features
.heapsnapshotfiles, with a class-diff summary by default and optional per-class output via--class-index(deterministic ordering).Bug Fixes
--force, prompt interactively, and require--forcewhen non-interactive.Documentation
Tests
--class-indexerrors.