Skip to content

feat: update 'feedback' command to include feedback about the Slack CLI#66

Merged
mwbrooks merged 10 commits into
mainfrom
mwbrooks-feedback-github
Apr 29, 2025
Merged

feat: update 'feedback' command to include feedback about the Slack CLI#66
mwbrooks merged 10 commits into
mainfrom
mwbrooks-feedback-github

Conversation

@mwbrooks

@mwbrooks mwbrooks commented Apr 25, 2025

Copy link
Copy Markdown
Member

CHANGELOG

Updated feedback command to display a prompt for Slack CLI feedback. Now developers can ask questions, submit issues, or suggest features for the Slack CLI through GitHub Issues.

Summary

This pull request updates the feedback command to display a prompt with 2 choices:

  1. Slack CLI
  2. Slack Platform

While expanding feedback to display multiple options, there are a lot of improvements that we can make. However, I've tried to scope this PR to only adding the 2nd option and tweaking things to look & feel alright.

The main changes are:

  • Added option for "Slack CLI Feedback"
  • Renamed the original option to "Slack Platform Feedback"
    • Previously, it was "Help make the Slack platform better"
  • Added a Never frequency
    • So that the Slack CLI Feedback is never, randomly asked
    • The Slack Platform Feedback continues to be randomly asked after some commands
  • Added a SkipQueryParams field (optional)
    • So that the the system_id, project_id, and other query string params are not included in the https://github.com/slackapi/slack-cli/issues URL
    • Default is false, so the Slack Platform Feedback URL continues to have the query string params
  • Updated slacktrace.FeedbackMessage to include the feedback option chosen
    • e.g. SLACK_TRACE_FEEDBACK_MESSAGE=slack-cli-feedback
  • Generalized "survey" wording to be "feedback" or "options"
    • This isn't perfect, but it's a step in the right direction
    • It just felt weird to see things like "Open survey in browser?" when a user was opening GitHub Issues

Preview

2025-04-25-feedback.mov

Reviewers

# Feedback Prompt
$ lack feedback
# → Select Slack CLI
# → Confirm opening in browser without qs params

$ lack feedback
# → Confirm completed timestamp on Slack CLI
# CTRL+C

# Feedback Flag
$ lack feedback --name slack-cli-feedback
# → Confirm skips prompt and asks to open browser
# CTRL+C

# Feedback Trace
$ SLACK_TEST_TRACE=1 lack feedback --verbose
# → Select something
# → Confirm Trace with selection
# CTRL+C

# Post-Command Feedback Prompt
$ lack doctor
# → Confirm feedback message after command completes

Requirements

@mwbrooks mwbrooks added enhancement M-T: A feature request for new functionality semver:minor Use on pull requests to describe the release version increment labels Apr 25, 2025
@mwbrooks mwbrooks added this to the Next Release milestone Apr 25, 2025
@mwbrooks mwbrooks self-assigned this Apr 25, 2025
@codecov

codecov Bot commented Apr 25, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 23.25581% with 33 lines in your changes missing coverage. Please review.

Project coverage is 63.03%. Comparing base (48cddd3) to head (48802d9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cmd/feedback/feedback.go 23.25% 31 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #66      +/-   ##
==========================================
- Coverage   63.12%   63.03%   -0.10%     
==========================================
  Files         210      210              
  Lines       22159    22186      +27     
==========================================
- Hits        13987    13984       -3     
- Misses       7086     7114      +28     
- Partials     1086     1088       +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 mwbrooks left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤟🏻 Some notes to guide the reviewer through my thoughts! 💭

Comment thread cmd/feedback/feedback.go
Comment on lines +43 to +44
// SkipQueryParams is a flag to skip adding query params to the survey URL (optional, default false)
SkipQueryParams bool

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: Added a new, optional param to skip the query string params. This produces a clean URL for the GitHub Issues, which don't use our query string data.

Comment thread cmd/feedback/feedback.go
Always Frequency = iota // We always want to ask
Once // Ask user to complete the survey only once
Monthly // Ask user to complete the survey once a month
Never // Do not ask the user to complete the survey

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: Added a new frequency option to Never ask the user to complete a survey.

Comment thread cmd/feedback/feedback.go Outdated
Comment on lines +73 to +74
SlackCLIFeedback = "slack-cli-feedback"
SlackPlatformFeedback = "platform-improvements"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: We can't rename the platform-improvements ID without some migration logic, but how does the slack-cli-feedback ID look? We'll follow this pattern for the Bolt Frameworks, so let's make sure we're happy with it. 🕹️

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

QQ: For my own understanding, would Bolt feedback follow this pattern as:

  • bolt-feedback

Or were you thinking that this might be runtime specific?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Edit: At a glance I think the current slack-cli-feedback is solid too!

Before settling on it I am also wondering if this might be extended more, but I like that we can have different types of issues after the "CLI" option is selected, on GitHub Issues.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good question! My thought is that we'd like to add a link for each framework that the CLI supports, so that developers can learn where to ask question. So:

  • Bolt for JavaScript (bolt-js-feedback)
  • Bolt for Python (bolt-python-feedback)
  • Deno SDK (deno-slack-sdk-feedback)
  • Slack CLI (slack-cli-feedback)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For the ID, we could also go with (-github), which would be more accurate:

  • Slack CLI (slack-cli-github)

Or, we could drop the post-fix entirely and follow the GitHub repo names:

  • Bolt for JavaScript (bolt-js)
  • Bolt for Python (bolt-python)
  • Deno SDK (deno-slack-sdk)
  • Slack CLI (slack-cli)

@zimeg zimeg Apr 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@mwbrooks The standalone* GitHub repo seems so good! I like that it is simple.

I'm now wondering if expanding the doctor hook responses to include the upstream paths might be helpful for this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
SlackCLIFeedback = "slack-cli-feedback"
SlackPlatformFeedback = "platform-improvements"
SlackCLIFeedback = "slack-cli"
SlackPlatformFeedback = "platform-improvements"

🪓 Removing the "-feedback" suffix to leave the specifics to a GitHub issue! This also makes flag options more intuitive IMO:

- $ slack feedback --name slack-cli-feedback
+ $ slack feedback --name slack-cli

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Cool, i like the simple names for slack-cli, bolt-js, bolt-python`, etc.

Commit f5b343f updates it to slack feedback --name slack-cli.

In PR #68, I'll also explore whether I can switch platform-improvements to slack-platform with backwards compatibility that we deprecated in v4.0.0. 🤔

Comment thread cmd/feedback/feedback.go
Comment on lines +85 to +86
// SlackCLIFeedback asks for Slack CLI feedback using GitHub Issues
SlackCLIFeedback: {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: Here's a screenshot of the prompt. How do the title and descriptions look?
image

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔍 I am so biased to compressed outputs with sections so was expecting something perhaps more as:

$ slack feedback

💌 Feedback
   Thanks for taking a moment to share your thoughts!

? What type of feedback would you like to give?
> Slack CLI
  Questions, issues, and feature requests about the Slack CLI
  Slack Platform
  Developer support for the Slack Platform, Slack API, Block Kit, and more

But perhaps with secondary text for the descriptions? This is more opinion than suggestion, but I'm also curious what you think! I do like the wording here 📚 ✨

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I share you feeling that I'd like to see the prompts condensed. Unfortunately, I don't think we jump into doing it. 😢

The reason is that this prompt is rendered by clients.IO.SelectPrompt and it adds the extra space when a description is provided. The same look & feel exists for slack samples. So, I think we would want to apply this change everywhere, consistently.

I think it's better to wait for the BubbleTea refactor. Thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ahha! I didn't think to check this in implementation, but I agree BubbleTea will be a super great improvement for prompts 🫧 ✨

Comment thread cmd/feedback/feedback.go
style.Secondary(style.Highlight("https://github.com/slackapi/slack-cli/issues")),
))
},
Trace: slacktrace.FeedbackMessage,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: In the future, we could make the trace optional or removed entirely. I think it's better to have 1 trace for all feedback options (slacktrace.FeedbackMessage) then include the selected feedback name as trace data.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Heh I missed this in a quick review earlier! But I so agree! Thanks for opening #68 🙏 ✨

Comment thread cmd/feedback/feedback.go
Comment on lines +102 to +111
Ask: func(ctx context.Context, clients *shared.ClientFactory) (bool, error) {
clients.IO.PrintInfo(ctx, false, style.Sectionf(style.TextSection{
Emoji: "love_letter",
Text: "We would love to know how things are going",
Secondary: []string{
"Share your experience with " + style.Commandf("feedback --name slack-cli-feedback", false),
},
}))
return false, nil
},

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: Since we Never randomly ask about this survey, this function is never actually executed. But it's a required field.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤔 No blocker for this PR, but it might be interesting to replace the Ask parameter with a "string" for style.TextSection inputs instead.

IMO it's somewhat confusing to keep this prompt with the "Never" option otherwise 🔍

@mwbrooks mwbrooks Apr 29, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is a great suggestion. I agree, the Ask with Never is unnecessary. The style.TextSection also simplifies the implementation while keeping the prompt more consistent.

I've made a note and we can follow-up in a future PR!

Honestly, I don't see us using the Ask in the originally intended way. If we don't leverage it in the future, we could also replace it with a single "ask" for feedback.

Comment thread cmd/feedback/feedback.go
Comment on lines +117 to +121
// SlackPlatformFeedback asks for general developer experience feedback
SlackPlatformFeedback: {
Name: SlackPlatformFeedback,
PromptDisplayText: "Slack Platform",
PromptDescription: "Developer support for the Slack Platform, Slack API, Block Kit, and more",

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: Renamed this to be focused on Slack Platform features - API, Block Kit, etc. Is the distinction clear enough?

Comment thread cmd/feedback/feedback.go
"%s\n%s\n",
style.Secondary("You can send us a message at "+style.Highlight(email)),
style.Secondary("Or, survey your experiences at "+style.Highlight("https://docs.slack.dev/developer-support")),
style.Secondary("Or, share your experiences at "+style.Highlight("https://docs.slack.dev/developer-support")),

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: Throughout, I remove the "survey" reference for terms around sharing feedback, since we don't actually link to any surveys now.

@mwbrooks mwbrooks marked this pull request as ready for review April 25, 2025 19:06
@mwbrooks mwbrooks requested a review from a team as a code owner April 25, 2025 19:06
@mwbrooks mwbrooks added the changelog Use on updates to be included in the release notes label Apr 25, 2025

@zimeg zimeg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

💌 Super quick notes before I wanted to drop before popping offline for a few minutes but this is super great!

Comment thread cmd/feedback/feedback.go Outdated
Comment on lines +73 to +74
SlackCLIFeedback = "slack-cli-feedback"
SlackPlatformFeedback = "platform-improvements"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

QQ: For my own understanding, would Bolt feedback follow this pattern as:

  • bolt-feedback

Or were you thinking that this might be runtime specific?

Comment thread cmd/feedback/feedback.go
surveyNames, surveyPromptOptions := initSurveyOpts(ctx, clients, SurveyStore)

if _, ok := SurveyStore[surveyNameFlag]; !ok && surveyNameFlag != "" {
return slackerror.New("invalid_survey_name").

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
return slackerror.New("invalid_survey_name").
return slackerror.New(slackerror.ErrInvalidSurveyName).

I'm not sure if this is a coded error at the moment, but this can help match formats for quick lookups later:

- 🚫 Please provide a feedback name or remove the --no-prompt flag
+ 🚫 Please provide a feedback name or remove the --no-prompt flag (invalid_survey_name)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Great suggestion! I'd like to keep this PR focused on adding the Slack CLI feedback option, but I agree that we should improve the feedback's implementation a bit!

I've pull this suggestion into PR #68, which adds slackerror.ErrFeedbackNameInvalid and slackerror.ErrFeedbackNameRequired!

Comment thread cmd/feedback/feedback.go
Comment on lines 257 to 261
if surveyNameFlag == "" && noPromptFlag {
return slackerror.New("survey_name_required").
WithMessage("Please provide a survey name or remove the --no-prompt flag").
WithRemediation("View survey options with %s", style.Commandf("feedback --help", false))
WithMessage("Please provide a feedback name or remove the --no-prompt flag").
WithRemediation("View feedback options with %s", style.Commandf("feedback --help", false))
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤔 This was a confusing message for me and I tried to remediate it with:

$ slack feedback --no-prompt slack-cli-feedback

I realize now the --help command demonstrates the --name flag, but I was more stumped than I'd like to admit 😉

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point, PR #68 (commit 602085f) updates this error message to be:

"Please provide a feedback --name flag or remove the --no-prompt flag"

Comment thread cmd/feedback/feedback.go
s.Info(ctx, clients)
}
clients.IO.PrintTrace(ctx, s.Trace)
clients.IO.PrintTrace(ctx, s.Trace, s.Name)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
clients.IO.PrintTrace(ctx, s.Trace, s.Name)
clients.IO.PrintTrace(ctx, slacktrace.FeedbackMessage, s.Name)

🪓 Since the same trace argument is used it might be more clear to hardcode that here? No blocker at all though!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm all for axing it! 🪓

I've pulled this work out to PR #68, which:

  • Removes the Trace field from the Survey struct
  • Always uses slacktrace.FeedbackMessage
  • Adds tests to check that the trace is printed

Comment thread cmd/feedback/feedback.go
Comment on lines +85 to +86
// SlackCLIFeedback asks for Slack CLI feedback using GitHub Issues
SlackCLIFeedback: {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔍 I am so biased to compressed outputs with sections so was expecting something perhaps more as:

$ slack feedback

💌 Feedback
   Thanks for taking a moment to share your thoughts!

? What type of feedback would you like to give?
> Slack CLI
  Questions, issues, and feature requests about the Slack CLI
  Slack Platform
  Developer support for the Slack Platform, Slack API, Block Kit, and more

But perhaps with secondary text for the descriptions? This is more opinion than suggestion, but I'm also curious what you think! I do like the wording here 📚 ✨

Comment thread cmd/feedback/feedback.go Outdated
Comment on lines +73 to +74
SlackCLIFeedback = "slack-cli-feedback"
SlackPlatformFeedback = "platform-improvements"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Edit: At a glance I think the current slack-cli-feedback is solid too!

Before settling on it I am also wondering if this might be extended more, but I like that we can have different types of issues after the "CLI" option is selected, on GitHub Issues.

@mwbrooks

Copy link
Copy Markdown
Member Author

@zimeg Thanks for the quick feedback on Friday! 🙇🏻 💌

I'd like to keep this PR focused on adding Slack CLI to the feedback command, so I've created PR #68 to capture the refactor suggestions - Traces, Errors, etc.

I think I've addressed your feedback - except the condensed prompt, which would be a wider change - so let me know how it looks on your end!

@zimeg zimeg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thanks for making a follow up PR with the unrelated changes noted in prior comments 🤖 ✨

I left a few more comments about formatting outputs that might be worth revisiting later, and a few more comments about the feedback name that we can settle on before merging. Once these all look good please feel free to merge 🚢

I'm looking forward to feedback and reports to come 💌 ✨

Comment thread cmd/feedback/feedback.go Outdated
Comment on lines +73 to +74
SlackCLIFeedback = "slack-cli-feedback"
SlackPlatformFeedback = "platform-improvements"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
SlackCLIFeedback = "slack-cli-feedback"
SlackPlatformFeedback = "platform-improvements"
SlackCLIFeedback = "slack-cli"
SlackPlatformFeedback = "platform-improvements"

🪓 Removing the "-feedback" suffix to leave the specifics to a GitHub issue! This also makes flag options more intuitive IMO:

- $ slack feedback --name slack-cli-feedback
+ $ slack feedback --name slack-cli

Comment thread cmd/feedback/feedback.go
Comment on lines +102 to +111
Ask: func(ctx context.Context, clients *shared.ClientFactory) (bool, error) {
clients.IO.PrintInfo(ctx, false, style.Sectionf(style.TextSection{
Emoji: "love_letter",
Text: "We would love to know how things are going",
Secondary: []string{
"Share your experience with " + style.Commandf("feedback --name slack-cli-feedback", false),
},
}))
return false, nil
},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤔 No blocker for this PR, but it might be interesting to replace the Ask parameter with a "string" for style.TextSection inputs instead.

IMO it's somewhat confusing to keep this prompt with the "Never" option otherwise 🔍

Comment thread cmd/feedback/feedback.go
clients.Browser().OpenURL(url)
} else { // Print survey URL
clients.IO.PrintInfo(ctx, false, fmt.Sprint("Survey URL: \n", style.Secondary(u)))
clients.IO.PrintInfo(ctx, false, fmt.Sprint("Feedback URL: \n", style.Secondary(url)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

📝 No blocker but this might be another section to revisit formatting for:

💌 Feedback
   Share thoughts or ideas or findings at the following URL:
   https://github.com/slackapi/slack-cli/issues

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This looks so much better than the raw "Feedback URL:" that exists right now. I'll add a note and iterate on this in future PR!

Comment thread cmd/feedback/feedback.go Outdated
Emoji: "love_letter",
Text: "We would love to know how things are going",
Secondary: []string{
"Share your experience with " + style.Commandf("feedback --name slack-cli-feedback", false),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"Share your experience with " + style.Commandf("feedback --name slack-cli-feedback", false),
"Share your experience with " + style.Commandf("feedback --name slack-cli", false),

Comment thread cmd/feedback/feedback.go Outdated
Example: style.ExampleCommandsf([]style.ExampleCommand{
{Command: "feedback", Meaning: "Open a feedback survey in your browser"},
{Command: "feedback", Meaning: "Choose to give feedback on part of the Slack Platform"},
{Command: "feedback --name slack-cli-feedback", Meaning: "Give feedback on the Slack CLI"},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
{Command: "feedback --name slack-cli-feedback", Meaning: "Give feedback on the Slack CLI"},
{Command: "feedback --name slack-cli", Meaning: "Give feedback on the Slack CLI"},

AFAICT the suggested changes in comments above catch all changes from "slack-cli-feedback" to "slack-cli" if this is a change we're wanting to make!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks! Commits f5b343f and 48802d9 change the --name slack-cli and update the 2 output references!

@mwbrooks

Copy link
Copy Markdown
Member Author

@zimeg Thanks for the review and suggestions on future improvements! I've updated the --name slack-cli and I'll handle a couple of our ideas in the follow-up PR #68. I'm feeling confident that we're leaving the feedback command in a better place than we started! ⛺

@mwbrooks mwbrooks merged commit 8e2e918 into main Apr 29, 2025
@mwbrooks mwbrooks deleted the mwbrooks-feedback-github branch April 29, 2025 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants