feat(snapshots): Add CI image export via SNAPSHOTS_EXPORT_DIR env var (EME-931)#252
Merged
cameroncooke merged 11 commits intomainfrom Apr 8, 2026
Merged
feat(snapshots): Add CI image export via SNAPSHOTS_EXPORT_DIR env var (EME-931)#252cameroncooke merged 11 commits intomainfrom
cameroncooke merged 11 commits intomainfrom
Conversation
3a8c305 to
6712190
Compare
Underscores are valid filename characters and common in Swift type names. Previously they were treated as unsafe and collapsed with adjacent non-kept characters. Now they are preserved as-is. Update test to verify unsafe char collapsing separately from underscore preservation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The _shared static var was read/written without any isolation while hasDrained used NSLock protection. Both production callers (discoverPreviews, testPreview) are already @mainactor, so this makes the contract explicit and prevents future data races. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The init already creates the directory or calls preconditionFailure, so the directory is guaranteed to exist. The guard also had the side-effect of skipping the drain while operations could still be in-flight. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace manual semaphore+barrier pattern with the built-in OperationQueue API that does the same thing. Blocks the calling thread until all enqueued operations complete. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move the color scheme reset into a defer block so the override is always cleared even if the render loop exits early due to an unexpected error. Previously the reset only ran after normal loop completion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The grouping key in discoverPreviews and testPreview used fileID ?? typeName, skipping the typeDisplayName fallback that canonicalGroup uses. This could cause filename collisions when a PreviewProvider has no fileId but a typeDisplayName differing from typeName. Extract canonicalGroup to accept raw parameters and make it internal so both SnapshotTest and the coordinator share the same fallback chain: fileId -> typeDisplayName -> typeName. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move filename sanitization to the point where baseFileName is created in testPreview, so SnapshotContext.baseFileName and the sidecar image_file_name field are always consistent. Previously baseFileName stored the raw unsanitized name while image_file_name was sanitized, creating a confusing mismatch for sidecar consumers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the branch-added color scheme override API from SnapshotTest. Render each preview once using its natural appearance again so snapshot names and exports no longer fan out by forced light and dark passes. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Reject snapshot renders above the 40 million pixel upload limit before writing PNGs to disk. This prevents extremely tall expanding snapshots from producing files that the upload CLI will reject. Co-Authored-By: OpenAI Codex <noreply@openai.com>
NicoHinderling
approved these changes
Apr 7, 2026
noahsmartin
reviewed
Apr 7, 2026
| #endif | ||
| Self.renderingStrategy = strategy | ||
| } | ||
| let strategy = Self.renderingStrategy! |
Member
There was a problem hiding this comment.
Can we avoid force unwrap? Pretty sure the pattern from before can e used here too in a way that the compiler would be able to tell it should always be non-null
noahsmartin
reviewed
Apr 7, 2026
| ) | ||
| let colorSchemeValue = result.colorScheme?.stringValue | ||
|
|
||
| let context = SnapshotContext( |
Member
There was a problem hiding this comment.
Should we move this into the if statement below? Doesn't seem to be used if CI exports aren't enabled
noahsmartin
reviewed
Apr 7, 2026
| /// - Returns: An array of `DiscoveredPreview` objects representing the found previews. | ||
| @MainActor | ||
| override class func discoverPreviews() -> [DiscoveredPreview] { | ||
| _ = SnapshotCIExportCoordinator.sharedIfEnabled() |
Member
There was a problem hiding this comment.
This seems like a code smell, why assign to _, is there a side effect of calling sharedIfEnabled that needs to happen here?
Can we make SnapshotCIExportCoordinator not a singleton and instead just have it be a static property of the SnapshotTest class?
Contributor
Author
There was a problem hiding this comment.
Yes we can do that and I agree.
- Remove force unwrap of renderingStrategy by restoring if-let pattern - Scope SnapshotContext creation to CI export path only - Replace singleton with static factory + SnapshotTest-owned property Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This branch adds filesystem-based snapshot export for CI flows and makes exported snapshot naming and metadata deterministic across the preview scenarios we cover.
Summary
SNAPSHOTS_EXPORT_DIRSnapshotCIExportCoordinatorgroupanddisplay_namemetadatabaseFileNameat construction time so attachments and exports use the same stable namesNaming and metadata behavior
Filename and sidecar naming now cover the main preview cases explicitly:
line-<n>when neededPreviewProvidervariants fall back to the preview indexgroupresolves asfileId -> typeDisplayName -> typeNamedisplay_nameresolves as explicit preview display name ->At line #N-> preview indexArtifacts:
Images and sidecar files from HackerNews local run:
HackerNewsSnapshots.zip
Latest workflow run https://github.com/EmergeTools/hackernews/actions/runs/23940370307/job/69825189739?pr=780
Sentry Snapshots Viewer: https://sentry.sentry.io/preprod/snapshots/180875/
Example
HackerNews_FeedScreen.swift_161_Has_posts.png:
HackerNews_FeedScreen.swift_161_Has_posts.json:
{ "baseFileName" : "HackerNews_FeedScreen.swift_161_Has_posts", "display_name" : "Has posts", "fileId" : "HackerNews\/FeedScreen.swift", "group" : "HackerNews\/FeedScreen.swift", "image_file_name" : "HackerNews_FeedScreen.swift_161_Has_posts", "line" : 161, "orientation" : "portrait", "previewDisplayName" : "Has posts", "previewId" : "0", "previewIndex" : 0, "simulatorDeviceName" : "iPhone 17 Pro Max", "simulatorModelIdentifier" : "iPhone18,2", "testName" : "-[HackerNewsSnapshotTest portrait-Feed Screen-0-19]", "typeDisplayName" : "Feed Screen", "typeName" : "HackerNews.$s10HackerNews0021FeedScreenswift_IfFDefMX160_0_33_2B34B1DE919BF62EDDE40FEC34EF23D3Ll7PreviewfMf2_15PreviewRegistryfMu_" }Same snapshot in Sentry UI:
Testing
Run tests with
TEST_RUNNER_SNAPSHOTS_EXPORT_DIRset:View files
Upload to Sentry
Linear
EME-931