fix: clean up reporter diff artifacts on Snap#delete!#173
Conversation
…nap#delete! Snap#delete! only removed .png and .base.png but left .diff.png, .base.diff.png, and .heatmap.diff.png orphaned. Also Reporters::Default #clean_tmp_files missed heatmap cleanup. Each test run leaked ~30 files into tmp/ that accumulated indefinitely across runs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reviewer's GuideExtends snapshot and reporter cleanup logic so that all diff-related artifacts (including heatmap diffs) are removed when snapshots are deleted or temporary files are cleaned, and adds tests to lock in this behavior and prevent tmp/ bloat. Sequence diagram for Snap delete! diff artifact cleanupsequenceDiagram
participant TestRunner
participant SnapManager
participant Snap
participant FileSystem
TestRunner->>SnapManager: delete_snap(full_name)
SnapManager->>Snap: delete!()
Snap->>FileSystem: delete(path) if exist
Snap->>FileSystem: delete(base_path) if exist
Snap->>Snap: cleanup_diff_artifacts!()
Snap->>FileSystem: delete(path_with_diff_ext) if exist
Snap->>FileSystem: delete(path_with_heatmap_diff_ext) if exist
Snap->>FileSystem: delete(base_path_with_diff_ext) if exist
Snap->>Snap: cleanup_attempts!()
Snap->>FileSystem: delete(attempt_paths[]) if exist
Class diagram for updated Snap and Default reporter cleanup behaviorclassDiagram
class CapybaraScreenshotDiff_Snap {
+initialize(full_name, format, manager)
+delete!()
+cleanup_attempts!()
+find_attempts_paths()
-cleanup_diff_artifacts!()
path
base_path
manager
}
class Capybara_Screenshot_Diff_Reporters_Default {
+generate()
+clean_tmp_files()
annotated_base_image_path
annotated_image_path
heatmap_diff_path
}
CapybaraScreenshotDiff_Snap --> Capybara_Screenshot_Diff_Reporters_Default : generates_diff_artifacts_for
Flow diagram for reporter tmp file cleanup including heatmap diffflowchart TD
A[start clean_tmp_files] --> B{annotated_base_image_path.exist?}
B -->|yes| C[annotated_base_image_path.unlink]
B -->|no| D[next]
C --> D
D --> E{annotated_image_path.exist?}
E -->|yes| F[annotated_image_path.unlink]
E -->|no| G[next]
F --> G
G --> H{heatmap_diff_path.exist?}
H -->|yes| I[heatmap_diff_path.unlink]
H -->|no| J[end]
I --> J[end clean_tmp_files]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 20 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The diff artifact filename patterns are now duplicated between
Reporters::DefaultandSnap#cleanup_diff_artifacts!; consider centralizing the naming logic (e.g., via a shared helper or constants) so future changes to artifact naming don’t silently drift out of sync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The diff artifact filename patterns are now duplicated between `Reporters::Default` and `Snap#cleanup_diff_artifacts!`; consider centralizing the naming logic (e.g., via a shared helper or constants) so future changes to artifact naming don’t silently drift out of sync.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Snap#delete!now removes.diff.png,.base.diff.png, and.heatmap.diff.pngartifacts created byReporters::Default— previously these were orphaned on every test runReporters::Default#clean_tmp_filesnow also removes.heatmap.diff.png(was missing)tmp/bloat from ~30 leaked files per test run (accumulating indefinitely) to 0Context
Each unit test run left reporter diff artifacts behind because:
Snap#delete!only cleaned.pngand.base.pngbut not the 3 reporter-generated diff filesReporters::Default#clean_tmp_filescleaned 2 of 3 artifact types (missed heatmap)Time.now.nsec, so old artifacts never matched for cleanupTest plan
SnapManagerTest#cleanup! removes diff artifacts created by reportersDefaultTest#clean_tmp_files removes heatmap diff along with other diff artifactstmp/stays at 2-3 files across multiple runs (was 31→56→80+)DEBUG/persist_comparisons?escape hatch still works🤖 Generated with Claude Code
Summary by Sourcery
Ensure snapshot deletion and reporter cleanup remove all generated diff artifacts to prevent tmp directory bloat.
Bug Fixes:
Tests: