Skip to content

fix: preserve original app name as manifest display name #521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
srtaalej merged 6 commits into from
May 8, 2026
Merged

fix: preserve original app name as manifest display name #521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7e47fee
fix: use title case for manifest display names during app creation
srtaalej May 4, 2026
af8e3f4
fix: pass original app name as display name instead of title-casing k…
srtaalej May 4, 2026
e0c3bb0
Update test/testdata/manifest-app-name.ts
srtaalej May 7, 2026
39cdaad
liinter errors
srtaalej May 7, 2026
0a2a5fc
Merge branch 'main' into ale-fix-display-name-casing
srtaalej May 8, 2026
240dccb
test: add WriteFile error test for UpdateDefaultProjectFiles
srtaalej May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions internal/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,18 @@ func NewClient(
}

// UpdateDefaultProjectFiles should update any project specific files if any
func UpdateDefaultProjectFiles(fs afero.Fs, dirPath string, appDirName string) error {
func UpdateDefaultProjectFiles(fs afero.Fs, dirPath string, appDirName string, displayName string) error {
// Files and their corresponding app name replacement functions
projectFiles := []struct {
filename string
replacer func([]byte, string) []byte
name string
}{
{"manifest.json", regexReplaceAppNameInManifest},
{"manifest.js", regexReplaceAppNameInManifest},
{"manifest.ts", regexReplaceAppNameInManifest},
{"package.json", regexReplaceAppNameInPackageJSON},
{"pyproject.toml", regexReplaceAppNameInPyprojectToml},
{"manifest.json", regexReplaceAppNameInManifest, displayName},
{"manifest.js", regexReplaceAppNameInManifest, displayName},
{"manifest.ts", regexReplaceAppNameInManifest, displayName},
{"package.json", regexReplaceAppNameInPackageJSON, appDirName},
{"pyproject.toml", regexReplaceAppNameInPyprojectToml, appDirName},
Comment on lines +60 to +61
Copy link
Copy Markdown
Member

@zimeg zimeg May 7, 2026

Choose a reason for hiding this comment

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

🧠 praise: Nice separation for project titles!

}

for _, pf := range projectFiles {
Expand All @@ -67,7 +68,7 @@ func UpdateDefaultProjectFiles(fs afero.Fs, dirPath string, appDirName string) e
continue
}

fileData = pf.replacer(fileData, appDirName)
fileData = pf.replacer(fileData, pf.name)
if err := afero.WriteFile(fs, filePath, fileData, 0644); err != nil {
return err
}
Expand Down
60 changes: 38 additions & 22 deletions internal/app/app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,28 @@ package app

import (
"fmt"
"os"
"path/filepath"
"testing"

"github.com/slackapi/slack-cli/internal/slackdeps"
"github.com/slackapi/slack-cli/test/testdata"
"github.com/spf13/afero"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

func Test_App_UpdateDefaultProjectFiles(t *testing.T) {
tests := map[string]struct {
appDirName string
displayName string
existingFiles map[string]string
expectedFiles map[string]string
expectedErrorType error
}{
"manifest.json file exists": {
appDirName: "vibrant-butterfly-1234",
appDirName: "vibrant-butterfly-1234",
displayName: "Vibrant butterfly - 1234",
existingFiles: map[string]string{
"manifest.json": string(testdata.ManifestJSON),
},
Expand All @@ -43,7 +47,8 @@ func Test_App_UpdateDefaultProjectFiles(t *testing.T) {
expectedErrorType: nil,
},
"manifest.js file exists": {
appDirName: "vibrant-butterfly-1234",
appDirName: "vibrant-butterfly-1234",
displayName: "Vibrant butterfly - 1234",
existingFiles: map[string]string{
"manifest.js": string(testdata.ManifestJS),
},
Expand All @@ -53,7 +58,8 @@ func Test_App_UpdateDefaultProjectFiles(t *testing.T) {
expectedErrorType: nil,
},
"manifest.ts file exists": {
appDirName: "vibrant-butterfly-1234",
appDirName: "vibrant-butterfly-1234",
displayName: "Vibrant butterfly - 1234",
existingFiles: map[string]string{
"manifest.ts": string(testdata.ManifestTS),
},
Expand All @@ -63,7 +69,8 @@ func Test_App_UpdateDefaultProjectFiles(t *testing.T) {
expectedErrorType: nil,
},
"Multiple manifest files exist": {
appDirName: "vibrant-butterfly-1234",
appDirName: "vibrant-butterfly-1234",
displayName: "Vibrant butterfly - 1234",
existingFiles: map[string]string{
"manifest.json": string(testdata.ManifestJSON),
"manifest.ts": string(testdata.ManifestTS),
Expand All @@ -75,7 +82,8 @@ func Test_App_UpdateDefaultProjectFiles(t *testing.T) {
expectedErrorType: nil,
},
"package.json file exists": {
appDirName: "vibrant-butterfly-1234",
appDirName: "vibrant-butterfly-1234",
displayName: "Vibrant butterfly - 1234",
existingFiles: map[string]string{
"package.json": string(testdata.PackageJSON),
},
Expand All @@ -85,7 +93,8 @@ func Test_App_UpdateDefaultProjectFiles(t *testing.T) {
expectedErrorType: nil,
},
"pyproject.toml file exists": {
appDirName: "vibrant-butterfly-1234",
appDirName: "vibrant-butterfly-1234",
displayName: "Vibrant butterfly - 1234",
existingFiles: map[string]string{
"pyproject.toml": string(testdata.PyprojectTOML),
},
Expand All @@ -95,7 +104,8 @@ func Test_App_UpdateDefaultProjectFiles(t *testing.T) {
expectedErrorType: nil,
},
"Multiple project files exist": {
appDirName: "vibrant-butterfly-1234",
appDirName: "vibrant-butterfly-1234",
displayName: "Vibrant butterfly - 1234",
existingFiles: map[string]string{
"manifest.json": string(testdata.ManifestJSON),
"package.json": string(testdata.PackageJSON),
Expand All @@ -110,20 +120,11 @@ func Test_App_UpdateDefaultProjectFiles(t *testing.T) {
},
"No manifest files exist": {
appDirName: "vibrant-butterfly-1234",
displayName: "Vibrant Butterfly 1234",
existingFiles: map[string]string{},
expectedFiles: map[string]string{},
expectedErrorType: nil,
},
"WriteFile error": {
appDirName: "vibrant-butterfly-1234",
existingFiles: map[string]string{
"manifest.json": string(testdata.ManifestJSON),
},
expectedFiles: map[string]string{
"manifest.json": string(testdata.ManifestJSONAppName),
},
expectedErrorType: nil,
},
Comment on lines -117 to -126
Copy link
Copy Markdown
Member

@zimeg zimeg May 8, 2026

Choose a reason for hiding this comment

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

📠 question: Was this case patched? It's not clear to me why the test is removed?

Copy link
Copy Markdown
Contributor Author

@srtaalej srtaalej May 8, 2026

Choose a reason for hiding this comment

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

that test case is identical to the "manifest.json file exists" test case - i removed it because it was redundant

Copy link
Copy Markdown
Contributor Author

@srtaalej srtaalej May 8, 2026

Choose a reason for hiding this comment

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

i will add a test case that actually produces the write file error separately

}

for name, tc := range tests {
Expand All @@ -146,7 +147,7 @@ func Test_App_UpdateDefaultProjectFiles(t *testing.T) {
}

// Run the tests
err := UpdateDefaultProjectFiles(fs, projectDirPath, tc.appDirName)
err := UpdateDefaultProjectFiles(fs, projectDirPath, tc.appDirName, tc.displayName)

// Assertions
require.IsType(t, err, tc.expectedErrorType)
Expand All @@ -161,6 +162,21 @@ func Test_App_UpdateDefaultProjectFiles(t *testing.T) {
}
}

func Test_App_UpdateDefaultProjectFiles_WriteFileError(t *testing.T) {
fs := slackdeps.NewFsMock()
projectDirPath := "/path/to/project-name"

err := fs.MkdirAll(projectDirPath, 0755)
require.NoError(t, err)
err = afero.WriteFile(fs, filepath.Join(projectDirPath, "manifest.json"), testdata.ManifestJSON, 0644)
require.NoError(t, err)

fs.On("OpenFile", mock.Anything, mock.Anything, mock.Anything).Return((*os.File)(nil), os.ErrPermission)

err = UpdateDefaultProjectFiles(fs, projectDirPath, "vibrant-butterfly-1234", "Vibrant butterfly - 1234")
require.ErrorIs(t, err, os.ErrPermission)
}

func Test_RegexReplaceAppNameInManifest(t *testing.T) {
tests := map[string]struct {
src []byte
Expand All @@ -169,22 +185,22 @@ func Test_RegexReplaceAppNameInManifest(t *testing.T) {
}{
"manifest.json is validate": {
src: testdata.ManifestJSON,
appName: "vibrant-butterfly-1234",
appName: "Vibrant butterfly - 1234",
expectedSrc: testdata.ManifestJSONAppName,
},
"manifest.js is validate": {
src: testdata.ManifestJS,
appName: "vibrant-butterfly-1234",
appName: "Vibrant butterfly - 1234",
expectedSrc: testdata.ManifestJSAppName,
},
"manifest.ts is validate": {
src: testdata.ManifestTS,
appName: "vibrant-butterfly-1234",
appName: "Vibrant butterfly - 1234",
expectedSrc: testdata.ManifestTSAppName,
},
"manifest.ts with sdk is validate": {
src: testdata.ManifestSDKTS,
appName: "vibrant-butterfly-1234",
appName: "Vibrant butterfly - 1234",
expectedSrc: testdata.ManifestSDKTSAppName,
},
}
Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/create/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func Create(ctx context.Context, clients *shared.ClientFactory, createArgs Creat
}()

// Update default project files' app name, bot name, etc
if err := app.UpdateDefaultProjectFiles(clients.Fs, projectDirPath, appDirName); err != nil {
if err := app.UpdateDefaultProjectFiles(clients.Fs, projectDirPath, appDirName, createArgs.AppName); err != nil {
Copy link
Copy Markdown
Member

@zimeg zimeg May 7, 2026

Choose a reason for hiding this comment

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

🌟 praise: Nice! Keeping these separate makes it much easier to think about for me!

return "", slackerror.Wrap(err, slackerror.ErrProjectFileUpdate)
}

Expand Down
4 changes: 2 additions & 2 deletions test/testdata/manifest-app-name.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ export default {
major_version: 2
},
display_information: {
name: 'vibrant-butterfly-1234'
name: 'Vibrant butterfly - 1234'
},
// This is a comment
features: {
Expand All @@ -13,7 +13,7 @@ export default {
messages_tab_read_only_enabled: false
},
bot_user: {
display_name: 'vibrant-butterfly-1234'
display_name: 'Vibrant butterfly - 1234'
}
},
functions: {
Expand Down
4 changes: 2 additions & 2 deletions test/testdata/manifest-app-name.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"major_version": 2
},
"display_information": {
"name": "vibrant-butterfly-1234"
"name": "Vibrant butterfly - 1234"
},
"features": {
"app_home": {
Expand All @@ -12,7 +12,7 @@
"messages_tab_read_only_enabled": false
},
"bot_user": {
"display_name": "vibrant-butterfly-1234"
"display_name": "Vibrant butterfly - 1234"
}
},
"functions": {
Expand Down
4 changes: 2 additions & 2 deletions test/testdata/manifest-app-name.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export default {
"major_version": 2
},
"display_information": {
"name": "vibrant-butterfly-1234"
"name": "Vibrant butterfly - 1234"
},
// This is a comment
"features": {
Expand All @@ -22,7 +22,7 @@ export default {
"messages_tab_read_only_enabled": false
},
"bot_user": {
"display_name": "vibrant-butterfly-1234"
"display_name": "Vibrant butterfly - 1234"
}
},
"functions": {
Expand Down
2 changes: 1 addition & 1 deletion test/testdata/manifest-sdk-app-name.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const obj = {
};

export default Manifest({
"name": "vibrant-butterfly-1234",
"name": "Vibrant butterfly - 1234",
"description": "Reverse a string",
// "runtime_environment": "slack",
"runtime": "deno1.x",
Expand Down
Loading