feat: display manifest source in human-friendly format #90

mwbrooks · 2025-05-14T03:11:01Z

CHANGELOG

Display the manifest source as human-friendly output: "project (local)" and "app settings (remote)". You'll see these changes in commands such as slack create, slack init, and slack app link <id>.

Summary

This pull request updates the type ManifestSource with a manifestSource.Human() method to display human-friendly strings during output.

The string format is noun (id):

project (local)
app settings (remote)

Since we often need to output the manifest source, the local and remote IDs make the developer think a lot. I feel that we can be a good host by displaying these IDs in a more applicable way.

Preview

`slack init` or `slack create`

`slack app link <id>`

Reviewer

# Create command
$ lack create asdf/
# → Select "Starter App"
# → Select "Bolt for JavaScript"
# → Confirm the output "project (local)"

$ cd asdf/

# Init command
$ lack init
# → Confirm the output "project (local)"

# Link command
$ lack app link A0123
# → Confirm warning output uses "project (local)" and "app settings (remote)" in meaningful ways
# → Select "N"

# Clean up
$ cd ../
$ rm -rf asdf/

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.

mwbrooks

Quick notes to explain some of my edits ✏️

mwbrooks · 2025-05-14T03:12:19Z

cmd/app/link.go

+				fmt.Sprintf(`App manifest source can be "%s" or "%s":`, config.ManifestSourceLocal.Human(), config.ManifestSourceRemote.Human()),
 				fmt.Sprintf("- %s: uses manifest from your project's source code for all apps", config.ManifestSourceLocal.String()),
 				fmt.Sprintf("- %s: uses manifest from app settings for each app", config.ManifestSourceRemote.String()),
 				" ",
+				fmt.Sprintf(style.Highlight(`Your manifest source is "%s"`), manifestSource.Human()),
+				" ",


note: I tweaked the wording now that we've had some time to reflect on this. Once we default Bolt projects to remote manifest, this warning will no longer be shown, so it's not a long-lived change.

🙏 ✨ This section has been confusing to me before, but I hadn't realized that a defaulted remote manifest source will avoid this altogether!

Edit: For my own understanding, will it still be shown but instead much less often?

I can imagine changing the manifest source to local for automated productions then linking an app later, though this might not be the common case...

Good question! I think the warning becomes less common after bolt-install and we could consider changing it to a message instead of a prompt.

The warning should be displayed when a project manages the app manifest (manifest.source: "local") and an existing app is linked (slack app link).

Right now, the warning prompts you to switch to a remote manifest source. I think we have an opportunity to make this more flexible. There are some cases where you may want to link an app while keeping your local manifest source.

Long story short, we'll want to think more about this as we make decisions around bolt-install 😆

codecov · 2025-05-14T03:14:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.18%. Comparing base (1129877) to head (9329993).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
+ Coverage   63.17%   63.18%   +0.01%     
==========================================
  Files         210      210              
  Lines       22186    22194       +8     
==========================================
+ Hits        14015    14023       +8     
  Misses       7084     7084              
  Partials     1087     1087

☔ 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.

zimeg

@mwbrooks I started to ramble out of excitement for this change. It's so helpful in understanding what "remote" and "local" mean while still keeping these terms for actual configuration 🤩

Most of the comments that follow focus on sentence structure and formatting, but it seems aligned with the goals of this PR I hope 📚 ✨

Outside of a fallback default case with perhaps a test, nothing is blocking so this gets an approve but I am hoping to hear more of what you're thinking about these changes. I'm also looking forward to follow ups in copy, so no worries if some comments are ignored 🐏

zimeg · 2025-05-14T05:52:08Z

internal/config/manifest_test.go

 			a:        ManifestSourceLocal,
 			expected: "local",
 		},
-		"remote manifest source is remote": {


zimeg · 2025-05-14T05:58:38Z

cmd/app/link.go

+				fmt.Sprintf(`App manifest source can be "%s" or "%s":`, config.ManifestSourceLocal.Human(), config.ManifestSourceRemote.Human()),
 				fmt.Sprintf("- %s: uses manifest from your project's source code for all apps", config.ManifestSourceLocal.String()),
 				fmt.Sprintf("- %s: uses manifest from app settings for each app", config.ManifestSourceRemote.String()),
 				" ",
+				fmt.Sprintf(style.Highlight(`Your manifest source is "%s"`), manifestSource.Human()),
+				" ",


🙏 ✨ This section has been confusing to me before, but I hadn't realized that a defaulted remote manifest source will avoid this altogether!

cmd/app/link.go

zimeg · 2025-05-14T06:04:25Z

internal/config/manifest.go

+func (ms ManifestSource) Human() string {
+	switch ms {
+	case ManifestSourceLocal:
+		return fmt.Sprintf("project (%s)", ms.String())


🤔 This might be confusing, but I have a small favor towards quoting "project" while keeping (local) and (remote) in parentheses.

To me it'd read better in lines like:

App manifest source can be "project" (local) or "app settings" (remote)

Edit: But perhaps we remove quotes in these lines altogether? Or handle the cases of "project" and "app settings" in configuration, but with warning to use the preferred local or remote?

I now lean towards removing quotes, but I'd love to know what is most clear to you!

Great suggestion. I think the "project" (local) format is an improvement! I'll update the tests and commit the changes! 🙇🏻

Commit 4eaea06 now quotes the human-friendly name and wraps the ID in parentheses.

zimeg · 2025-05-14T06:05:58Z

internal/config/manifest.go

+	case ManifestSourceRemote:
+		return fmt.Sprintf("app settings (%s)", ms.String())
+	}
+	return ""


Suggested change

return ""

return ms.String()

Ought not we find a confusing blank output here in some strange case 😉

Edit: A test for this too would be superb but is also no blocker for me 🤖 ✨

Great eye! Commit 3913b13 handles unknown manifest sources with your suggestion above and adds a test case 👌🏻

zimeg · 2025-05-14T06:08:09Z

cmd/app/link.go

+				fmt.Sprintf("managed by %s.", config.ManifestSourceRemote.Human()),
 				" ",
-				fmt.Sprintf(style.Highlight(`Your manifest source is "%s"`), manifestSource.String()),
+				fmt.Sprintf(`App manifest source can be "%s" or "%s":`, config.ManifestSourceLocal.Human(), config.ManifestSourceRemote.Human()),


🗣️ Another comment suggests removing the quotations with Human but I'd be curious how this reads.

🤔 I think for this specific line, we want to tell the developer what their current manifest source is set to. I stuck with the .String() but we could switch it to the .Human().

I think for now, let's leave it as-is. We already list the options above this text. Originally, the text was meant to explain what the project's current configuration is. Underneath, we then show the value in config.json and explain what the value will be changed to.

zimeg · 2025-05-14T06:09:41Z

cmd/app/link.go

+				fmt.Sprintf(`App manifest source can be "%s" or "%s":`, config.ManifestSourceLocal.Human(), config.ManifestSourceRemote.Human()),
 				fmt.Sprintf("- %s: uses manifest from your project's source code for all apps", config.ManifestSourceLocal.String()),
 				fmt.Sprintf("- %s: uses manifest from app settings for each app", config.ManifestSourceRemote.String()),
 				" ",
+				fmt.Sprintf(style.Highlight(`Your manifest source is "%s"`), manifestSource.Human()),
+				" ",


Edit: For my own understanding, will it still be shown but instead much less often?

I can imagine changing the manifest source to local for automated productions then linking an app later, though this might not be the common case...

zimeg · 2025-05-14T06:13:58Z

cmd/app/link.go

 				" ",
-				fmt.Sprintf(style.Highlight(`Your manifest source is "%s"`), manifestSource.String()),
+				fmt.Sprintf(`App manifest source can be "%s" or "%s":`, config.ManifestSourceLocal.Human(), config.ManifestSourceRemote.Human()),
 				fmt.Sprintf("- %s: uses manifest from your project's source code for all apps", config.ManifestSourceLocal.String()),


Suggested change

fmt.Sprintf("- %s: uses manifest from your project's source code for all apps", config.ManifestSourceLocal.String()),

fmt.Sprintf("- %s: uses manifest from your project's source code on reinstallation", config.ManifestSourceLocal.String()),

📚 This might not be a good suggestion, but confusion could arise with when these updates are.

Perhaps this is alright though! Another command might make this difference more obvious in later iteration?

$ slack manifest update

Edit: This comment makes me think "on update" is better wording, but again no blocker!

Suggested change

fmt.Sprintf("- %s: uses manifest from your project's source code for all apps", config.ManifestSourceLocal.String()),

fmt.Sprintf("- %s: uses manifest from your project's source code on update", config.ManifestSourceLocal.String()),

I agree that this can be confusing. Unfortunately, the project-level manifest is used in many places and not just updates/re-installs. For example, slack manifest validate but also many areas of the internal-workings such as checking whether the app is hosted or not (a pre-run for some commands).

Looking ahead, this warning is probably going to be removed or reduced to a single line such as:

🤖 Some Title Reading app manifest from app settings Reading app manifest from project

So, we probably don't need to overthink the current warning too much.

@mwbrooks 🧠 ✨ Of course, how could I forget!

Unfortunately, the project-level manifest is used in many places

I will celebrate this learning with good a fortune 👾

Co-authored-by: Eden Zimbelman <[email protected]>

feat: display manifest source in human-readable format

31559f0

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

mwbrooks self-assigned this May 14, 2025

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

test: fix failing tests

5c27011

mwbrooks commented May 14, 2025

View reviewed changes

mwbrooks marked this pull request as ready for review May 14, 2025 03:17

mwbrooks requested a review from a team as a code owner May 14, 2025 03:18

zimeg approved these changes May 14, 2025

View reviewed changes

mwbrooks and others added 4 commits May 14, 2025 14:32

Update cmd/app/link.go

acd2a00

Co-authored-by: Eden Zimbelman <[email protected]>

feat: handle unknown manifest source

3913b13

feat: quote the human-friendly name

4eaea06

Merge branch 'main' into mwbrooks-manifest-source-human

9329993

mwbrooks merged commit c326b5d into main May 14, 2025
5 checks passed

mwbrooks deleted the mwbrooks-manifest-source-human branch May 14, 2025 22:13

	fmt.Sprintf("- %s: uses manifest from your project's source code for all apps", config.ManifestSourceLocal.String()),
	fmt.Sprintf("- %s: uses manifest from your project's source code on reinstallation", config.ManifestSourceLocal.String()),

feat: display manifest source in human-friendly format #90

feat: display manifest source in human-friendly format #90

Uh oh!

Conversation

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

Uh oh!

CHANGELOG

Summary

Preview

slack init or slack create

slack app link <id>

Reviewer

Requirements

Uh oh!

mwbrooks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

zimeg 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mwbrooks commented May 14, 2025 •

edited

Loading

`slack init` or `slack create`

`slack app link <id>`

mwbrooks left a comment •

edited

Loading

codecov bot commented May 14, 2025 •

edited

Loading