Upload image metadata json data as part of snapshots job#3163
Upload image metadata json data as part of snapshots job#3163
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- Upload image metadata json data as part of snapshots job ([#3163](https://github.com/getsentry/sentry-cli/pull/3163))If none of the above apply, you can opt out of this check by adding |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
src/commands/build/snapshots.rs
Outdated
| let matched = manifest_entries | ||
| .values_mut() | ||
| .find(|entry| entry.image_file_name == json_image.image_file_name); |
There was a problem hiding this comment.
Bug: When multiple images share the same filename in different directories, metadata from the manifest is only applied to the first one found, silently ignoring the others.
Severity: MEDIUM
Suggested Fix
Modify the matching logic to either use the full relative path for unique identification or iterate through and apply metadata to all images that match the given filename, not just the first one.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/commands/build/snapshots.rs#L341-L343
Potential issue: The `merge_manifest_metadata` function matches images from a JSON
manifest using only the filename, not the full relative path. If multiple images in
different subdirectories share the same filename (e.g., `ios/screenshot.png` and
`android/screenshot.png`), the logic uses `.find()` which only returns the first match.
As a result, metadata from the manifest is only applied to the first image found, and
any other images with the same filename are silently ignored, leading to inconsistent
metadata application without any warning or error.
Did we get this right? 👍 / 👎 to inform future reviews.
src/api/data_types/snapshots.rs
Outdated
| pub struct SnapshotManifestFile { | ||
| pub images: HashMap<String, ImageMetadata>, |
There was a problem hiding this comment.
Bug: The JSON manifest requires redundant width and height fields. If omitted, the entire manifest is silently ignored, and no metadata is applied.
Severity: HIGH
Suggested Fix
Make the width and height fields in the ImageMetadata struct optional for deserialization. This can be achieved by changing their types to Option<u32> or by using #[serde(default)].
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/api/data_types/snapshots.rs#L37-L38
Potential issue: The `ImageMetadata` struct, used for deserializing the JSON manifest,
requires `width` and `height` fields. However, these values are never used, as the
application extracts dimensions directly from the image files. If a user omits these
redundant fields from the JSON manifest, deserialization fails, and the entire manifest
is silently ignored with only a warning log. This prevents any intended metadata, such
as `display_name`, from being applied, making the feature appear broken to the user.
Did we get this right? 👍 / 👎 to inform future reviews.
| .unwrap_or_default() | ||
| .to_string_lossy() | ||
| .into_owned(); | ||
| let display_name = display_names.get(&image_file_name).cloned(); |
There was a problem hiding this comment.
Bug: The code uses only the filename to look up an image's display name, but the manifest is keyed by the full relative path, causing lookups for images in subdirectories to fail silently.
Severity: MEDIUM
Suggested Fix
Use the full image.relative_path as a string when performing the lookup in the display_names map. Do not call .file_name() on the path, as this discards the subdirectory information needed to find a match.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/commands/build/snapshots.rs#L251-L254
Potential issue: When discovering images, the code correctly identifies a relative path
like `ui/button.png`. However, to look up the corresponding `display_name` from the
manifest, it extracts only the filename (`button.png`) using `.file_name()`. The
manifest data, stored in the `display_names` map, is keyed by the full relative path
provided by the user. This key mismatch causes the lookup to silently fail. As a result,
the `display_name` is omitted from the uploaded metadata for any images organized in
subdirectories, even when correctly specified by the user.
1 participant
Description
Issues
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.