Skip to content

Commit d599052

Browse files
committed
fix: address review feedback for icon path resolution
- Update constant reference to slackCLIAppIconPathEnv - Stop falling through to manifest/icon.png when env var file is missing - Add file existence check for manifest icon values - Update tests for new behavior
1 parent d7e7ccd commit d599052

3 files changed

Lines changed: 13 additions & 10 deletions

File tree

internal/config/dotenv.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ func (c *Config) LoadEnvironmentVariables() error {
4646
}
4747

4848
// Load app icon path from environment variables
49-
var appIconPath = strings.TrimSpace(c.os.Getenv(slackAppIconPathEnv))
49+
var appIconPath = strings.TrimSpace(c.os.Getenv(slackCLIAppIconPathEnv))
5050
if appIconPath != "" {
5151
c.AppIconPathFlag = appIconPath
5252
}

internal/pkg/apps/install.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -647,9 +647,12 @@ func resolveIconPath(ctx context.Context, clients *shared.ClientFactory, manifes
647647
}
648648
clients.IO.PrintDebug(ctx, "SLACK_CLI_APP_ICON_PATH file not found: %s", envIconPath)
649649
_, _ = clients.IO.WriteOut().Write([]byte(style.SectionSecondaryf("Warning: icon path from SLACK_CLI_APP_ICON_PATH not found: %s", envIconPath)))
650+
return ""
650651
}
651652
if manifestIcon != "" {
652-
return manifestIcon
653+
if _, err := os.Stat(manifestIcon); !os.IsNotExist(err) {
654+
return manifestIcon
655+
}
653656
}
654657
if _, err := os.Stat("icon.png"); !os.IsNotExist(err) {
655658
return "icon.png"

internal/pkg/apps/install_test.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1771,25 +1771,25 @@ func Test_resolveIconPath(t *testing.T) {
17711771
setupFiles: func(t *testing.T, dir string) {},
17721772
expected: "",
17731773
},
1774-
"env var file not found falls back to manifest": {
1774+
"env var file not found returns empty": {
17751775
envIconPath: "missing-icon.png",
17761776
manifestIcon: "manifest-icon.png",
17771777
setupFiles: func(t *testing.T, dir string) {
17781778
require.NoError(t, os.WriteFile(filepath.Join(dir, "manifest-icon.png"), []byte("img"), 0o644))
17791779
},
1780-
expected: "manifest-icon.png",
1780+
expected: "",
17811781
},
1782-
"env var file not found falls back to icon.png": {
1783-
envIconPath: "missing-icon.png",
1782+
"manifest icon file not found falls back to icon.png": {
1783+
manifestIcon: "missing-manifest-icon.png",
17841784
setupFiles: func(t *testing.T, dir string) {
17851785
require.NoError(t, os.WriteFile(filepath.Join(dir, "icon.png"), []byte("img"), 0o644))
17861786
},
17871787
expected: "icon.png",
17881788
},
1789-
"env var file not found and no fallback returns empty": {
1790-
envIconPath: "missing-icon.png",
1791-
setupFiles: func(t *testing.T, dir string) {},
1792-
expected: "",
1789+
"manifest icon file not found and no icon.png returns empty": {
1790+
manifestIcon: "missing-manifest-icon.png",
1791+
setupFiles: func(t *testing.T, dir string) {},
1792+
expected: "",
17931793
},
17941794
}
17951795
for name, tc := range tests {

0 commit comments

Comments
 (0)