Skip to content

chore(preprod): Add AssetCatalog Parsing support for Multisize sets#2879

Merged
NicoHinderling merged 1 commit intomasterfrom
add-support-for-multisize-set-parsing
Oct 28, 2025
Merged

chore(preprod): Add AssetCatalog Parsing support for Multisize sets#2879
NicoHinderling merged 1 commit intomasterfrom
add-support-for-multisize-set-parsing

Conversation

@NicoHinderling
Copy link
Copy Markdown
Contributor

Certain assets weren't getting picked up for the Firefox app, and the ones missing appeared to be multisize sets, like this

    {
        "vector": false,
        "type": 0,
        "imageId": "42D31467-E63B-49D8-8D9B-1B608059E344",
        "size": 236,
        "height": 1024,
        "name": "AppIcon_Alt_Color_Purple",
        "filename": "AppIcon_Alt_Color_Purple",
        "idiom": "phone",
        "width": 1024
    },

They won't have a a file extension either it seems (ex. Assets.car/AppIcon_Alt_Color_Purple). So we'll go ahead and save them as a (default type) png

@NicoHinderling NicoHinderling requested review from a team as code owners October 28, 2025 16:59
if fileExtension != "svg", let unslicedImage = unslicedImage {
let format = fileExtension.isEmpty ? "png" : fileExtension
images[imageId] = (cgImage: unslicedImage, format: format)
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevor-e #2859 (comment)

I know you previously mentioned that we should be defensive and not include images missing a file type but in this case we want to include some that indeed don't have one

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep the old defensive logic if we restructure this a bit:

if 
  unslicedImage == nil && className == "_CUIThemeMultisizeImageSetRendition", 
  let result = findImageForMultisizeSet(rendition, key, assetKeys, structuredThemeStore) {
    unslicedImage = result.image
    width = result.width
    height = result.height
    images[imageId] = (cgImage: unslicedImage, format: "png")
} else !fileExtension.isEmpty && fileExtension != "svg", let unslicedImage = unslicedImage {
    images[imageId] = (cgImage: unslicedImage, format: fileExtension)
}

@NicoHinderling NicoHinderling force-pushed the add-support-for-multisize-set-parsing branch 2 times, most recently from a43cca2 to 0ac1433 Compare October 28, 2025 17:02
if fileExtension != "svg", let unslicedImage = unslicedImage {
let format = fileExtension.isEmpty ? "png" : fileExtension
images[imageId] = (cgImage: unslicedImage, format: format)
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep the old defensive logic if we restructure this a bit:

if 
  unslicedImage == nil && className == "_CUIThemeMultisizeImageSetRendition", 
  let result = findImageForMultisizeSet(rendition, key, assetKeys, structuredThemeStore) {
    unslicedImage = result.image
    width = result.width
    height = result.height
    images[imageId] = (cgImage: unslicedImage, format: "png")
} else !fileExtension.isEmpty && fileExtension != "svg", let unslicedImage = unslicedImage {
    images[imageId] = (cgImage: unslicedImage, format: fileExtension)
}


let isVector = type == 9
let (width, height, unslicedImage) = resolveImageDimensions(rendition, isVector)
var (width, height, unslicedImage) = resolveImageDimensions(rendition, isVector)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this mutable and then later on depending on unslicedImage to be nil for the findImageForMultisizeSet() feels weird to me. Is there something more stable we can check to see if it's a multisize set, like the type? Similar to how we do let isVector = type == 9

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I figured this would be structured like:

if isMultisizeImageSet {
 // logic
} else {
  // old logic
}

@NicoHinderling NicoHinderling force-pushed the add-support-for-multisize-set-parsing branch from 0ac1433 to 2ffddb5 Compare October 28, 2025 18:02
@NicoHinderling NicoHinderling merged commit 83d9b83 into master Oct 28, 2025
27 checks passed
@NicoHinderling NicoHinderling deleted the add-support-for-multisize-set-parsing branch October 28, 2025 18:39
NicoHinderling added a commit that referenced this pull request Oct 29, 2025
Previously in #2879 I added
the ability for us to collect multiset icons and have them included in
the ParsedAssets folder / assets.json.

There were two problems:
1. we were only picking up the primary image of the set. For example,
for a multiset called `AppIcon_Alt_Color_Red`, we'd only pick up the
first image (`red-light-min.png`) when really it contains three, all of
which we want to consider when analyzing for insights
2. We were saving the multiset image as the multiset object, rather than
as an image, which led us to have to [add some undesirable logic in the
launchpad
service](https://github.com/getsentry/launchpad/pull/437/files#r2470576784)

With these changes, we now get all the images in the multisets and we
save them as images
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants