feat: add 'app settings' command to open app settings webpage for a selected app #92

zimeg · 2025-05-14T04:30:25Z

Changelog

The link between code of a project and configurations on app settings is now a commanded call with the latest slack app settings command.

Bolt developers with a remote manifest source managed on app settings might find this useful for discovering new features or making updates otherwise.

Summary

This PR adds the app settings command to open app settings for a selected app.

Preview

demo.mov

Reviewers

Hello please feel free to test this with various apps, perhaps a:

Bolt app installed
Bolt app uninstalled
Bolt project without apps
Deno app (this errors)
QA or dev instances

$ slack app settings

For the most adventurous, the --app flag can be attempted too.

Notes

ROSI apps are not supported in app settings at this time and will cause this command to error! The --force flag can be used to attempt this opening.

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2025-05-14T05:26:59Z

Codecov Report

Attention: Patch coverage is 92.59259% with 6 lines in your changes missing coverage. Please review.

Project coverage is 63.31%. Comparing base (4485405) to head (2d78fb8).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/app/settings.go	92.40%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #92      +/-   ##
==========================================
+ Coverage   63.18%   63.31%   +0.12%     
==========================================
  Files         210      211       +1     
  Lines       22195    22276      +81     
==========================================
+ Hits        14025    14104      +79     
  Misses       7085     7085              
- Partials     1085     1087       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mwbrooks

✅ Amazing work @zimeg 🎉

🧪 Manual testing is looking great and you've got very high test coverage on this command 📈

📝 I left a few minor suggestions that you can take, leave, or riff with. I've also left a lengthy suggestion to follow-up with a 2nd PR that attempts to introduce a ~~best~~ better practice for our global xyzFunc swaps.

mwbrooks · 2025-05-15T03:33:46Z

cmd/app/app.go

 			{Command: "install", Meaning: "Install a production app to a team"},
 			{Command: "link", Meaning: "Link an existing app to the project"},
 			{Command: "list", Meaning: "List all teams with the app installed"},
+			{Command: "app settings", Meaning: "Open app settings in a web browser"},


note: Foreseeing a future of alphabetically ordered examples prefixed with app 🔮

As much as I appreicate alphabetics I was thinking these examples were ordered with expected steps in mind?

To me this tells an interesting story:

install

link

list

settings

uninstall

delete

Which is almost in alphabetics I realize now! So perhaps following "install" with "delete" isn't so wild 👾

Ahhhh, that's alright as well. For now, the order flows well and it's easy to see where unlink & update would land.

When unlink lands we should revisit this. Background thoughts are now nudging me towards preferring the more standard order 😳

mwbrooks · 2025-05-15T03:34:45Z

internal/slackerror/errors.go

 	ErrAppExists                                     = "app_add_exists"
 	ErrAppFlagRequired                               = "app_flag_required"
 	ErrAppFound                                      = "app_found"
+	ErrAppHosted                                     = "app_hosted"


note: I can see this being a useful error for us in the future!

cmd/app/settings.go

mwbrooks · 2025-05-15T04:01:51Z

cmd/app/settings.go

+	"github.com/spf13/cobra"
+)
+
+var settingsAppSelectPromptFunc = prompts.AppSelectPrompt


thought: [Non-Blocker] I'd like us to use this new command as a way to nudge out best practices for maintainability and testability. While we use the xyzFunc pattern for testability, it always feels like a hack.

I think the better approach would be dependency injection: NewSettingsCommand(clients, prompts.AppSelectPrompt).

But, how do we design this to scale for all commands and xyzFunc workarounds?

Maybe we use clients? This would remove the global swap-a-roo and isolate it to the instance of clients.

// clients.go clients.AppSelectPrompt = prompts.AppSelectPrompt // clients_mock.go clients.AppSelectPrompt = appSelectMock.AppSelectPrompt

I think this is out-of-scope of this PR, but perhaps we can follow-up with some approach in a future PR? We can take some time to see if it helps address other xyzFunc use-cases as well.

@mwbrooks I'm so interested in this as well 🧠 ✨

I considered mocking the calls used in AppSelectPrompt to avoid this pattern altogether, but found that this adds so much overhead and perhaps mixes concerns...

Using clients might be a decent approach! It seems like we're settling on patterns of having identical function signatures within a command:

RunE: func(cmd *cobra.Command, args []string) error { return appSettingsCommandRunE(clients, cmd, args) },

But we've discussed avoiding clients for sake of more obvious usage of packages and dependencies. I'm wondering if within the command handler clients might be alright to keep, but for all other code following it should be avoided?

I'll hold off on making changes here so we can keep these patterns consistent for now, but I want to know what you think and I'm happy to follow up with changes we do decide on 🤖

Totally agree, good call on not adding more complexity in this PR. Let's discuss it offline to figure out what we want to try out!

cmd/app/settings.go

mwbrooks · 2025-05-15T04:06:49Z

cmd/app/settings.go

+	parsed, err := url.Parse(host)
+	if err != nil {
+		return err
+	}
+	parsed.Host = "api." + parsed.Host
+	settingsURL := fmt.Sprintf("%s/apps/%s", parsed.String(), app.App.AppID)


praise: Very nice work 👏🏻

mwbrooks · 2025-05-15T04:11:30Z

cmd/app/settings.go

+		Secondary: []string{
+			settingsURL,
+		},


suggestion: What do you think about adding output explaining the action being taken (opening the web browser)?

Suggested change

Secondary: []string{

settingsURL,

},

Secondary: []string{

"Opening default web browser:",

settingsURL,

},

☝🏻 You might have better wording than that.

🤔 I might have preference against this since erroring cases might cause confusion that the standalone link saves?

The balance of text also feels lopsided but I don't think this is significant reason against this:

🏠 App Settings Opening default web browser: https://api.slack.com/apps/A08SPJ83H4L

📝 It does make me think --no-browser might be a flag to later support with the default behavior though!

Sounds good to me. Also --no-browser would be an appropriate future flag, since activity --browser already exists to open the activity in the browser.

mwbrooks · 2025-05-15T04:12:10Z

cmd/app/settings.go

+	}))
+	clients.Browser().OpenURL(settingsURL)
+
+	clients.IO.PrintTrace(ctx, slacktrace.AppSettingsSuccess, settingsURL)


praise: heh, you read my mind to add settingsURL 😉

The setups of PrintTrace continue to impress me! 👾

I'm hoping soon we can equip all commands with at least a simple "start" and "success" with follow ups in @slack/cli-test but for now I'm also so glad we can print a success message with additional detail 📚 ✨

It seems like a reasonable PR to add a "start" and "success" to each command and land the equivalent definitions in @slack/cli-test.

mwbrooks · 2025-05-15T04:13:46Z

cmd/app/settings_test.go

+			},
+			ExpectedError: slackerror.New(slackerror.ErrAppHosted),
+		},
+		"requires an existing application": {


note: Nice, I tested this manually and I'm happy to see you already added a test!

Co-authored-by: Michael Brooks <[email protected]>

zimeg · 2025-05-15T18:35:31Z

@mwbrooks I am so excited for this to land in the next release! 🚀 ✨

Thanks for helping refine this to a better place as always. I left a few responses about perhaps much larger changes that this PR poked at, but for now I will merge this.

Follow up for particular flag options is also interesting to me, but also might be outside the scope of a first iteration.

zimeg added 3 commits May 13, 2025 21:10

feat: open app settings for a selected app with the app settings command

34443a2

docs: include what a remote manifest source is in help pages

6239a1c

docs: include an example command in the top level app command

4a3e18e

zimeg added this to the Next Release milestone May 14, 2025

zimeg self-assigned this May 14, 2025

zimeg requested a review from a team as a code owner May 14, 2025 04:30

zimeg added enhancement M-T: A feature request for new functionality changelog Use on updates to be included in the release notes semver:minor Use on pull requests to describe the release version increment labels May 14, 2025

mwbrooks changed the title ~~feat: open app settings for a selected app with the app settings command~~ feat: add 'app settings' command to open app settings webpage for a selected app May 15, 2025

chore: merge w main

452a9e3

mwbrooks approved these changes May 15, 2025

View reviewed changes

zimeg and others added 3 commits May 15, 2025 10:30

docs: suggest app settings as a web interface for customizations

0937dad

Co-authored-by: Michael Brooks <[email protected]>

style: explain the reason a force flag is included in run checks

a178dd7

Co-authored-by: Michael Brooks <[email protected]>

test: confirm the force flag opens app settings for a rosi application

2d78fb8

zimeg merged commit 1977c44 into main May 15, 2025
6 checks passed

zimeg deleted the zimeg-feat-app-settings branch May 15, 2025 18:38

feat: add 'app settings' command to open app settings webpage for a selected app #92

feat: add 'app settings' command to open app settings webpage for a selected app #92

Uh oh!

Conversation

zimeg commented May 14, 2025

Summary

Preview

Reviewers

Notes

Requirements

Uh oh!

codecov bot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mwbrooks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg commented May 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented May 14, 2025 •

edited

Loading