Conversation
📝 WalkthroughWalkthroughAdds an offline ChangesCompare Heapsnapshots Feature
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
1 participant
Summary by CodeRabbit
.heapsnapshotfiles, with a class-diff summary by default and optional per-class details via--class-index(including deterministic ordering).class-indexerrors.