Skip to content

Commit 5cd1954

Browse files
fix(snapshots): Handle odiff error responses without server restart
Application-level odiff errors (e.g., corrupt images) are valid protocol responses — the server remains healthy. Separate them from transport errors to avoid unnecessary process restarts. Also fix layout reason string to match odiff server protocol ("layout-diff" not "layout"). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent b7684a3 commit 5cd1954

2 files changed

Lines changed: 42 additions & 36 deletions

File tree

src/commands/snapshots/diff.rs

Lines changed: 40 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -206,39 +206,49 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {
206206
}
207207

208208
match server.compare(&base_file, &head_file, &mask_path) {
209+
Ok(response) if response.error.is_some() => {
210+
errored_count += 1;
211+
images.push(ImageResult {
212+
name,
213+
status: "error".to_owned(),
214+
diff_percentage: None,
215+
diff_pixel_count: None,
216+
diff_mask_path: None,
217+
error: response.error,
218+
});
219+
}
220+
Ok(response) if response.is_match => {
221+
let _ = fs::remove_file(&mask_path);
222+
unchanged_count += 1;
223+
images.push(ImageResult {
224+
name,
225+
status: "unchanged".to_owned(),
226+
diff_percentage: response.diff_percentage,
227+
diff_pixel_count: response.diff_count,
228+
diff_mask_path: None,
229+
error: None,
230+
});
231+
}
209232
Ok(response) => {
210-
if response.is_match {
233+
let status = match response.reason.as_deref() {
234+
Some("layout-diff") => "layout_changed",
235+
_ => "changed",
236+
};
237+
changed_count += 1;
238+
let mask = if status == "layout_changed" {
211239
let _ = fs::remove_file(&mask_path);
212-
unchanged_count += 1;
213-
images.push(ImageResult {
214-
name,
215-
status: "unchanged".to_owned(),
216-
diff_percentage: response.diff_percentage,
217-
diff_pixel_count: response.diff_count,
218-
diff_mask_path: None,
219-
error: None,
220-
});
240+
None
221241
} else {
222-
let status = match response.reason.as_deref() {
223-
Some("layout") => "layout_changed",
224-
_ => "changed",
225-
};
226-
changed_count += 1;
227-
let mask = if status == "layout_changed" {
228-
let _ = fs::remove_file(&mask_path);
229-
None
230-
} else {
231-
Some(path_as_url(&mask_path))
232-
};
233-
images.push(ImageResult {
234-
name,
235-
status: status.to_owned(),
236-
diff_percentage: response.diff_percentage,
237-
diff_pixel_count: response.diff_count,
238-
diff_mask_path: mask,
239-
error: None,
240-
});
241-
}
242+
Some(path_as_url(&mask_path))
243+
};
244+
images.push(ImageResult {
245+
name,
246+
status: status.to_owned(),
247+
diff_percentage: response.diff_percentage,
248+
diff_pixel_count: response.diff_count,
249+
diff_mask_path: mask,
250+
error: None,
251+
});
242252
}
243253
Err(err) => {
244254
errored_count += 1;

src/utils/odiff/server.rs

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -158,10 +158,6 @@ impl OdiffServer {
158158
);
159159
}
160160

161-
if let Some(ref err) = response.error {
162-
bail!("odiff error comparing {}: {err}", base.display());
163-
}
164-
165161
Ok(response)
166162
}
167163
}
@@ -224,11 +220,11 @@ mod tests {
224220

225221
#[test]
226222
fn test_response_deserialization_layout() {
227-
let json = r#"{"requestId":3,"match":false,"reason":"layout"}"#;
223+
let json = r#"{"requestId":3,"match":false,"reason":"layout-diff"}"#;
228224
let response: OdiffResponse =
229225
serde_json::from_str(json).expect("deserialization should succeed");
230226
assert!(!response.is_match);
231-
assert_eq!(response.reason.as_deref(), Some("layout"));
227+
assert_eq!(response.reason.as_deref(), Some("layout-diff"));
232228
assert!(response.diff_count.is_none());
233229
}
234230

0 commit comments

Comments
 (0)