feat: support png, jpg, jpeg, gif for icon auto-detection#509
feat: support png, jpg, jpeg, gif for icon auto-detection#509
Conversation
The icon upload fallback only checked for icon.png in the project root. This adds assets/icon.png as the first fallback path, matching the convention used by slack-samples template repos and the existing auto-detection in slack_yaml.go.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #509 +/- ##
==========================================
- Coverage 71.17% 71.17% -0.01%
==========================================
Files 222 222
Lines 18678 18682 +4
==========================================
+ Hits 13294 13296 +2
- Misses 4201 4206 +5
+ Partials 1183 1180 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
changed the title
Extract resolveIconPath to centralize icon fallback logic across
Install and InstallLocalApp. Search for icon.{png,jpg,jpeg,gif}
in assets/ then project root, matching all formats the API accepts.
srtaalej
changed the title
mwbrooks
left a comment
mwbrooks
left a comment
There was a problem hiding this comment.
🙇🏻 Thanks for putting this PR together @srtaalej!
✏️ I've left a couple feedback questions and suggestions, but overall this is looking good.
❓ Curious, have you tested this on both Deno/ROSI and Bolt using multiple image formats for each?
❓ This implementation is using manifest.json to define the icon. Personally, I like how clean this is but it does put us in a pickle if someone tries to copy & paste the manifest into App Settings or use our manifest-schema package for validation. Are we planning to explore defining the icon in config.json?
|
|
||
| const additionalManifestInfoNotice = "App manifest contains some components that may require additional information" | ||
|
|
||
| var supportedIconExtensions = []string{".png", ".jpg", ".jpeg", ".gif"} |
There was a problem hiding this comment.
question: Just curious, have you tested all of these images types? I just want to make sure that the API supports each.
| for _, dir := range []string{"assets", "."} { | ||
| for _, ext := range supportedIconExtensions { | ||
| candidate := filepath.Join(dir, "icon"+ext) | ||
| if _, err := fs.Stat(candidate); err == nil { | ||
| return candidate | ||
| } | ||
| } |
There was a problem hiding this comment.
question: It looks like the implementation will attempt to find the first image that matches the file extensions and use it when there is no path defined?
There was a problem hiding this comment.
yes thats the fallback. it doesnt have to be that way but im worried the assets/icon.png convention is a little strict since the image also has to be named icon
|
|
||
| var supportedIconExtensions = []string{".png", ".jpg", ".jpeg", ".gif"} | ||
|
|
||
| func resolveIconPath(fs afero.Fs, manifestIcon string) string { |
There was a problem hiding this comment.
suggestion: Thoughts on moving this to internal/icon/icon.go or maybe internal/config/icon.go. My preference is the first to avoid cluttering our already messy package config.
| for _, ext := range supportedIconExtensions { | ||
| candidate := filepath.Join(wd, "assets", "icon"+ext) | ||
| if _, err := os.Stat(candidate); !os.IsNotExist(err) { | ||
| sy.Icon = filepath.Join("assets", "icon"+ext) | ||
| break | ||
| } |
There was a problem hiding this comment.
suggestion: Since we're using this loop logic in 2 places, I think it would make sense to implement it as a function in internal/icon/icon.go.
i have tested with both Deno and Bolt - will update once all format types are tested! |
2 participants
Summary
resolveIconPathfunction to centralize icon fallback logic used by bothInstallandInstallLocalAppicon.{png,jpg,jpeg,gif}inassets/first, then project root — matching all formats the API acceptshasValidIconPath()inslack_yaml.goto also recognize jpg, jpeg, gif inassets/Test plan
Unit tests
Manual testing
1. Build the branch
git fetch origin ale-icon-assets-fallback git checkout ale-icon-assets-fallback go build -o bin/slack .2. Set up a test app (or use an existing bolt app)
3. Test assets/ auto-detection with different formats
Place an icon in
assets/and run:Verify:
Updated app icon: assets/icon.pngRepeat with
assets/icon.jpg(removeicon.pngfirst) — should pick up the jpg.4. Test project root fallback
Remove any icon from
assets/. Placeicon.pngin the project root:rm assets/icon.* cp /path/to/icon.png icon.png ../bin/slack run -e set-iconVerify:
Updated app icon: icon.png5. Test priority (assets/ wins over root)
Place icons in both locations:
Verify:
Updated app icon: assets/icon.png(assets/ takes precedence)6. Test manifest takes precedence
Set
icon: "custom/my-icon.png"in the manifest. Run again — should use the manifest path, not auto-detection.