Skip to content

Commit 1c7b0cc

Browse files
fix(snapshots): Address review bot findings for diff command
Use platform-specific binary name for odiff cache path so Windows gets the required .exe extension. Always output diff masks with .png extension regardless of input format. Include added/removed counts in --fail-on-diff exit condition. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 0d88866 commit 1c7b0cc

2 files changed

Lines changed: 19 additions & 4 deletions

File tree

src/commands/snapshots/diff.rs

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
200200
Ok(false) => {}
201201
}
202202

203-
let mask_path = output_dir.join(rel_path);
203+
let mask_path = output_dir.join(rel_path).with_extension("png");
204204
if let Some(parent) = mask_path.parent() {
205205
fs::create_dir_all(parent)?;
206206
}
@@ -331,14 +331,28 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
331331
"\nSummary: {total} total, {changed_count} changed, {unchanged_count} unchanged, {added_count} added, {removed_count} removed, {errored_count} errored",
332332
);
333333

334-
if fail_on_diff && (changed_count > 0 || errored_count > 0) {
334+
if fail_on_diff
335+
&& (changed_count > 0 || errored_count > 0 || added_count > 0 || removed_count > 0)
336+
{
335337
let mut parts = Vec::new();
336338
if changed_count > 0 {
337339
parts.push(format!(
338340
"{changed_count} image{} differed from baseline",
339341
if changed_count == 1 { "" } else { "s" }
340342
));
341343
}
344+
if added_count > 0 {
345+
parts.push(format!(
346+
"{added_count} image{} added",
347+
if added_count == 1 { " was" } else { "s were" }
348+
));
349+
}
350+
if removed_count > 0 {
351+
parts.push(format!(
352+
"{removed_count} image{} removed",
353+
if removed_count == 1 { " was" } else { "s were" }
354+
));
355+
}
342356
if errored_count > 0 {
343357
parts.push(format!(
344358
"{errored_count} image{} errored during comparison",

src/utils/odiff/binary.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ fn cache_dir() -> Result<PathBuf> {
2828
fn download_and_extract() -> Result<PathBuf> {
2929
let binary_name = platform_binary_name()?;
3030
let cache = cache_dir()?;
31-
let binary_path = cache.join("odiff");
31+
let binary_path = cache.join(binary_name);
3232

3333
fs::create_dir_all(&cache).context("Failed to create odiff cache directory")?;
3434

@@ -99,8 +99,9 @@ pub fn ensure_binary() -> Result<PathBuf> {
9999
return Ok(system_path);
100100
}
101101

102+
let binary_name = platform_binary_name()?;
102103
let cache = cache_dir()?;
103-
let cached_binary = cache.join("odiff");
104+
let cached_binary = cache.join(binary_name);
104105
if cached_binary.exists() {
105106
info!("Using cached odiff at {}", cached_binary.display());
106107
return Ok(cached_binary);

0 commit comments

Comments
 (0)