From 273e4dfce354fe1ee516c8b1acb65ce66fd557bc Mon Sep 17 00:00:00 2001 From: Aero Date: Sat, 4 Jul 2026 00:38:08 +0800 Subject: [PATCH 01/11] feat: add compare-heapsnapshots command for offline heap snapshot comparison --- src/commands/executor.rs | 1 + src/commands/memory.rs | 476 ++++++++++++++++++++++++++++++++++++++- src/lib.rs | 39 ++++ 3 files changed, 509 insertions(+), 7 deletions(-) diff --git a/src/commands/executor.rs b/src/commands/executor.rs index 7846bc5..d4034fd 100644 --- a/src/commands/executor.rs +++ b/src/commands/executor.rs @@ -55,6 +55,7 @@ pub fn known_args(cmd: &str) -> &'static [&'static str] { "read-page" => &["output"], "take-heapsnapshot" => &["output"], "inspect-heapsnapshot-node" => &["file_path", "node_id"], + "compare-heapsnapshots" => &["base", "current", "class_index"], "emulate" => &[ "viewport", "device_scale_factor", diff --git a/src/commands/memory.rs b/src/commands/memory.rs index e56185d..e1f86da 100644 --- a/src/commands/memory.rs +++ b/src/commands/memory.rs @@ -159,13 +159,7 @@ pub fn parse_node_from_snapshot( file_path: &str, node_id: u64, ) -> Result<(String, u64)> { - use anyhow::Context; - let file = File::open(file_path) - .with_context(|| format!("Failed to open heap snapshot file at: {}", file_path))?; - let reader = BufReader::new(file); - let val: HeapSnapshot = serde_json::from_reader(reader) - .context("Failed to deserialize heap snapshot file. Ensure it is valid JSON.")?; - + let val = parse_snapshot_file(file_path)?; find_node_in_snapshot(&val, node_id) } @@ -268,6 +262,290 @@ pub async fn inspect_heapsnapshot_node_offline( Ok(CommandResult::output(out)) } +/// Per-class aggregate. Tracks id → self_size so the diff can recover exact +/// per-id sizes for added/deleted nodes without re-parsing the file. +struct ClassAggregate { + nodes: std::collections::HashMap, +} + +impl ClassAggregate { + fn new() -> Self { + Self { nodes: std::collections::HashMap::new() } + } +} + +/// Walk every node in the snapshot and group by class name (the `name` +/// field's string). Pure (no I/O) so it can be unit-tested alongside +/// `find_node_in_snapshot`. +fn build_class_aggregates(val: &HeapSnapshot) -> Result> { + use anyhow::Context; + let nodes = &val.nodes; + let node_fields = &val.snapshot.meta.node_fields; + let id_offset = node_fields.iter().position(|f| f == "id") + .context("Invalid snapshot schema: 'id' node field meta is missing")?; + let name_offset = node_fields.iter().position(|f| f == "name") + .context("Invalid snapshot schema: 'name' node field meta is missing")?; + let self_size_offset = node_fields.iter().position(|f| f == "self_size") + .context("Invalid snapshot schema: 'self_size' node field meta is missing")?; + let node_size = node_fields.len(); + if node_size == 0 { + bail!("Invalid snapshot: node_fields schema is empty"); + } + + let mut aggregates: std::collections::HashMap = std::collections::HashMap::new(); + let mut current_idx = 0; + while current_idx + node_size <= nodes.len() { + let id = nodes[current_idx + id_offset]; + let name_str_idx = usize::try_from(nodes[current_idx + name_offset]) + .ok() + .context("Corrupt snapshot: string index overflow on 32-bit architecture")?; + let name = val.strings.get(name_str_idx).cloned() + .ok_or_else(|| anyhow!("Corrupt snapshot: string index {} out of bounds (strings len {})", name_str_idx, val.strings.len()))?; + let self_size = nodes[current_idx + self_size_offset]; + + aggregates.entry(name).or_insert_with(ClassAggregate::new) + .nodes.insert(id, self_size); + + current_idx += node_size; + } + Ok(aggregates) +} + +/// One row of the summary diff. Mirrors the MCP `HeapSnapshotClassDiff` shape +/// so output stays familiar to anyone moving between the two tools. +#[derive(Debug, Clone)] +pub struct HeapSnapshotClassDiff { + pub class_name: String, + pub added_count: usize, + pub removed_count: usize, + pub count_delta: i64, + pub added_size: u64, + pub removed_size: u64, + pub size_delta: i64, + // Per-id detail. Exposed only via the `--class-index` path. + pub added_ids: Vec, + pub added_self_sizes: Vec, + pub deleted_ids: Vec, + pub deleted_self_sizes: Vec, +} + +/// Compute the per-class diff between two snapshots. Returns rows filtered to +/// classes with any change (addedCount > 0 OR removedCount > 0) and sorted by +/// sizeDelta descending — matching DevTools' `#getSortedRawClassDiffs` so the +/// summary list and the `--class-index` detail view share stable indices. +fn diff_snapshots( + base: &std::collections::HashMap, + current: &std::collections::HashMap, +) -> Vec { + let mut all_names: std::collections::HashSet<&str> = std::collections::HashSet::new(); + all_names.extend(base.keys().map(String::as_str)); + all_names.extend(current.keys().map(String::as_str)); + + let mut diffs: Vec = all_names.into_iter().filter_map(|name| { + let base_agg = base.get(name); + let cur_agg = current.get(name); + + let mut added_ids: Vec = Vec::new(); + let mut added_self_sizes: Vec = Vec::new(); + let mut deleted_ids: Vec = Vec::new(); + let mut deleted_self_sizes: Vec = Vec::new(); + let mut added_size: u64 = 0; + let mut removed_size: u64 = 0; + + if let Some(cur) = cur_agg { + for (id, size) in &cur.nodes { + let in_base = base_agg.map_or(false, |b| b.nodes.contains_key(id)); + if !in_base { + added_ids.push(*id); + added_self_sizes.push(*size); + added_size += size; + } + } + } + if let Some(base_a) = base_agg { + for (id, size) in &base_a.nodes { + let in_cur = cur_agg.map_or(false, |c| c.nodes.contains_key(id)); + if !in_cur { + deleted_ids.push(*id); + deleted_self_sizes.push(*size); + removed_size += size; + } + } + } + + let added_count = added_ids.len(); + let removed_count = deleted_ids.len(); + if added_count == 0 && removed_count == 0 { + return None; + } + let count_delta = added_count as i64 - removed_count as i64; + let size_delta = added_size as i64 - removed_size as i64; + + Some(HeapSnapshotClassDiff { + class_name: name.to_string(), + added_count, + removed_count, + count_delta, + added_size, + removed_size, + size_delta, + added_ids, + added_self_sizes, + deleted_ids, + deleted_self_sizes, + }) + }).collect(); + + diffs.sort_by(|a, b| b.size_delta.cmp(&a.size_delta)); + diffs +} + +/// CSV-escape a class name the same way `format_node_details` escapes node +/// names — names like `(closure)` are safe, but `(string, joined)` would break +/// naive CSV parsing. +fn csv_escape(s: &str) -> String { + if s.contains(',') || s.contains('"') || s.contains('\n') || s.contains('\r') { + format!("\"{}\"", s.replace('"', "\"\"")) + } else { + s.to_string() + } +} + +/// Format the summary diff table (one row per changed class). +pub fn format_class_diff_summary( + diffs: &[HeapSnapshotClassDiff], + format: crate::format::OutputFormat, +) -> Result { + if format.is_text() { + let mut out = String::new(); + out.push_str("idx,className,addedCount,removedCount,countDelta,addedSize,removedSize,sizeDelta\n"); + for (i, d) in diffs.iter().enumerate() { + out.push_str(&format!( + "{},{},{},{},{},{},{},{}\n", + i, + csv_escape(&d.class_name), + d.added_count, + d.removed_count, + d.count_delta, + d.added_size, + d.removed_size, + d.size_delta, + )); + } + Ok(out) + } else { + let rows: Vec = diffs.iter().enumerate().map(|(i, d)| { + json!({ + "idx": i, + "className": d.class_name, + "addedCount": d.added_count, + "removedCount": d.removed_count, + "countDelta": d.count_delta, + "addedSize": d.added_size, + "removedSize": d.removed_size, + "sizeDelta": d.size_delta, + }) + }).collect(); + Ok(crate::format::format_structured(&json!({ "diffs": rows }), format)?) + } +} + +/// Format the per-class detail (added/deleted node IDs + sizes). Mirrors the +/// summary's `idx` so a user can copy the index straight from summary → detail. +pub fn format_class_diff_detail( + idx: usize, + diff: &HeapSnapshotClassDiff, + format: crate::format::OutputFormat, +) -> Result { + if format.is_text() { + let mut out = String::new(); + out.push_str(&format!( + "idx:{},className:{},addedCount:{},removedCount:{},countDelta:{},addedSize:{},removedSize:{},sizeDelta:{}\n", + idx, + csv_escape(&diff.class_name), + diff.added_count, + diff.removed_count, + diff.count_delta, + diff.added_size, + diff.removed_size, + diff.size_delta, + )); + out.push_str("\nop,nodeId,selfSize\n"); + for (id, size) in diff.added_ids.iter().zip(diff.added_self_sizes.iter()) { + out.push_str(&format!("+,{},{}\n", id, size)); + } + for (id, size) in diff.deleted_ids.iter().zip(diff.deleted_self_sizes.iter()) { + out.push_str(&format!("-,{},{}\n", id, size)); + } + Ok(out) + } else { + let added: Vec = diff.added_ids.iter().zip(diff.added_self_sizes.iter()) + .map(|(id, size)| json!({ "op": "+", "nodeId": id, "selfSize": size })) + .collect(); + let deleted: Vec = diff.deleted_ids.iter().zip(diff.deleted_self_sizes.iter()) + .map(|(id, size)| json!({ "op": "-", "nodeId": id, "selfSize": size })) + .collect(); + let mut nodes: Vec = added; + nodes.extend(deleted); + let detail = json!({ + "idx": idx, + "className": diff.class_name, + "addedCount": diff.added_count, + "removedCount": diff.removed_count, + "countDelta": diff.count_delta, + "addedSize": diff.added_size, + "removedSize": diff.removed_size, + "sizeDelta": diff.size_delta, + "nodes": nodes, + }); + Ok(crate::format::format_structured(&detail, format)?) + } +} + +/// Offline implementation of `compare-heapsnapshots`. Parses both files, +/// diffs, and renders summary or per-class detail depending on `class_index`. +pub async fn compare_heapsnapshots_offline( + base_path: &str, + current_path: &str, + class_index: Option, + format: crate::format::OutputFormat, +) -> Result { + let base_owned = base_path.to_string(); + let current_owned = current_path.to_string(); + let diffs = tokio::task::spawn_blocking(move || -> Result> { + let base_val = parse_snapshot_file(&base_owned)?; + let current_val = parse_snapshot_file(¤t_owned)?; + let base_agg = build_class_aggregates(&base_val)?; + let current_agg = build_class_aggregates(¤t_val)?; + Ok(diff_snapshots(&base_agg, ¤t_agg)) + }) + .await + .map_err(|e| anyhow!("Failed to execute blocking snapshot diff: {e}"))??; + + let out = match class_index { + None => format_class_diff_summary(&diffs, format)?, + Some(idx) => { + let diff = diffs.get(idx).ok_or_else(|| anyhow!( + "Invalid classIndex: {}. Total classes with changes: {}", + idx, diffs.len() + ))?; + format_class_diff_detail(idx, diff, format)? + } + }; + Ok(CommandResult::output(out)) +} + +/// Read + deserialize a .heapsnapshot file. Shared by the diff path so both +/// base and current snapshots parse identically. +fn parse_snapshot_file(file_path: &str) -> Result { + use anyhow::Context; + let file = File::open(file_path) + .with_context(|| format!("Failed to open heap snapshot file at: {}", file_path))?; + let reader = BufReader::new(file); + serde_json::from_reader(reader) + .context("Failed to deserialize heap snapshot file. Ensure it is valid JSON.") +} + #[cfg(test)] mod tests { use super::*; @@ -381,4 +659,188 @@ mod tests { assert!(out_toon.contains("nodeId")); assert!(out_toon.contains("ClassA")); } + + /// Build a synthetic HeapSnapshot from a list of (id, name, self_size) + /// triples. Keeps diff tests readable — node_fields order is fixed at the + /// schema the production parser actually sees. + fn make_snapshot(nodes: &[(u64, &str, u64)]) -> HeapSnapshot { + let mut flat: Vec = Vec::with_capacity(nodes.len() * 3); + let mut strings: Vec = Vec::new(); + let mut string_idx: std::collections::HashMap<&str, u64> = std::collections::HashMap::new(); + for (id, name, size) in nodes { + let &mut idx = string_idx.entry(name).or_insert_with(|| { + let i = strings.len() as u64; + strings.push(name.to_string()); + i + }); + flat.extend_from_slice(&[*id, idx, *size]); + } + HeapSnapshot { + snapshot: SnapshotMeta { + meta: MetaDetails { + node_fields: vec!["id".into(), "name".into(), "self_size".into()], + }, + }, + nodes: flat, + strings, + } + } + + #[test] + fn test_build_class_aggregates_groups_by_name() { + let snap = make_snapshot(&[(1, "Map", 100), (2, "Map", 200), (3, "String", 50)]); + let aggs = build_class_aggregates(&snap).unwrap(); + let map = aggs.get("Map").unwrap(); + assert_eq!(map.nodes.len(), 2); + assert_eq!(map.nodes.get(&1), Some(&100)); + assert_eq!(map.nodes.get(&2), Some(&200)); + let s = aggs.get("String").unwrap(); + assert_eq!(s.nodes.get(&3), Some(&50)); + } + + #[test] + fn test_diff_added_removed_and_retained() { + // Base: Map{1@100, 2@100}, String{3@50} + // Current: Map{1@100 (retained), 4@150 (new)}, Window{5@500 (new class)} + let base = build_class_aggregates(&make_snapshot(&[ + (1, "Map", 100), (2, "Map", 100), (3, "String", 50), + ])).unwrap(); + let current = build_class_aggregates(&make_snapshot(&[ + (1, "Map", 100), (4, "Map", 150), (5, "Window", 500), + ])).unwrap(); + + let diffs = diff_snapshots(&base, ¤t); + + // Sorted by sizeDelta desc: Window(500) > Map(50) > String(-50) + assert_eq!(diffs.len(), 3); + assert_eq!(diffs[0].class_name, "Window"); + assert_eq!(diffs[0].added_count, 1); + assert_eq!(diffs[0].removed_count, 0); + assert_eq!(diffs[0].added_size, 500); + assert_eq!(diffs[0].removed_size, 0); + assert_eq!(diffs[0].size_delta, 500); + assert_eq!(diffs[0].added_ids, vec![5]); + + assert_eq!(diffs[1].class_name, "Map"); + assert_eq!(diffs[1].added_count, 1); + assert_eq!(diffs[1].removed_count, 1); + assert_eq!(diffs[1].count_delta, 0); + assert_eq!(diffs[1].added_size, 150); + assert_eq!(diffs[1].removed_size, 100); + assert_eq!(diffs[1].size_delta, 50); + assert_eq!(diffs[1].added_ids, vec![4]); + assert_eq!(diffs[1].deleted_ids, vec![2]); + + assert_eq!(diffs[2].class_name, "String"); + assert_eq!(diffs[2].added_count, 0); + assert_eq!(diffs[2].removed_count, 1); + assert_eq!(diffs[2].count_delta, -1); + assert_eq!(diffs[2].size_delta, -50); + assert_eq!(diffs[2].deleted_ids, vec![3]); + } + + #[test] + fn test_diff_filters_unchanged_classes() { + // Map appears in both with identical nodes → no diff row at all. + let base = build_class_aggregates(&make_snapshot(&[(1, "Map", 100)])).unwrap(); + let current = build_class_aggregates(&make_snapshot(&[(1, "Map", 100)])).unwrap(); + let diffs = diff_snapshots(&base, ¤t); + assert!(diffs.is_empty()); + } + + #[test] + fn test_diff_class_gone_entirely_from_current() { + let base = build_class_aggregates(&make_snapshot(&[ + (1, "Old", 80), (2, "Old", 40), + ])).unwrap(); + let current = build_class_aggregates(&make_snapshot(&[])).unwrap(); + let diffs = diff_snapshots(&base, ¤t); + assert_eq!(diffs.len(), 1); + assert_eq!(diffs[0].class_name, "Old"); + assert_eq!(diffs[0].removed_count, 2); + assert_eq!(diffs[0].removed_size, 120); + assert_eq!(diffs[0].size_delta, -120); + assert_eq!(diffs[0].deleted_ids.len(), 2); + } + + #[test] + fn test_format_summary_and_detail_share_indices() { + let base = build_class_aggregates(&make_snapshot(&[ + (1, "Map", 100), (2, "String", 50), + ])).unwrap(); + let current = build_class_aggregates(&make_snapshot(&[ + (3, "Window", 500), (4, "Map", 150), + ])).unwrap(); + let diffs = diff_snapshots(&base, ¤t); + + let summary = format_class_diff_summary(&diffs, crate::format::OutputFormat::Text).unwrap(); + // First data row (idx 0) should be the largest sizeDelta — Window. + assert!(summary.starts_with("idx,className,addedCount,removedCount,countDelta,addedSize,removedSize,sizeDelta\n")); + assert!(summary.contains("0,Window,1,0,1,500,0,500\n")); + + // Detail for idx 0 must reference the same class. + let detail = format_class_diff_detail(0, &diffs[0], crate::format::OutputFormat::Text).unwrap(); + assert!(detail.contains("className:Window")); + assert!(detail.contains("+,3,500")); + // Map's id 4 should NOT appear here — it's a different class. + assert!(!detail.contains("4,150")); + } + + #[test] + fn test_compare_offline_end_to_end_via_files() { + use crate::format::OutputFormat; + use std::io::Write; + use tempfile::NamedTempFile; + + let base_json = json!({ + "snapshot": { "meta": { "node_fields": ["id","name","self_size"] } }, + "nodes": [1, 0, 100, 2, 1, 50], + "strings": ["Map", "String"], + }); + let cur_json = json!({ + "snapshot": { "meta": { "node_fields": ["id","name","self_size"] } }, + "nodes": [3, 0, 500, 4, 1, 150], + "strings": ["Window", "Map"], + }); + + let mut base_file = NamedTempFile::new().unwrap(); + write!(base_file, "{}", base_json).unwrap(); + let mut cur_file = NamedTempFile::new().unwrap(); + write!(cur_file, "{}", cur_json).unwrap(); + + let rt = tokio::runtime::Runtime::new().unwrap(); + let summary = rt.block_on(compare_heapsnapshots_offline( + base_file.path().to_str().unwrap(), + cur_file.path().to_str().unwrap(), + None, + OutputFormat::Text, + )).unwrap(); + let summary_out = summary.output; + assert!(summary_out.contains("0,Window")); + assert!(summary_out.contains("1,Map")); + + // Detail for idx 0 should print Window's node 3. + let detail = rt.block_on(compare_heapsnapshots_offline( + base_file.path().to_str().unwrap(), + cur_file.path().to_str().unwrap(), + Some(0), + OutputFormat::Text, + )).unwrap(); + assert!(detail.output.contains("className:Window")); + assert!(detail.output.contains("+,3,500")); + + // Out-of-range index should error with a clear message. + let err = rt.block_on(compare_heapsnapshots_offline( + base_file.path().to_str().unwrap(), + cur_file.path().to_str().unwrap(), + Some(99), + OutputFormat::Text, + )); + let msg = match err { + Ok(_) => panic!("expected error for out-of-range class_index"), + Err(e) => e.to_string(), + }; + assert!(msg.contains("Invalid classIndex"), "got: {msg}"); + assert!(msg.contains("99")); + } } diff --git a/src/lib.rs b/src/lib.rs index 69eb08c..92433a0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -240,6 +240,27 @@ pub enum Commands { node_id: u64, }, + /// Compare two .heapsnapshot files and report per-class added/deleted nodes. + /// Offline: parses local files, no Chrome connection required. Without + /// --class-index, prints a summary table sorted by size delta. With + /// --class-index N, prints the added/deleted node IDs for row N (use the + /// idx column from the summary). Node IDs in the detail output are + /// directly usable with `inspect-heapsnapshot-node --node-id`. + #[command(name = "compare-heapsnapshots")] + CompareHeapsnapshots { + /// Path to the base (earlier) .heapsnapshot file + #[arg(long, short = 'b')] + base: String, + /// Path to the current (later) .heapsnapshot file + #[arg(long, short = 'c')] + current: String, + /// Optional 0-based row index from the summary output. When omitted, + /// prints the summary table; when present, prints per-id detail for + /// that class. + #[arg(long)] + class_index: Option, + }, + /// Manage page emulation (viewport, geolocation, etc.) Emulate { /// Set viewport size as WxH (e.g. 1280x720) @@ -420,6 +441,7 @@ impl Cli { Commands::ReadPage { .. } => "read-page", Commands::TakeHeapSnapshot { .. } => "take-heapsnapshot", Commands::InspectHeapSnapshotNode { .. } => "inspect-heapsnapshot-node", + Commands::CompareHeapsnapshots { .. } => "compare-heapsnapshots", Commands::Emulate { .. } => "emulate", Commands::WaitFor { .. } => "wait-for", Commands::List3pTools => "list-3p-tools", @@ -720,6 +742,9 @@ fn build_request(cli: &Cli) -> Result { Commands::InspectHeapSnapshotNode { .. } => { unreachable!("InspectHeapSnapshotNode is handled before build_request") } + Commands::CompareHeapsnapshots { .. } => { + unreachable!("CompareHeapsnapshots is handled before build_request") + } }; Ok(DaemonRequest { @@ -899,6 +924,20 @@ pub async fn run() -> Result<()> { return Ok(()); } + // Handle compare-heapsnapshots offline — parses two local .heapsnapshot + // files and diffs them. No Chrome connection or daemon required. + if let Commands::CompareHeapsnapshots { base, current, class_index } = &cli.command { + let result = commands::memory::compare_heapsnapshots_offline( + base, + current, + *class_index, + cli.output_format(), + ) + .await?; + print_result(&result); + return Ok(()); + } + let ws_url = browser::resolve_ws_url( cli.ws_endpoint.as_deref(), cli.user_data_dir.as_deref(), From 612a30377e58e5387f953ba728aefe44016baadc Mon Sep 17 00:00:00 2001 From: Aero Date: Sat, 4 Jul 2026 21:25:56 +0800 Subject: [PATCH 02/11] fix: improve sorting of snapshot diffs by class name as a secondary criterion --- src/commands/memory.rs | 5 ++++- src/lib.rs | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/commands/memory.rs b/src/commands/memory.rs index e1f86da..91f4db7 100644 --- a/src/commands/memory.rs +++ b/src/commands/memory.rs @@ -396,7 +396,10 @@ fn diff_snapshots( }) }).collect(); - diffs.sort_by(|a, b| b.size_delta.cmp(&a.size_delta)); + diffs.sort_by(|a, b| { + b.size_delta.cmp(&a.size_delta) + .then_with(|| a.class_name.cmp(&b.class_name)) + }); diffs } diff --git a/src/lib.rs b/src/lib.rs index 92433a0..b2bc613 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -249,10 +249,10 @@ pub enum Commands { #[command(name = "compare-heapsnapshots")] CompareHeapsnapshots { /// 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, /// Optional 0-based row index from the summary output. When omitted, /// prints the summary table; when present, prints per-id detail for From 9f5098e017b2e9a5d03f4994318ae6306346a6d3 Mon Sep 17 00:00:00 2001 From: Aero Date: Sun, 5 Jul 2026 12:41:55 +0800 Subject: [PATCH 03/11] refactor: improve readability and consistency - 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. --- src/commands/evaluate.rs | 98 +++++++--- src/commands/executor.rs | 27 ++- src/commands/memory.rs | 378 ++++++++++++++++++++++++------------ src/commands/read_page.rs | 27 +-- src/commands/screenshot.rs | 42 +++- src/commands/third_party.rs | 4 +- src/lib.rs | 57 ++++-- 7 files changed, 438 insertions(+), 195 deletions(-) diff --git a/src/commands/evaluate.rs b/src/commands/evaluate.rs index d8ce78c..57ae555 100644 --- a/src/commands/evaluate.rs +++ b/src/commands/evaluate.rs @@ -248,7 +248,9 @@ pub async fn run_script( } } - let nav_url = if interpolated_url.starts_with("http://") || interpolated_url.starts_with("https://") { + let nav_url = if interpolated_url.starts_with("http://") + || interpolated_url.starts_with("https://") + { interpolated_url.clone() } else if is_local_host(&interpolated_url) { format!("http://{}", interpolated_url) @@ -258,7 +260,10 @@ pub async fn run_script( let current_url = client.current_url(session_id).await?; if current_url.trim_end_matches('/') != nav_url.trim_end_matches('/') { - eprintln!("[script] Current URL '{}' does not match target URL '{}'. Auto-navigating...", current_url, nav_url); + eprintln!( + "[script] Current URL '{}' does not match target URL '{}'. Auto-navigating...", + current_url, nav_url + ); crate::commands::navigate::navigate( client, @@ -327,7 +332,10 @@ fn parse_adapter_domains(content: &str) -> Vec { if trimmed.starts_with("import ") || trimmed.starts_with("import(") { continue; } - if matches!(trimmed, "\"use strict\";" | "'use strict';" | "\"use strict\"" | "'use strict'") { + if matches!( + trimmed, + "\"use strict\";" | "'use strict';" | "\"use strict\"" | "'use strict'" + ) { continue; } break; @@ -516,7 +524,9 @@ pub async fn run_adapter( let domains = parse_adapter_domains(&script_content); if !domains.is_empty() { let current_url = client.current_url(session_id).await?; - let matched = domains.iter().any(|domain| url_matches_domain(¤t_url, domain)); + let matched = domains + .iter() + .any(|domain| url_matches_domain(¤t_url, domain)); if !matched { let target_domain = &domains[0]; @@ -524,18 +534,19 @@ pub async fn run_adapter( // scheme when one is missing. Forcing a `www.` subdomain breaks apex // hosts and adapters that target an existing subdomain // (e.g. `creator.xiaohongshu.com`). - let target_url = if target_domain.starts_with("http://") || target_domain.starts_with("https://") { - // An explicit scheme always wins, so authors can force http/https - // by writing it in `@domain` (e.g. `@domain http://localhost:3000`). - target_domain.clone() - } else if is_local_host(target_domain) { - // Local dev servers generally speak http, not https. - format!("http://{}", target_domain) - } else { - format!("https://{}", target_domain) - }; + let target_url = + if target_domain.starts_with("http://") || target_domain.starts_with("https://") { + // An explicit scheme always wins, so authors can force http/https + // by writing it in `@domain` (e.g. `@domain http://localhost:3000`). + target_domain.clone() + } else if is_local_host(target_domain) { + // Local dev servers generally speak http, not https. + format!("http://{}", target_domain) + } else { + format!("https://{}", target_domain) + }; eprintln!("[adapter] Current URL '{}' does not match adapter domains {:?}. Auto-navigating to '{}'...", current_url, domains, target_url); - + crate::commands::navigate::navigate( client, session_id, @@ -549,7 +560,9 @@ pub async fn run_adapter( .await?; let post_nav_url = client.current_url(session_id).await?; - let post_matched = domains.iter().any(|domain| url_matches_domain(&post_nav_url, domain)); + let post_matched = domains + .iter() + .any(|domain| url_matches_domain(&post_nav_url, domain)); if !post_matched { anyhow::bail!( "Auto-navigation to '{}' resulted in URL '{}' which does not match adapter domains {:?}", @@ -720,7 +733,10 @@ mod tests { // (only declarations and bare re-exports are handled). let src = "export * from './x';\nexport const ok = 1;\nexport constants = 2;"; let out = strip_export_keywords(src); - assert_eq!(out, "export * from './x';\nconst ok = 1;\nexport constants = 2;"); + assert_eq!( + out, + "export * from './x';\nconst ok = 1;\nexport constants = 2;" + ); } #[test] @@ -748,9 +764,15 @@ mod tests { #[test] fn test_normalize_host() { - assert_eq!(normalize_host("http://user:pass@example.com/some/path"), "example.com"); + assert_eq!( + normalize_host("http://user:pass@example.com/some/path"), + "example.com" + ); assert_eq!(normalize_host("http://user:pass@[::1]:8080"), "[::1]"); - assert_eq!(normalize_host("http://user:pass@127.0.0.1:8080"), "127.0.0.1"); + assert_eq!( + normalize_host("http://user:pass@127.0.0.1:8080"), + "127.0.0.1" + ); assert_eq!(normalize_host("https://foo:bar@localhost"), "localhost"); assert_eq!(normalize_host("example.com"), "example.com"); assert_eq!(normalize_host("http://example.com:3000/"), "example.com"); @@ -772,9 +794,18 @@ mod tests { #[test] fn test_url_matches_domain() { - assert!(url_matches_domain("https://www.xiaohongshu.com/explore", "xiaohongshu.com")); - assert!(url_matches_domain("http://creator.xiaohongshu.com", "creator.xiaohongshu.com")); - assert!(url_matches_domain("https://xiaohongshu.com:8080/path", "xiaohongshu.com")); + assert!(url_matches_domain( + "https://www.xiaohongshu.com/explore", + "xiaohongshu.com" + )); + assert!(url_matches_domain( + "http://creator.xiaohongshu.com", + "creator.xiaohongshu.com" + )); + assert!(url_matches_domain( + "https://xiaohongshu.com:8080/path", + "xiaohongshu.com" + )); assert!(url_matches_domain("http://[::1]:3000", "[::1]")); assert!(!url_matches_domain("https://google.com", "xiaohongshu.com")); } @@ -782,9 +813,18 @@ mod tests { #[test] fn test_url_matches_domain_normalizes_domain() { // `@domain` written with a scheme and/or path still matches the host. - assert!(url_matches_domain("https://www.example.com/page", "https://example.com")); - assert!(url_matches_domain("https://example.com/explore", "example.com/path")); - assert!(url_matches_domain("https://example.com", "http://example.com:443/")); + assert!(url_matches_domain( + "https://www.example.com/page", + "https://example.com" + )); + assert!(url_matches_domain( + "https://example.com/explore", + "example.com/path" + )); + assert!(url_matches_domain( + "https://example.com", + "http://example.com:443/" + )); assert!(!url_matches_domain("https://example.com", "")); } @@ -800,7 +840,13 @@ mod tests { let ctx = build_ctx_object(r#"{"query":"hi"}"#); assert!(ctx.starts_with("const ctx = {")); assert!(ctx.contains(r#"args: {"query":"hi"}"#)); - for helper in ["wait:", "waitForText:", "waitForSelector:", "click:", "fill:"] { + for helper in [ + "wait:", + "waitForText:", + "waitForSelector:", + "click:", + "fill:", + ] { assert!(ctx.contains(helper), "missing helper: {helper}"); } // fill must special-case checkable inputs instead of setting `value`. diff --git a/src/commands/executor.rs b/src/commands/executor.rs index d4034fd..2c5460a 100644 --- a/src/commands/executor.rs +++ b/src/commands/executor.rs @@ -73,8 +73,21 @@ pub fn known_args(cmd: &str) -> &'static [&'static str] { "console" => &["duration", "type"], "network" => &["duration", "type"], "sw-logs" => &["duration", "extension_id"], - "run-script" => &["file_path", "script_args", "raw_args", "output", "track_navigation"], - "adapter" => &["file_path", "function_name", "script_args", "raw_args", "output", "track_navigation"], + "run-script" => &[ + "file_path", + "script_args", + "raw_args", + "output", + "track_navigation", + ], + "adapter" => &[ + "file_path", + "function_name", + "script_args", + "raw_args", + "output", + "track_navigation", + ], "kill-daemon" => &[], _ => &[], } @@ -326,7 +339,10 @@ fn script_exec_args(args: &serde_json::Value) -> Result> { .get("file_path") .and_then(|v| v.as_str()) .ok_or_else(|| anyhow!("file_path required"))?; - let script_args = args.get("script_args").cloned().unwrap_or_else(|| json!({})); + let script_args = args + .get("script_args") + .cloned() + .unwrap_or_else(|| json!({})); let output = args.get("output").and_then(|v| v.as_str()); let track_navigation = args .get("track_navigation") @@ -413,7 +429,10 @@ async fn inner_execute( client, session_id, commands::screenshot::ScreenshotOptions { - output: args.get("output").and_then(|v| v.as_str()).map(String::from), + output: args + .get("output") + .and_then(|v| v.as_str()) + .map(String::from), format: args .get("format") .and_then(|v| v.as_str()) diff --git a/src/commands/memory.rs b/src/commands/memory.rs index 91f4db7..b02e2dc 100644 --- a/src/commands/memory.rs +++ b/src/commands/memory.rs @@ -22,7 +22,10 @@ pub async fn take_heapsnapshot( // can't collide, and rename is atomic (same filesystem). let temp_path = output_path.with_file_name(format!( ".{}.{}.tmp", - output_path.file_name().unwrap_or_default().to_string_lossy(), + output_path + .file_name() + .unwrap_or_default() + .to_string_lossy(), std::process::id(), )); // Drop guard ensures the temp file is removed under all termination paths @@ -43,13 +46,17 @@ pub async fn take_heapsnapshot( // Heap snapshots can be tens or hundreds of MB; buffer the writes to avoid a // syscall per streamed chunk. let mut file = tokio::io::BufWriter::new( - tokio::fs::File::create(&temp_path) - .await - .with_context(|| format!("Failed to create heap snapshot temp file: {}", temp_path.display()))?, + tokio::fs::File::create(&temp_path).await.with_context(|| { + format!( + "Failed to create heap snapshot temp file: {}", + temp_path.display() + ) + })?, ); - + // First, let's enable the HeapProfiler. - client.send_to_target(session_id, "HeapProfiler.enable", json!({})) + client + .send_to_target(session_id, "HeapProfiler.enable", json!({})) .await .context("Failed to enable HeapProfiler via CDP")?; @@ -70,7 +77,7 @@ pub async fn take_heapsnapshot( .context("Failed to read WebSocket stream message during heap snapshot chunk collection")?; let event: serde_json::Value = serde_json::from_str(&text) .context("Failed to parse WebSocket text frame into JSON event")?; - + // Check if this is the completion response for our takeHeapSnapshot command if event.get("id").and_then(|v| v.as_u64()) == Some(msg_id) { if let Some(error) = event.get("error") { @@ -105,7 +112,9 @@ pub async fn take_heapsnapshot( } .await; - let _ = client.send_to_target(session_id, "HeapProfiler.disable", json!({})).await; + let _ = client + .send_to_target(session_id, "HeapProfiler.disable", json!({})) + .await; if let Err(e) = snapshot_result { return Err(e); @@ -132,7 +141,9 @@ pub async fn take_heapsnapshot( "output": output, "message": format!("Heap snapshot successfully saved to {}", output) }); - Ok(CommandResult::output(crate::format::format_structured(&details, format)?)) + Ok(CommandResult::output(crate::format::format_structured( + &details, format, + )?)) } } @@ -155,10 +166,7 @@ struct HeapSnapshot { /// Parse the JSON heap snapshot and locate details for the given node ID. /// Returns a tuple of (node_name, self_size). -pub fn parse_node_from_snapshot( - file_path: &str, - node_id: u64, -) -> Result<(String, u64)> { +pub fn parse_node_from_snapshot(file_path: &str, node_id: u64) -> Result<(String, u64)> { let val = parse_snapshot_file(file_path)?; find_node_in_snapshot(&val, node_id) } @@ -171,11 +179,17 @@ fn find_node_in_snapshot(val: &HeapSnapshot, node_id: u64) -> Result<(String, u6 let node_fields = &val.snapshot.meta.node_fields; // Find fields offsets within the flat nodes array - let id_offset = node_fields.iter().position(|f| f == "id") + let id_offset = node_fields + .iter() + .position(|f| f == "id") .context("Invalid snapshot schema: 'id' node field meta is missing")?; - let name_offset = node_fields.iter().position(|f| f == "name") + let name_offset = node_fields + .iter() + .position(|f| f == "name") .context("Invalid snapshot schema: 'name' node field meta is missing")?; - let self_size_offset = node_fields.iter().position(|f| f == "self_size") + let self_size_offset = node_fields + .iter() + .position(|f| f == "self_size") .context("Invalid snapshot schema: 'self_size' node field meta is missing")?; let node_size = node_fields.len(); if node_size == 0 { @@ -206,8 +220,13 @@ fn find_node_in_snapshot(val: &HeapSnapshot, node_id: u64) -> Result<(String, u6 let name_str_idx = usize::try_from(nodes[target_node_index + name_offset]) .ok() .context("Corrupt snapshot: string index overflow on 32-bit architecture")?; - let name = val.strings.get(name_str_idx).cloned() - .ok_or_else(|| anyhow!("Corrupt snapshot: string index {} out of bounds (strings len {})", name_str_idx, val.strings.len()))?; + let name = val.strings.get(name_str_idx).cloned().ok_or_else(|| { + anyhow!( + "Corrupt snapshot: string index {} out of bounds (strings len {})", + name_str_idx, + val.strings.len() + ) + })?; let self_size = nodes[target_node_index + self_size_offset]; Ok((name, self_size)) @@ -223,15 +242,8 @@ pub fn format_node_details( if format.is_text() { let mut out = String::new(); out.push_str("nodeId,nodeName,selfSize\n"); - let escaped_name = if name.contains(',') || name.contains('"') || name.contains('\n') || name.contains('\r') { - format!("\"{}\"", name.replace('"', "\"\"")) - } else { - name.to_string() - }; - out.push_str(&format!( - "{},{},{}\n", - node_id, escaped_name, self_size - )); + let escaped_name = csv_escape(name); + out.push_str(&format!("{},{},{}\n", node_id, escaped_name, self_size)); Ok(out) } else { let details = json!({ @@ -252,11 +264,10 @@ pub async fn inspect_heapsnapshot_node_offline( format: crate::format::OutputFormat, ) -> Result { let file_path_owned = file_path.to_string(); - let (name, self_size) = tokio::task::spawn_blocking(move || { - parse_node_from_snapshot(&file_path_owned, node_id) - }) - .await - .map_err(|e| anyhow!("Failed to execute blocking snapshot parser: {e}"))??; + let (name, self_size) = + tokio::task::spawn_blocking(move || parse_node_from_snapshot(&file_path_owned, node_id)) + .await + .map_err(|e| anyhow!("Failed to execute blocking snapshot parser: {e}"))??; let out = format_node_details(node_id, &name, self_size, format)?; Ok(CommandResult::output(out)) @@ -270,41 +281,62 @@ struct ClassAggregate { impl ClassAggregate { fn new() -> Self { - Self { nodes: std::collections::HashMap::new() } + Self { + nodes: std::collections::HashMap::new(), + } } } /// Walk every node in the snapshot and group by class name (the `name` /// field's string). Pure (no I/O) so it can be unit-tested alongside /// `find_node_in_snapshot`. -fn build_class_aggregates(val: &HeapSnapshot) -> Result> { +fn build_class_aggregates( + val: &HeapSnapshot, +) -> Result> { use anyhow::Context; let nodes = &val.nodes; let node_fields = &val.snapshot.meta.node_fields; - let id_offset = node_fields.iter().position(|f| f == "id") + let id_offset = node_fields + .iter() + .position(|f| f == "id") .context("Invalid snapshot schema: 'id' node field meta is missing")?; - let name_offset = node_fields.iter().position(|f| f == "name") + let name_offset = node_fields + .iter() + .position(|f| f == "name") .context("Invalid snapshot schema: 'name' node field meta is missing")?; - let self_size_offset = node_fields.iter().position(|f| f == "self_size") + let self_size_offset = node_fields + .iter() + .position(|f| f == "self_size") .context("Invalid snapshot schema: 'self_size' node field meta is missing")?; let node_size = node_fields.len(); if node_size == 0 { bail!("Invalid snapshot: node_fields schema is empty"); } - let mut aggregates: std::collections::HashMap = std::collections::HashMap::new(); + let mut aggregates: std::collections::HashMap = + std::collections::HashMap::new(); let mut current_idx = 0; while current_idx + node_size <= nodes.len() { let id = nodes[current_idx + id_offset]; let name_str_idx = usize::try_from(nodes[current_idx + name_offset]) .ok() .context("Corrupt snapshot: string index overflow on 32-bit architecture")?; - let name = val.strings.get(name_str_idx).cloned() - .ok_or_else(|| anyhow!("Corrupt snapshot: string index {} out of bounds (strings len {})", name_str_idx, val.strings.len()))?; + let name_ref = val.strings.get(name_str_idx).ok_or_else(|| { + anyhow!( + "Corrupt snapshot: string index {} out of bounds (strings len {})", + name_str_idx, + val.strings.len() + ) + })?; let self_size = nodes[current_idx + self_size_offset]; - aggregates.entry(name).or_insert_with(ClassAggregate::new) - .nodes.insert(id, self_size); + 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); + } current_idx += node_size; } @@ -337,13 +369,11 @@ fn diff_snapshots( base: &std::collections::HashMap, current: &std::collections::HashMap, ) -> Vec { - let mut all_names: std::collections::HashSet<&str> = std::collections::HashSet::new(); - all_names.extend(base.keys().map(String::as_str)); - all_names.extend(current.keys().map(String::as_str)); + let mut diffs: Vec = Vec::new(); - let mut diffs: Vec = all_names.into_iter().filter_map(|name| { + // 1. Process all classes in `current` (covers retained and added classes) + for (name, cur_agg) in current { let base_agg = base.get(name); - let cur_agg = current.get(name); let mut added_ids: Vec = Vec::new(); let mut added_self_sizes: Vec = Vec::new(); @@ -352,52 +382,118 @@ fn diff_snapshots( let mut added_size: u64 = 0; let mut removed_size: u64 = 0; - if let Some(cur) = cur_agg { - for (id, size) in &cur.nodes { - let in_base = base_agg.map_or(false, |b| b.nodes.contains_key(id)); - if !in_base { + if let Some(b) = base_agg { + for (id, size) in &cur_agg.nodes { + if !b.nodes.contains_key(id) { added_ids.push(*id); added_self_sizes.push(*size); added_size += size; } } - } - if let Some(base_a) = base_agg { - for (id, size) in &base_a.nodes { - let in_cur = cur_agg.map_or(false, |c| c.nodes.contains_key(id)); - if !in_cur { + for (id, size) in &b.nodes { + if !cur_agg.nodes.contains_key(id) { deleted_ids.push(*id); deleted_self_sizes.push(*size); removed_size += size; } } + } else { + for (id, size) in &cur_agg.nodes { + added_ids.push(*id); + added_self_sizes.push(*size); + added_size += size; + } } let added_count = added_ids.len(); let removed_count = deleted_ids.len(); - if added_count == 0 && removed_count == 0 { - return None; + if added_count > 0 || removed_count > 0 { + // Ensure added_ids and added_self_sizes are sorted deterministically + if added_count > 1 { + let mut zipped: Vec<(u64, u64)> = + added_ids.into_iter().zip(added_self_sizes).collect(); + zipped.sort_unstable_by_key(|&(id, _)| id); + let (ids, sizes): (Vec, Vec) = zipped.into_iter().unzip(); + added_ids = ids; + added_self_sizes = sizes; + } + + // Ensure deleted_ids and deleted_self_sizes are sorted deterministically + if removed_count > 1 { + let mut zipped: Vec<(u64, u64)> = + deleted_ids.into_iter().zip(deleted_self_sizes).collect(); + zipped.sort_unstable_by_key(|&(id, _)| id); + let (ids, sizes): (Vec, Vec) = zipped.into_iter().unzip(); + deleted_ids = ids; + deleted_self_sizes = sizes; + } + + let count_delta = added_count as i64 - removed_count as i64; + let size_delta = added_size as i64 - removed_size as i64; + + diffs.push(HeapSnapshotClassDiff { + class_name: name.clone(), + added_count, + removed_count, + count_delta, + added_size, + removed_size, + size_delta, + added_ids, + added_self_sizes, + deleted_ids, + deleted_self_sizes, + }); } - let count_delta = added_count as i64 - removed_count as i64; - let size_delta = added_size as i64 - removed_size as i64; - - Some(HeapSnapshotClassDiff { - class_name: name.to_string(), - added_count, - removed_count, - count_delta, - added_size, - removed_size, - size_delta, - added_ids, - added_self_sizes, - deleted_ids, - deleted_self_sizes, - }) - }).collect(); + } + + // 2. Process classes only in `base` (covers entirely deleted classes) + for (name, base_agg) in base { + if !current.contains_key(name) { + let mut deleted_ids: Vec = Vec::with_capacity(base_agg.nodes.len()); + let mut deleted_self_sizes: Vec = Vec::with_capacity(base_agg.nodes.len()); + let mut removed_size: u64 = 0; + + for (id, size) in &base_agg.nodes { + deleted_ids.push(*id); + deleted_self_sizes.push(*size); + removed_size += size; + } + + let removed_count = deleted_ids.len(); + + // Ensure deleted_ids and deleted_self_sizes are sorted deterministically + if removed_count > 1 { + let mut zipped: Vec<(u64, u64)> = + deleted_ids.into_iter().zip(deleted_self_sizes).collect(); + zipped.sort_unstable_by_key(|&(id, _)| id); + let (ids, sizes): (Vec, Vec) = zipped.into_iter().unzip(); + deleted_ids = ids; + deleted_self_sizes = sizes; + } + + let count_delta = -(removed_count as i64); + let size_delta = -(removed_size as i64); + + diffs.push(HeapSnapshotClassDiff { + class_name: name.clone(), + added_count: 0, + removed_count, + count_delta, + added_size: 0, + removed_size, + size_delta, + added_ids: Vec::new(), + added_self_sizes: Vec::new(), + deleted_ids, + deleted_self_sizes, + }); + } + } diffs.sort_by(|a, b| { - b.size_delta.cmp(&a.size_delta) + b.size_delta + .cmp(&a.size_delta) .then_with(|| a.class_name.cmp(&b.class_name)) }); diffs @@ -421,7 +517,9 @@ pub fn format_class_diff_summary( ) -> Result { if format.is_text() { let mut out = String::new(); - out.push_str("idx,className,addedCount,removedCount,countDelta,addedSize,removedSize,sizeDelta\n"); + out.push_str( + "idx,className,addedCount,removedCount,countDelta,addedSize,removedSize,sizeDelta\n", + ); for (i, d) in diffs.iter().enumerate() { out.push_str(&format!( "{},{},{},{},{},{},{},{}\n", @@ -437,19 +535,26 @@ pub fn format_class_diff_summary( } Ok(out) } else { - let rows: Vec = diffs.iter().enumerate().map(|(i, d)| { - json!({ - "idx": i, - "className": d.class_name, - "addedCount": d.added_count, - "removedCount": d.removed_count, - "countDelta": d.count_delta, - "addedSize": d.added_size, - "removedSize": d.removed_size, - "sizeDelta": d.size_delta, + let rows: Vec = diffs + .iter() + .enumerate() + .map(|(i, d)| { + json!({ + "idx": i, + "className": d.class_name, + "addedCount": d.added_count, + "removedCount": d.removed_count, + "countDelta": d.count_delta, + "addedSize": d.added_size, + "removedSize": d.removed_size, + "sizeDelta": d.size_delta, + }) }) - }).collect(); - Ok(crate::format::format_structured(&json!({ "diffs": rows }), format)?) + .collect(); + Ok(crate::format::format_structured( + &json!({ "diffs": rows }), + format, + )?) } } @@ -482,10 +587,16 @@ pub fn format_class_diff_detail( } Ok(out) } else { - let added: Vec = diff.added_ids.iter().zip(diff.added_self_sizes.iter()) + let added: Vec = diff + .added_ids + .iter() + .zip(diff.added_self_sizes.iter()) .map(|(id, size)| json!({ "op": "+", "nodeId": id, "selfSize": size })) .collect(); - let deleted: Vec = diff.deleted_ids.iter().zip(diff.deleted_self_sizes.iter()) + let deleted: Vec = diff + .deleted_ids + .iter() + .zip(diff.deleted_self_sizes.iter()) .map(|(id, size)| json!({ "op": "-", "nodeId": id, "selfSize": size })) .collect(); let mut nodes: Vec = added; @@ -528,10 +639,13 @@ pub async fn compare_heapsnapshots_offline( let out = match class_index { None => format_class_diff_summary(&diffs, format)?, Some(idx) => { - let diff = diffs.get(idx).ok_or_else(|| anyhow!( - "Invalid classIndex: {}. Total classes with changes: {}", - idx, diffs.len() - ))?; + let diff = diffs.get(idx).ok_or_else(|| { + anyhow!( + "Invalid classIndex: {}. Total classes with changes: {}", + idx, + diffs.len() + ) + })?; format_class_diff_detail(idx, diff, format)? } }; @@ -635,11 +749,17 @@ mod tests { // Name with comma let out_comma = format_node_details(123, "My,Class", 100, OutputFormat::Text).unwrap(); - assert_eq!(out_comma, "nodeId,nodeName,selfSize\n123,\"My,Class\",100\n"); + assert_eq!( + out_comma, + "nodeId,nodeName,selfSize\n123,\"My,Class\",100\n" + ); // Name with quotes let out_quotes = format_node_details(123, "My\"Class", 100, OutputFormat::Text).unwrap(); - assert_eq!(out_quotes, "nodeId,nodeName,selfSize\n123,\"My\"\"Class\",100\n"); + assert_eq!( + out_quotes, + "nodeId,nodeName,selfSize\n123,\"My\"\"Class\",100\n" + ); // Name with newline let out_nl = format_node_details(123, "My\nClass", 100, OutputFormat::Text).unwrap(); @@ -706,11 +826,17 @@ mod tests { // Base: Map{1@100, 2@100}, String{3@50} // Current: Map{1@100 (retained), 4@150 (new)}, Window{5@500 (new class)} let base = build_class_aggregates(&make_snapshot(&[ - (1, "Map", 100), (2, "Map", 100), (3, "String", 50), - ])).unwrap(); + (1, "Map", 100), + (2, "Map", 100), + (3, "String", 50), + ])) + .unwrap(); let current = build_class_aggregates(&make_snapshot(&[ - (1, "Map", 100), (4, "Map", 150), (5, "Window", 500), - ])).unwrap(); + (1, "Map", 100), + (4, "Map", 150), + (5, "Window", 500), + ])) + .unwrap(); let diffs = diff_snapshots(&base, ¤t); @@ -753,9 +879,8 @@ mod tests { #[test] fn test_diff_class_gone_entirely_from_current() { - let base = build_class_aggregates(&make_snapshot(&[ - (1, "Old", 80), (2, "Old", 40), - ])).unwrap(); + let base = + build_class_aggregates(&make_snapshot(&[(1, "Old", 80), (2, "Old", 40)])).unwrap(); let current = build_class_aggregates(&make_snapshot(&[])).unwrap(); let diffs = diff_snapshots(&base, ¤t); assert_eq!(diffs.len(), 1); @@ -768,21 +893,22 @@ mod tests { #[test] fn test_format_summary_and_detail_share_indices() { - let base = build_class_aggregates(&make_snapshot(&[ - (1, "Map", 100), (2, "String", 50), - ])).unwrap(); - let current = build_class_aggregates(&make_snapshot(&[ - (3, "Window", 500), (4, "Map", 150), - ])).unwrap(); + let base = + build_class_aggregates(&make_snapshot(&[(1, "Map", 100), (2, "String", 50)])).unwrap(); + let current = + build_class_aggregates(&make_snapshot(&[(3, "Window", 500), (4, "Map", 150)])).unwrap(); let diffs = diff_snapshots(&base, ¤t); let summary = format_class_diff_summary(&diffs, crate::format::OutputFormat::Text).unwrap(); // First data row (idx 0) should be the largest sizeDelta — Window. - assert!(summary.starts_with("idx,className,addedCount,removedCount,countDelta,addedSize,removedSize,sizeDelta\n")); + assert!(summary.starts_with( + "idx,className,addedCount,removedCount,countDelta,addedSize,removedSize,sizeDelta\n" + )); assert!(summary.contains("0,Window,1,0,1,500,0,500\n")); // Detail for idx 0 must reference the same class. - let detail = format_class_diff_detail(0, &diffs[0], crate::format::OutputFormat::Text).unwrap(); + let detail = + format_class_diff_detail(0, &diffs[0], crate::format::OutputFormat::Text).unwrap(); assert!(detail.contains("className:Window")); assert!(detail.contains("+,3,500")); // Map's id 4 should NOT appear here — it's a different class. @@ -812,23 +938,27 @@ mod tests { write!(cur_file, "{}", cur_json).unwrap(); let rt = tokio::runtime::Runtime::new().unwrap(); - let summary = rt.block_on(compare_heapsnapshots_offline( - base_file.path().to_str().unwrap(), - cur_file.path().to_str().unwrap(), - None, - OutputFormat::Text, - )).unwrap(); + let summary = rt + .block_on(compare_heapsnapshots_offline( + base_file.path().to_str().unwrap(), + cur_file.path().to_str().unwrap(), + None, + OutputFormat::Text, + )) + .unwrap(); let summary_out = summary.output; assert!(summary_out.contains("0,Window")); assert!(summary_out.contains("1,Map")); // Detail for idx 0 should print Window's node 3. - let detail = rt.block_on(compare_heapsnapshots_offline( - base_file.path().to_str().unwrap(), - cur_file.path().to_str().unwrap(), - Some(0), - OutputFormat::Text, - )).unwrap(); + let detail = rt + .block_on(compare_heapsnapshots_offline( + base_file.path().to_str().unwrap(), + cur_file.path().to_str().unwrap(), + Some(0), + OutputFormat::Text, + )) + .unwrap(); assert!(detail.output.contains("className:Window")); assert!(detail.output.contains("+,3,500")); diff --git a/src/commands/read_page.rs b/src/commands/read_page.rs index 063de2b..994ec8a 100644 --- a/src/commands/read_page.rs +++ b/src/commands/read_page.rs @@ -329,7 +329,10 @@ mod tests { #[test] fn extract_title_missing() { - assert_eq!(extract_title_from_html("Hello"), None); + assert_eq!( + extract_title_from_html("Hello"), + None + ); } #[test] @@ -352,10 +355,7 @@ mod tests { #[test] fn extract_title_decodes_html_entities() { let html = "Foo & Bar"; - assert_eq!( - extract_title_from_html(html), - Some("Foo & Bar".to_string()) - ); + assert_eq!(extract_title_from_html(html), Some("Foo & Bar".to_string())); } #[test] @@ -456,10 +456,7 @@ mod tests { #[test] fn unwrap_iframes_empty() { - assert_eq!( - unwrap_iframes(""), - "" - ); + assert_eq!(unwrap_iframes(""), ""); } #[test] @@ -506,7 +503,8 @@ mod tests { #[test] fn extract_content_passes_url_for_relative_link_resolution() { - let html = "Testlink"; + let html = + "Testlink"; let url = "https://example.com/article"; let (_, _) = extract_content(html, Some(url)); // Smoke test: URL is accepted without panic. Readability may or may not @@ -613,13 +611,8 @@ mod tests { excerpt: None, site_name: None, }; - let result = format_output( - html, - &meta, - Some("https://example.com"), - OutputFormat::Json, - ) - .unwrap(); + let result = + format_output(html, &meta, Some("https://example.com"), OutputFormat::Json).unwrap(); let parsed: Value = serde_json::from_str(&result).unwrap(); assert_eq!(parsed["url"], "https://example.com"); } diff --git a/src/commands/screenshot.rs b/src/commands/screenshot.rs index a95f19f..a788c57 100644 --- a/src/commands/screenshot.rs +++ b/src/commands/screenshot.rs @@ -78,15 +78,27 @@ pub async fn take_screenshot( if let Some(size) = metrics.get("cssContentSize") { // Filter non-positive values (empty/unrendered pages, certain // document types) — they'd produce an invalid CDP clip. - src_w = size["width"].as_f64().filter(|&v| v > 0.0).unwrap_or(1920.0); - src_h = size["height"].as_f64().filter(|&v| v > 0.0).unwrap_or(1080.0); + src_w = size["width"] + .as_f64() + .filter(|&v| v > 0.0) + .unwrap_or(1920.0); + src_h = size["height"] + .as_f64() + .filter(|&v| v > 0.0) + .unwrap_or(1080.0); } } else { // cssLayoutViewport.clientWidth/Height is the visible viewport in // CSS pixels; pageX/pageY are its document-origin scroll offsets. if let Some(viewport) = metrics.get("cssLayoutViewport") { - src_w = viewport["clientWidth"].as_f64().filter(|&v| v > 0.0).unwrap_or(1920.0); - src_h = viewport["clientHeight"].as_f64().filter(|&v| v > 0.0).unwrap_or(1080.0); + src_w = viewport["clientWidth"] + .as_f64() + .filter(|&v| v > 0.0) + .unwrap_or(1920.0); + src_h = viewport["clientHeight"] + .as_f64() + .filter(|&v| v > 0.0) + .unwrap_or(1080.0); scroll_x = viewport["pageX"].as_f64().unwrap_or(0.0); scroll_y = viewport["pageY"].as_f64().unwrap_or(0.0); } @@ -138,7 +150,12 @@ pub async fn take_screenshot( /// Returns the smaller of the width and height scale ratios, clamped to <= 1.0 /// (never upscales). A `None` dimension, or a non-positive max/src value, yields /// 1.0 for that axis (no scaling). Returns 1.0 when neither dimension is set. -fn clip_scale_factor(src_w: f64, src_h: f64, max_width: Option, max_height: Option) -> f64 { +fn clip_scale_factor( + src_w: f64, + src_h: f64, + max_width: Option, + max_height: Option, +) -> 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, @@ -166,7 +183,10 @@ mod tests { #[test] fn negative_max_is_treated_as_no_scaling() { - assert_eq!(clip_scale_factor(1920.0, 1080.0, Some(-100.0), Some(-50.0)), 1.0); + assert_eq!( + clip_scale_factor(1920.0, 1080.0, Some(-100.0), Some(-50.0)), + 1.0 + ); } #[test] @@ -189,12 +209,18 @@ mod tests { #[test] fn both_dimensions_uses_the_smaller_ratio() { // src 2000x1000, max 1000x250 → width_scale 0.5, height_scale 0.25 → 0.25 - assert_eq!(clip_scale_factor(2000.0, 1000.0, Some(1000.0), Some(250.0)), 0.25); + assert_eq!( + clip_scale_factor(2000.0, 1000.0, Some(1000.0), Some(250.0)), + 0.25 + ); } #[test] fn never_upscales_when_max_exceeds_source() { // src 800x600, max 1600x1200 → both ratios > 1.0, clamped to 1.0 - assert_eq!(clip_scale_factor(800.0, 600.0, Some(1600.0), Some(1200.0)), 1.0); + assert_eq!( + clip_scale_factor(800.0, 600.0, Some(1600.0), Some(1200.0)), + 1.0 + ); } } diff --git a/src/commands/third_party.rs b/src/commands/third_party.rs index 41c84b2..c18e0ab 100644 --- a/src/commands/third_party.rs +++ b/src/commands/third_party.rs @@ -143,9 +143,7 @@ pub async fn list_3p_tools( let parts: Vec = ann .as_object() .map(|obj| { - obj.iter() - .map(|(k, v)| format!("{}={}", k, v)) - .collect() + obj.iter().map(|(k, v)| format!("{}={}", k, v)).collect() }) .unwrap_or_default(); if !parts.is_empty() { diff --git a/src/lib.rs b/src/lib.rs index b2bc613..baffcdb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -468,8 +468,11 @@ fn absolutize_path(path: &str) -> Result { // Fail loudly if the CWD can't be resolved: silently falling back to an // empty path would send a bogus relative path to the daemon, which would // resolve it against the daemon's (different) startup CWD. - let cwd = std::env::current_dir() - .map_err(|e| anyhow::anyhow!("Failed to resolve CLI working directory to absolutize path '{path}': {e}"))?; + let cwd = std::env::current_dir().map_err(|e| { + anyhow::anyhow!( + "Failed to resolve CLI working directory to absolutize path '{path}': {e}" + ) + })?; Ok(cwd.join(p).to_string_lossy().to_string()) } } @@ -539,7 +542,9 @@ fn parse_args(named_args: &[String], raw_args: &[String]) -> Result Date: Tue, 7 Jul 2026 12:33:12 +0800 Subject: [PATCH 05/11] feat: improve Chrome devtools connection handling and safeguard daemon 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 --- AGENTS.md | 29 +++++++++++ skill/chrome-devtools/SKILL.md | 43 ++++++++++++++- src/cdp.rs | 27 +++++++++- src/commands/executor.rs | 2 +- src/lib.rs | 95 ++++++++++++++++++++++++++++++++-- 5 files changed, 188 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9d1e61b..caf01de 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,6 +46,18 @@ Detailed documentation for individual commands: - [read-page](wiki/read-page.md) — page content extraction as markdown +## Agent Skill + +`skill/chrome-devtools/SKILL.md` is the **source of truth** for the agent-facing +skill/documentation — it's what teaches an agent (e.g. Claude Code, opencode) how +to use this CLI (targeting, standard patterns, gotchas, failure handling, etc.). +It gets installed/copied into an agent's skills directory (e.g. +`~/.config/opencode/skills/chrome-devtools/SKILL.md`); that installed copy is a +**deployed artifact**, not the source — always edit `skill/chrome-devtools/SKILL.md` +in this repo, then re-sync/reinstall it, rather than editing the installed copy +directly. `skill/chrome-devtools/CUSTOM_SCRIPTING.md` documents `run-script` and +`adapter` in more depth. + ## Key Concepts ### Daemon Architecture @@ -54,6 +66,17 @@ A background daemon (`/tmp/chrome-devtools-daemon.sock`) keeps a persistent CDP WebSocket connection. First CLI invocation spawns it; subsequent commands reuse it. 5-minute idle timeout. +`CdpClient::connect` (`cdp.rs`) bounds the WebSocket handshake with a timeout +(`CHROME_CONNECT_TIMEOUT_SECS`, default 10s). Without it, a pending Chrome +remote-debugging consent dialog would hang the handshake indefinitely — and +since the daemon binds its socket *before* connecting to Chrome (see comments +in `daemon.rs`), the CLI's `wait_for_daemon()` would succeed immediately while +the actual request silently hung forever waiting on the daemon's response, with +no error and no way for a caller (especially an unattended agent) to tell what +was wrong. The timeout error message is written to be agent-actionable: retry +at most once, then stop and ask a human rather than looping or calling +`kill-daemon`. + ### Page Targeting Every page gets a deterministic friendly name (e.g. `warm-squid`) derived from @@ -77,6 +100,12 @@ LLM-friendly) produce structured output. Mutually exclusive. before any Chrome connection or daemon spawn. `inspect-heapsnapshot-node` parses a local `.heapsnapshot` file purely offline. +`kill-daemon` drops the daemon's already-approved Chrome connection, so it's +guarded (`kill_daemon_decision` in `lib.rs`): interactive (TTY) callers get a +`[y/N]` confirmation prompt; non-interactive callers (agents, scripts) are +refused outright unless `--force` is passed. It must never be used as a +"retry" step for connection failures — see the timeout note above. + ### Path Resolution The daemon retains its startup CWD, so the CLI resolves all relative file-path diff --git a/skill/chrome-devtools/SKILL.md b/skill/chrome-devtools/SKILL.md index 3a9335f..5a74604 100644 --- a/skill/chrome-devtools/SKILL.md +++ b/skill/chrome-devtools/SKILL.md @@ -396,8 +396,33 @@ chrome-devtools --target adapter [--arg key=v ### Daemon ```bash -chrome-devtools kill-daemon # stop the background daemon process -``` +chrome-devtools kill-daemon # refuses when run non-interactively (agents) unless --force +chrome-devtools kill-daemon --force # kills unconditionally +``` + +## Failure Handling: "Failed to connect to Chrome" / a command hangs + +Chrome's remote-debugging connection requires a one-time **human approval dialog** +in Chrome. If a command hangs or fails with a connection/timeout error, the most +likely cause is that this dialog is open and waiting for the human — not a bug +you can fix by retrying. + +**If a command hangs for a long time or errors with "Failed to connect to Chrome" +or "Timed out ... connecting to Chrome":** +1. Retry **at most once** (the human may have already approved it just now). +2. If it fails again, **STOP.** Do not keep retrying — the human is very likely + away from the keyboard and no amount of retrying will approve the dialog for + them. +3. Tell the user directly that Chrome is waiting for them to approve the + remote-debugging connection dialog, and wait for their response. + +**Never run `kill-daemon` as a way to "fix" a connection problem.** It does not +help — it destroys the daemon's already-approved connection (if one exists) and +guarantees the *next* attempt needs a fresh human approval, making things worse. +For this reason, `kill-daemon` refuses to run non-interactively without +`--force`. As an agent, only pass `--force` if the user has explicitly asked you +to kill the daemon for some other reason (e.g. it's stuck on unrelated JS +execution) — never as a reaction to a connection failure. ## Critical Gotchas @@ -459,6 +484,20 @@ chrome-devtools --target warm-squid console ### ✓ CORRECT: Each drain is a fresh window Run `console` / `network` right after the action that produces events, OR use `--duration` to collect for a window of time. +### ✗ WRONG: Retrying in a loop or running `kill-daemon` on connection failure +```bash +chrome-devtools list-pages # hangs / fails: "Failed to connect to Chrome" +chrome-devtools kill-daemon --force # ❌ WRONG: destroys the approved connection, makes it worse +chrome-devtools list-pages # retry again, and again... ❌ WRONG: human is likely away +``` + +### ✓ CORRECT: Retry once, then stop and ask the human +```bash +chrome-devtools list-pages # hangs / fails: "Failed to connect to Chrome" +chrome-devtools list-pages # ✓ one retry, in case the human just approved it +# still failing → STOP retrying, tell the user Chrome needs approval, and wait +``` + ## Output Format Summary | Flag | Description | diff --git a/src/cdp.rs b/src/cdp.rs index 0efaf2f..4df0c41 100644 --- a/src/cdp.rs +++ b/src/cdp.rs @@ -162,11 +162,36 @@ fn stash_tab_emulation( } } +/// Read the Chrome WebSocket connect timeout from `CHROME_CONNECT_TIMEOUT_SECS`, +/// defaulting to 10. +fn connect_timeout() -> std::time::Duration { + std::env::var("CHROME_CONNECT_TIMEOUT_SECS") + .ok() + .and_then(|v| v.parse::().ok()) + .map(std::time::Duration::from_secs) + .unwrap_or(std::time::Duration::from_secs(10)) +} + impl CdpClient { /// Connect to Chrome via WebSocket and return a CDP client. + /// + /// Bounded by a timeout: without it, a pending Chrome remote-debugging + /// consent dialog leaves the WebSocket handshake hanging indefinitely, + /// which (via the daemon) makes every command appear to hang forever + /// with no diagnostic. pub async fn connect(ws_url: &str) -> Result { - let (ws, _) = connect_async(ws_url) + let (ws, _) = tokio::time::timeout(connect_timeout(), connect_async(ws_url)) .await + .map_err(|_| { + anyhow!( + "Timed out after {}s connecting to Chrome at {ws_url}. Chrome may be \ + waiting for a human to approve the remote-debugging connection dialog. \ + If you are an automated agent: do not retry in a loop and do not run \ + kill-daemon (it will not fix this and will require a fresh approval) — \ + stop and ask the human to check Chrome, then retry once.", + connect_timeout().as_secs() + ) + })? .map_err(|e| anyhow!("Failed to connect to Chrome at {ws_url}: {e}"))?; let (write, read) = ws.split(); Ok(Self { diff --git a/src/commands/executor.rs b/src/commands/executor.rs index 2c5460a..75f4bcf 100644 --- a/src/commands/executor.rs +++ b/src/commands/executor.rs @@ -88,7 +88,7 @@ pub fn known_args(cmd: &str) -> &'static [&'static str] { "output", "track_navigation", ], - "kill-daemon" => &[], + "kill-daemon" => &["force"], _ => &[], } } diff --git a/src/lib.rs b/src/lib.rs index baffcdb..2acc6e2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -405,7 +405,16 @@ pub enum Commands { /// Stop the background daemon process #[command(name = "kill-daemon")] - KillDaemon, + KillDaemon { + /// Skip the confirmation/refusal guard and kill unconditionally. + /// + /// Killing the daemon drops any already-approved Chrome remote-debugging + /// connection; reconnecting requires the human to re-approve Chrome's + /// consent dialog. This does NOT fix "Failed to connect to Chrome" + /// errors — do not use it as a retry step. + #[arg(long)] + force: bool, + }, } impl Cli { @@ -451,11 +460,36 @@ impl Cli { Commands::SwLogs { .. } => "sw-logs", Commands::RunScript { .. } => "run-script", Commands::Adapter { .. } => "adapter", - Commands::KillDaemon => "kill-daemon", + Commands::KillDaemon { .. } => "kill-daemon", } } } +/// What to do when `kill-daemon` is invoked, given `--force` and whether +/// stdin is a TTY. Kept pure so the policy is unit-testable without +/// mocking stdin I/O. +#[derive(Debug, PartialEq, Eq)] +enum KillDaemonDecision { + /// Kill immediately, no confirmation needed. + Proceed, + /// Interactive session — ask the human to confirm. + PromptUser, + /// Non-interactive (e.g. an agent) without `--force` — refuse outright. + /// Killing the daemon drops the approved Chrome connection and forces + /// the human to re-approve it, so an agent must never do this silently. + RefuseNonInteractive, +} + +fn kill_daemon_decision(force: bool, is_tty: bool) -> KillDaemonDecision { + if force { + KillDaemonDecision::Proceed + } else if is_tty { + KillDaemonDecision::PromptUser + } else { + KillDaemonDecision::RefuseNonInteractive + } +} + /// Build a DaemonRequest from parsed CLI args. /// Resolve a relative path to an absolute one using the CLI process's CWD. /// The daemon retains its own startup CWD, so relative paths sent to it would @@ -743,7 +777,7 @@ fn build_request(cli: &Cli) -> Result { "track_navigation": track_navigation }), ), - Commands::KillDaemon => unreachable!("KillDaemon is handled before build_request"), + Commands::KillDaemon { .. } => unreachable!("KillDaemon is handled before build_request"), Commands::InspectHeapSnapshotNode { .. } => { unreachable!("InspectHeapSnapshotNode is handled before build_request") } @@ -837,7 +871,36 @@ pub async fn run() -> Result<()> { }; // Handle kill-daemon without connecting to Chrome - if matches!(cli.command, Commands::KillDaemon) { + if let Commands::KillDaemon { force } = cli.command { + use std::io::IsTerminal; + match kill_daemon_decision(force, std::io::stdin().is_terminal()) { + KillDaemonDecision::RefuseNonInteractive => { + return Err(error::CliError::new( + error::ErrorCode::InvalidInput, + "Refusing to kill the daemon: this will NOT fix \"Failed to connect to \ + Chrome\" errors and will force the human to re-approve Chrome's \ + remote-debugging connection. If you are certain this is what you want, \ + re-run with --force.", + ) + .into()); + } + KillDaemonDecision::PromptUser => { + use std::io::Write; + eprint!( + "Kill the daemon? This drops the approved Chrome connection and will \ + require re-approval in Chrome. [y/N] " + ); + std::io::stderr().flush().ok(); + let mut answer = String::new(); + std::io::stdin().read_line(&mut answer).ok(); + if !matches!(answer.trim().to_ascii_lowercase().as_str(), "y" | "yes") { + println!("Aborted."); + return Ok(()); + } + } + KillDaemonDecision::Proceed => {} + } + let pid_path = protocol::pid_path(); #[cfg(unix)] let sock_path = protocol::socket_path(); @@ -1395,6 +1458,30 @@ mod tests { assert!(!obj.get("bool_false").unwrap().as_bool().unwrap()); } + #[test] + fn test_kill_daemon_decision_force_always_proceeds() { + assert_eq!(kill_daemon_decision(true, true), KillDaemonDecision::Proceed); + assert_eq!(kill_daemon_decision(true, false), KillDaemonDecision::Proceed); + } + + #[test] + fn test_kill_daemon_decision_tty_prompts() { + assert_eq!( + kill_daemon_decision(false, true), + KillDaemonDecision::PromptUser + ); + } + + #[test] + fn test_kill_daemon_decision_non_tty_refuses() { + // This is the agent/non-interactive case: refuse rather than silently + // kill a daemon holding an already-approved Chrome connection. + assert_eq!( + kill_daemon_decision(false, false), + KillDaemonDecision::RefuseNonInteractive + ); + } + #[test] fn test_parse_args_preserves_leading_zeros() { // ZIP codes, phone numbers, and signed tokens must not be rewritten as From 15b1efb6a114f40fe0d86eb2c829c0fb6457ab83 Mon Sep 17 00:00:00 2001 From: Aero Date: Tue, 7 Jul 2026 15:39:53 +0800 Subject: [PATCH 06/11] fix(memory): validate snapshot integrity and standardize error handling * 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 --- src/commands/memory.rs | 58 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 7 deletions(-) diff --git a/src/commands/memory.rs b/src/commands/memory.rs index 0f7880f..4706821 100644 --- a/src/commands/memory.rs +++ b/src/commands/memory.rs @@ -4,6 +4,7 @@ use std::fs::File; use std::io::BufReader; use crate::cdp::CdpClient; +use crate::error::{CliError, ErrorCode}; use crate::result::CommandResult; /// Take a heap snapshot of the page and save it to a file. @@ -54,7 +55,8 @@ pub async fn take_heapsnapshot( })?, ); - // First, let's enable the HeapProfiler. + // HeapProfiler must be enabled before takeHeapSnapshot — Chrome rejects + // the command otherwise. client .send_to_target(session_id, "HeapProfiler.enable", json!({})) .await @@ -312,6 +314,14 @@ fn build_class_aggregates( if node_size == 0 { bail!("Invalid snapshot: node_fields schema is empty"); } + if !nodes.len().is_multiple_of(node_size) { + bail!( + "Corrupt snapshot: nodes array length ({}) is not a multiple of node_size ({}); \ + the file is truncated or malformed", + nodes.len(), + node_size + ); + } let mut aggregates: std::collections::HashMap = std::collections::HashMap::new(); @@ -650,10 +660,13 @@ pub async fn compare_heapsnapshots_offline( None => format_class_diff_summary(&diffs, format)?, Some(idx) => { let diff = diffs.get(idx).ok_or_else(|| { - anyhow!( - "Invalid classIndex: {}. Total classes with changes: {}", - idx, - diffs.len() + CliError::new( + ErrorCode::InvalidInput, + format!( + "Invalid classIndex: {}. Total classes with changes: {}", + idx, + diffs.len() + ), ) })?; format_class_diff_detail(idx, diff, format)? @@ -831,6 +844,30 @@ mod tests { assert_eq!(s.nodes.get(&3), Some(&50)); } + #[test] + fn test_build_class_aggregates_rejects_truncated_nodes() { + // node_fields describes 3-field records, but nodes has 4 entries — a + // truncated/malformed flat array that isn't a multiple of node_size. + // Must error instead of silently dropping the trailing partial record. + let snap = HeapSnapshot { + snapshot: SnapshotMeta { + meta: MetaDetails { + node_fields: vec!["id".into(), "name".into(), "self_size".into()], + }, + }, + nodes: vec![1, 0, 100, 2], + strings: vec!["Map".into()], + }; + let err = match build_class_aggregates(&snap) { + Ok(_) => panic!("expected error for truncated nodes array"), + Err(e) => e, + }; + assert!( + err.to_string().contains("not a multiple of node_size"), + "got: {err}" + ); + } + #[test] fn test_diff_added_removed_and_retained() { // Base: Map{1@100, 2@100}, String{3@50} @@ -979,11 +1016,18 @@ mod tests { Some(99), OutputFormat::Text, )); - let msg = match err { + let err = match err { Ok(_) => panic!("expected error for out-of-range class_index"), - Err(e) => e.to_string(), + Err(e) => e, }; + let msg = err.to_string(); assert!(msg.contains("Invalid classIndex"), "got: {msg}"); assert!(msg.contains("99")); + // Must surface as a typed InvalidInput error so callers (e.g. main's + // exit-code mapping) get a stable, non-Unspecified error code. + let cli_err = err + .downcast_ref::() + .expect("expected a CliError"); + assert_eq!(cli_err.code(), crate::error::ErrorCode::InvalidInput); } } From ef2fcb6231d3e7d97b889e13740dc433450a25b9 Mon Sep 17 00:00:00 2001 From: Aero Date: Tue, 7 Jul 2026 22:04:18 +0800 Subject: [PATCH 07/11] refactor(memory): simplify class aggregate handling and update snapshot diff structure --- src/commands/memory.rs | 107 +++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 69 deletions(-) diff --git a/src/commands/memory.rs b/src/commands/memory.rs index 4706821..ac0f87d 100644 --- a/src/commands/memory.rs +++ b/src/commands/memory.rs @@ -340,13 +340,11 @@ fn build_class_aggregates( })?; let self_size = nodes[current_idx + self_size_offset]; - 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); current_idx += node_size; } @@ -365,10 +363,10 @@ pub struct HeapSnapshotClassDiff { pub removed_size: u64, pub size_delta: i64, // Per-id detail. Exposed only via the `--class-index` path. - pub added_ids: Vec, - pub added_self_sizes: Vec, - pub deleted_ids: Vec, - pub deleted_self_sizes: Vec, + // Each tuple is (node_id, self_size) — kept together so sorting and + // formatting never have to zip/unzip parallel vectors. + pub added_nodes: Vec<(u64, u64)>, + pub deleted_nodes: Vec<(u64, u64)>, } /// Compute the per-class diff between two snapshots. Returns rows filtered to @@ -387,58 +385,42 @@ fn diff_snapshots( // Upper-bounds: every current node could be new, every base node // could be gone. Avoids reallocation churn on large classes. - let mut added_ids: Vec = Vec::with_capacity(cur_agg.nodes.len()); - let mut added_self_sizes: Vec = Vec::with_capacity(cur_agg.nodes.len()); + let mut added_nodes: Vec<(u64, u64)> = Vec::with_capacity(cur_agg.nodes.len()); let base_len = base_agg.map(|b| b.nodes.len()).unwrap_or(0); - let mut deleted_ids: Vec = Vec::with_capacity(base_len); - let mut deleted_self_sizes: Vec = Vec::with_capacity(base_len); + let mut deleted_nodes: Vec<(u64, u64)> = Vec::with_capacity(base_len); let mut added_size: u64 = 0; let mut removed_size: u64 = 0; if let Some(b) = base_agg { for (id, size) in &cur_agg.nodes { if !b.nodes.contains_key(id) { - added_ids.push(*id); - added_self_sizes.push(*size); + added_nodes.push((*id, *size)); added_size += size; } } for (id, size) in &b.nodes { if !cur_agg.nodes.contains_key(id) { - deleted_ids.push(*id); - deleted_self_sizes.push(*size); + deleted_nodes.push((*id, *size)); removed_size += size; } } } else { for (id, size) in &cur_agg.nodes { - added_ids.push(*id); - added_self_sizes.push(*size); + added_nodes.push((*id, *size)); added_size += size; } } - let added_count = added_ids.len(); - let removed_count = deleted_ids.len(); + let added_count = added_nodes.len(); + let removed_count = deleted_nodes.len(); if added_count > 0 || removed_count > 0 { - // Ensure added_ids and added_self_sizes are sorted deterministically + // Sort deterministically by node id so summary/detail indices + // stay stable across runs. if added_count > 1 { - let mut zipped: Vec<(u64, u64)> = - added_ids.into_iter().zip(added_self_sizes).collect(); - zipped.sort_unstable_by_key(|&(id, _)| id); - let (ids, sizes): (Vec, Vec) = zipped.into_iter().unzip(); - added_ids = ids; - added_self_sizes = sizes; + added_nodes.sort_unstable_by_key(|(id, _)| *id); } - - // Ensure deleted_ids and deleted_self_sizes are sorted deterministically if removed_count > 1 { - let mut zipped: Vec<(u64, u64)> = - deleted_ids.into_iter().zip(deleted_self_sizes).collect(); - zipped.sort_unstable_by_key(|&(id, _)| id); - let (ids, sizes): (Vec, Vec) = zipped.into_iter().unzip(); - deleted_ids = ids; - deleted_self_sizes = sizes; + deleted_nodes.sort_unstable_by_key(|(id, _)| *id); } let count_delta = added_count as i64 - removed_count as i64; @@ -452,10 +434,8 @@ fn diff_snapshots( added_size, removed_size, size_delta, - added_ids, - added_self_sizes, - deleted_ids, - deleted_self_sizes, + added_nodes, + deleted_nodes, }); } } @@ -463,26 +443,19 @@ fn diff_snapshots( // 2. Process classes only in `base` (covers entirely deleted classes) for (name, base_agg) in base { if !current.contains_key(name) { - let mut deleted_ids: Vec = Vec::with_capacity(base_agg.nodes.len()); - let mut deleted_self_sizes: Vec = Vec::with_capacity(base_agg.nodes.len()); + let mut deleted_nodes: Vec<(u64, u64)> = Vec::with_capacity(base_agg.nodes.len()); let mut removed_size: u64 = 0; for (id, size) in &base_agg.nodes { - deleted_ids.push(*id); - deleted_self_sizes.push(*size); + deleted_nodes.push((*id, *size)); removed_size += size; } - let removed_count = deleted_ids.len(); + let removed_count = deleted_nodes.len(); - // Ensure deleted_ids and deleted_self_sizes are sorted deterministically + // Sort deterministically by node id. if removed_count > 1 { - let mut zipped: Vec<(u64, u64)> = - deleted_ids.into_iter().zip(deleted_self_sizes).collect(); - zipped.sort_unstable_by_key(|&(id, _)| id); - let (ids, sizes): (Vec, Vec) = zipped.into_iter().unzip(); - deleted_ids = ids; - deleted_self_sizes = sizes; + deleted_nodes.sort_unstable_by_key(|(id, _)| *id); } let count_delta = -(removed_count as i64); @@ -496,10 +469,8 @@ fn diff_snapshots( added_size: 0, removed_size, size_delta, - added_ids: Vec::new(), - added_self_sizes: Vec::new(), - deleted_ids, - deleted_self_sizes, + added_nodes: Vec::new(), + deleted_nodes, }); } } @@ -592,24 +563,22 @@ pub fn format_class_diff_detail( diff.size_delta, )); out.push_str("\nop,nodeId,selfSize\n"); - for (id, size) in diff.added_ids.iter().zip(diff.added_self_sizes.iter()) { + for (id, size) in &diff.added_nodes { out.push_str(&format!("+,{},{}\n", id, size)); } - for (id, size) in diff.deleted_ids.iter().zip(diff.deleted_self_sizes.iter()) { + for (id, size) in &diff.deleted_nodes { out.push_str(&format!("-,{},{}\n", id, size)); } Ok(out) } else { let added: Vec = diff - .added_ids + .added_nodes .iter() - .zip(diff.added_self_sizes.iter()) .map(|(id, size)| json!({ "op": "+", "nodeId": id, "selfSize": size })) .collect(); let deleted: Vec = diff - .deleted_ids + .deleted_nodes .iter() - .zip(diff.deleted_self_sizes.iter()) .map(|(id, size)| json!({ "op": "-", "nodeId": id, "selfSize": size })) .collect(); let mut nodes: Vec = added; @@ -895,7 +864,7 @@ mod tests { assert_eq!(diffs[0].added_size, 500); assert_eq!(diffs[0].removed_size, 0); assert_eq!(diffs[0].size_delta, 500); - assert_eq!(diffs[0].added_ids, vec![5]); + assert_eq!(diffs[0].added_nodes, vec![(5, 500)]); assert_eq!(diffs[1].class_name, "Map"); assert_eq!(diffs[1].added_count, 1); @@ -904,15 +873,15 @@ mod tests { assert_eq!(diffs[1].added_size, 150); assert_eq!(diffs[1].removed_size, 100); assert_eq!(diffs[1].size_delta, 50); - assert_eq!(diffs[1].added_ids, vec![4]); - assert_eq!(diffs[1].deleted_ids, vec![2]); + assert_eq!(diffs[1].added_nodes, vec![(4, 150)]); + assert_eq!(diffs[1].deleted_nodes, vec![(2, 100)]); assert_eq!(diffs[2].class_name, "String"); assert_eq!(diffs[2].added_count, 0); assert_eq!(diffs[2].removed_count, 1); assert_eq!(diffs[2].count_delta, -1); assert_eq!(diffs[2].size_delta, -50); - assert_eq!(diffs[2].deleted_ids, vec![3]); + assert_eq!(diffs[2].deleted_nodes, vec![(3, 50)]); } #[test] @@ -935,7 +904,7 @@ mod tests { assert_eq!(diffs[0].removed_count, 2); assert_eq!(diffs[0].removed_size, 120); assert_eq!(diffs[0].size_delta, -120); - assert_eq!(diffs[0].deleted_ids.len(), 2); + assert_eq!(diffs[0].deleted_nodes.len(), 2); } #[test] From 3f178a6cddcba83774d5629170264cf0f4279d0c Mon Sep 17 00:00:00 2001 From: Aero Date: Tue, 7 Jul 2026 22:22:13 +0800 Subject: [PATCH 08/11] refactor(memory): optimize class aggregate handling in build_class_aggregates function --- src/cdp.rs | 5 +++-- src/commands/memory.rs | 12 +++++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/cdp.rs b/src/cdp.rs index 4df0c41..2ee0875 100644 --- a/src/cdp.rs +++ b/src/cdp.rs @@ -180,7 +180,8 @@ impl CdpClient { /// which (via the daemon) makes every command appear to hang forever /// with no diagnostic. pub async fn connect(ws_url: &str) -> Result { - let (ws, _) = tokio::time::timeout(connect_timeout(), connect_async(ws_url)) + let timeout_dur = connect_timeout(); + let (ws, _) = tokio::time::timeout(timeout_dur, connect_async(ws_url)) .await .map_err(|_| { anyhow!( @@ -189,7 +190,7 @@ impl CdpClient { If you are an automated agent: do not retry in a loop and do not run \ kill-daemon (it will not fix this and will require a fresh approval) — \ stop and ask the human to check Chrome, then retry once.", - connect_timeout().as_secs() + timeout_dur.as_secs() ) })? .map_err(|e| anyhow!("Failed to connect to Chrome at {ws_url}: {e}"))?; diff --git a/src/commands/memory.rs b/src/commands/memory.rs index ac0f87d..e5bac50 100644 --- a/src/commands/memory.rs +++ b/src/commands/memory.rs @@ -340,11 +340,13 @@ fn build_class_aggregates( })?; let self_size = nodes[current_idx + self_size_offset]; - aggregates - .entry(name_ref.clone()) - .or_insert_with(ClassAggregate::new) - .nodes - .insert(id, self_size); + 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); + } current_idx += node_size; } From 2e24379d160584f779690b1a17a65281ceffb020 Mon Sep 17 00:00:00 2001 From: Aero Date: Tue, 7 Jul 2026 22:47:14 +0800 Subject: [PATCH 09/11] refactor(memory): optimize snapshot diff processing by modifying argument passing and reducing unnecessary lookups --- src/commands/memory.rs | 82 +++++++++++++++++++++--------------------- 1 file changed, 42 insertions(+), 40 deletions(-) diff --git a/src/commands/memory.rs b/src/commands/memory.rs index e5bac50..290e1bf 100644 --- a/src/commands/memory.rs +++ b/src/commands/memory.rs @@ -376,24 +376,27 @@ pub struct HeapSnapshotClassDiff { /// sizeDelta descending — matching DevTools' `#getSortedRawClassDiffs` so the /// summary list and the `--class-index` detail view share stable indices. fn diff_snapshots( - base: &std::collections::HashMap, - current: &std::collections::HashMap, + mut base: std::collections::HashMap, + current: std::collections::HashMap, ) -> Vec { let mut diffs: Vec = Vec::new(); - // 1. Process all classes in `current` (covers retained and added classes) + // 1. Process all classes in `current` (covers retained and added classes). + // Removing matched classes from `base` as we go means whatever remains in + // `base` after this loop is exactly the set of classes deleted entirely + // from `current` — no second `contains_key` pass needed. for (name, cur_agg) in current { - let base_agg = base.get(name); + let base_agg = base.remove(&name); // Upper-bounds: every current node could be new, every base node // could be gone. Avoids reallocation churn on large classes. let mut added_nodes: Vec<(u64, u64)> = Vec::with_capacity(cur_agg.nodes.len()); - let base_len = base_agg.map(|b| b.nodes.len()).unwrap_or(0); + let base_len = base_agg.as_ref().map(|b| b.nodes.len()).unwrap_or(0); let mut deleted_nodes: Vec<(u64, u64)> = Vec::with_capacity(base_len); let mut added_size: u64 = 0; let mut removed_size: u64 = 0; - if let Some(b) = base_agg { + if let Some(b) = &base_agg { for (id, size) in &cur_agg.nodes { if !b.nodes.contains_key(id) { added_nodes.push((*id, *size)); @@ -429,7 +432,7 @@ fn diff_snapshots( let size_delta = added_size as i64 - removed_size as i64; diffs.push(HeapSnapshotClassDiff { - class_name: name.clone(), + class_name: name, added_count, removed_count, count_delta, @@ -442,39 +445,38 @@ fn diff_snapshots( } } - // 2. Process classes only in `base` (covers entirely deleted classes) + // 2. Whatever remains in `base` was never matched in `current` — these + // are classes deleted entirely. for (name, base_agg) in base { - if !current.contains_key(name) { - let mut deleted_nodes: Vec<(u64, u64)> = Vec::with_capacity(base_agg.nodes.len()); - let mut removed_size: u64 = 0; - - for (id, size) in &base_agg.nodes { - deleted_nodes.push((*id, *size)); - removed_size += size; - } - - let removed_count = deleted_nodes.len(); + let mut deleted_nodes: Vec<(u64, u64)> = Vec::with_capacity(base_agg.nodes.len()); + let mut removed_size: u64 = 0; - // Sort deterministically by node id. - if removed_count > 1 { - deleted_nodes.sort_unstable_by_key(|(id, _)| *id); - } + for (id, size) in &base_agg.nodes { + deleted_nodes.push((*id, *size)); + removed_size += size; + } - let count_delta = -(removed_count as i64); - let size_delta = -(removed_size as i64); + let removed_count = deleted_nodes.len(); - diffs.push(HeapSnapshotClassDiff { - class_name: name.clone(), - added_count: 0, - removed_count, - count_delta, - added_size: 0, - removed_size, - size_delta, - added_nodes: Vec::new(), - deleted_nodes, - }); + // Sort deterministically by node id. + if removed_count > 1 { + deleted_nodes.sort_unstable_by_key(|(id, _)| *id); } + + let count_delta = -(removed_count as i64); + let size_delta = -(removed_size as i64); + + diffs.push(HeapSnapshotClassDiff { + class_name: name, + added_count: 0, + removed_count, + count_delta, + added_size: 0, + removed_size, + size_delta, + added_nodes: Vec::new(), + deleted_nodes, + }); } diffs.sort_by(|a, b| { @@ -622,7 +624,7 @@ pub async fn compare_heapsnapshots_offline( let current_val = parse_snapshot_file(¤t_owned)?; build_class_aggregates(¤t_val)? }; - Ok(diff_snapshots(&base_agg, ¤t_agg)) + Ok(diff_snapshots(base_agg, current_agg)) }) .await .map_err(|e| anyhow!("Failed to execute blocking snapshot diff: {e}"))??; @@ -856,7 +858,7 @@ mod tests { ])) .unwrap(); - let diffs = diff_snapshots(&base, ¤t); + let diffs = diff_snapshots(base, current); // Sorted by sizeDelta desc: Window(500) > Map(50) > String(-50) assert_eq!(diffs.len(), 3); @@ -891,7 +893,7 @@ mod tests { // Map appears in both with identical nodes → no diff row at all. let base = build_class_aggregates(&make_snapshot(&[(1, "Map", 100)])).unwrap(); let current = build_class_aggregates(&make_snapshot(&[(1, "Map", 100)])).unwrap(); - let diffs = diff_snapshots(&base, ¤t); + let diffs = diff_snapshots(base, current); assert!(diffs.is_empty()); } @@ -900,7 +902,7 @@ mod tests { let base = build_class_aggregates(&make_snapshot(&[(1, "Old", 80), (2, "Old", 40)])).unwrap(); let current = build_class_aggregates(&make_snapshot(&[])).unwrap(); - let diffs = diff_snapshots(&base, ¤t); + let diffs = diff_snapshots(base, current); assert_eq!(diffs.len(), 1); assert_eq!(diffs[0].class_name, "Old"); assert_eq!(diffs[0].removed_count, 2); @@ -915,7 +917,7 @@ mod tests { build_class_aggregates(&make_snapshot(&[(1, "Map", 100), (2, "String", 50)])).unwrap(); let current = build_class_aggregates(&make_snapshot(&[(3, "Window", 500), (4, "Map", 150)])).unwrap(); - let diffs = diff_snapshots(&base, ¤t); + let diffs = diff_snapshots(base, current); let summary = format_class_diff_summary(&diffs, crate::format::OutputFormat::Text).unwrap(); // First data row (idx 0) should be the largest sizeDelta — Window. From f1a4712dee062566210577efe727677cfc5bb8ec Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 8 Jul 2026 21:11:31 +0800 Subject: [PATCH 10/11] refactor(memory): streamline class aggregate handling and improve CSV escaping in snapshot diff functions --- src/commands/memory.rs | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/src/commands/memory.rs b/src/commands/memory.rs index 290e1bf..8314566 100644 --- a/src/commands/memory.rs +++ b/src/commands/memory.rs @@ -314,7 +314,7 @@ fn build_class_aggregates( if node_size == 0 { bail!("Invalid snapshot: node_fields schema is empty"); } - if !nodes.len().is_multiple_of(node_size) { + if nodes.len() % node_size != 0 { bail!( "Corrupt snapshot: nodes array length ({}) is not a multiple of node_size ({}); \ the file is truncated or malformed", @@ -340,13 +340,11 @@ fn build_class_aggregates( })?; let self_size = nodes[current_idx + self_size_offset]; - 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); current_idx += node_size; } @@ -490,11 +488,11 @@ fn diff_snapshots( /// CSV-escape a class name the same way `format_node_details` escapes node /// names — names like `(closure)` are safe, but `(string, joined)` would break /// naive CSV parsing. -fn csv_escape(s: &str) -> String { +fn csv_escape(s: &str) -> std::borrow::Cow<'_, str> { if s.contains(',') || s.contains('"') || s.contains('\n') || s.contains('\r') { - format!("\"{}\"", s.replace('"', "\"\"")) + std::borrow::Cow::Owned(format!("\"{}\"", s.replace('"', "\"\""))) } else { - s.to_string() + std::borrow::Cow::Borrowed(s) } } @@ -504,12 +502,14 @@ pub fn format_class_diff_summary( format: crate::format::OutputFormat, ) -> Result { if format.is_text() { + use std::fmt::Write; let mut out = String::new(); out.push_str( "idx,className,addedCount,removedCount,countDelta,addedSize,removedSize,sizeDelta\n", ); for (i, d) in diffs.iter().enumerate() { - out.push_str(&format!( + let _ = write!( + out, "{},{},{},{},{},{},{},{}\n", i, csv_escape(&d.class_name), @@ -519,7 +519,7 @@ pub fn format_class_diff_summary( d.added_size, d.removed_size, d.size_delta, - )); + ); } Ok(out) } else { @@ -554,8 +554,10 @@ pub fn format_class_diff_detail( format: crate::format::OutputFormat, ) -> Result { if format.is_text() { + use std::fmt::Write; let mut out = String::new(); - out.push_str(&format!( + let _ = write!( + out, "idx:{},className:{},addedCount:{},removedCount:{},countDelta:{},addedSize:{},removedSize:{},sizeDelta:{}\n", idx, csv_escape(&diff.class_name), @@ -565,13 +567,13 @@ pub fn format_class_diff_detail( diff.added_size, diff.removed_size, diff.size_delta, - )); + ); out.push_str("\nop,nodeId,selfSize\n"); for (id, size) in &diff.added_nodes { - out.push_str(&format!("+,{},{}\n", id, size)); + let _ = write!(out, "+,{},{}\n", id, size); } for (id, size) in &diff.deleted_nodes { - out.push_str(&format!("-,{},{}\n", id, size)); + let _ = write!(out, "-,{},{}\n", id, size); } Ok(out) } else { From 093bfa67ba4f8be9f1f0f9012cf2dcaa28036898 Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 8 Jul 2026 22:11:17 +0800 Subject: [PATCH 11/11] refactor(memory): optimize class aggregate insertion to reduce allocations in build_class_aggregates function --- src/commands/memory.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/commands/memory.rs b/src/commands/memory.rs index 8314566..374e73f 100644 --- a/src/commands/memory.rs +++ b/src/commands/memory.rs @@ -340,11 +340,16 @@ fn build_class_aggregates( })?; let self_size = nodes[current_idx + self_size_offset]; - aggregates - .entry(name_ref.clone()) - .or_insert_with(ClassAggregate::new) - .nodes - .insert(id, self_size); + // Only clone the name on the cold insert path. `entry()` would force a + // clone per node (millions of allocations on large snapshots); the + // get_mut/insert split keeps the hot lookup path allocation-free. + 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); + } current_idx += node_size; }