-
Notifications
You must be signed in to change notification settings - Fork 679
feat(types): add streaming_state to bot_message event #2371
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.
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## ai-apps #2371 +/- ##
==========================================
Coverage ? 92.78%
==========================================
Files ? 39
Lines ? 10711
Branches ? 692
==========================================
Hits ? 9938
Misses ? 761
Partials ? 12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
mwbrooks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Thanks a bunch for adding this event type!
🧪 Confirmed that streaming_state appears as a type for the message event.
❓ Should this belong to more than just the BotMessageEvent? For example, the MessageChangedEvent?
| event_ts: string; | ||
| channel: string; | ||
| channel_type: ChannelTypes; | ||
| streaming_state?: 'in_progress' | 'completed' | 'errored'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Matches the schema! 👌🏻
nit: We may want to pull the values out into an enum at the top of the file, similar to ChannelTypes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Should this belong to more than just the
BotMessageEvent? For example, theMessageChangedEvent?nit: We may want to pull the values out into an enum at the top of the file, similar to
ChannelTypes.
@mwbrooks I was curious about this as well - at the moment I find a "bot_message" subtype within a "message_changed" event as the message but I'm not sure if it can be found in other events...
node-slack-sdk/packages/types/src/events/message.ts
Lines 230 to 240 in 4b8bfec
| export interface MessageChangedEvent { | |
| type: 'message'; | |
| subtype: 'message_changed'; | |
| event_ts: string; | |
| hidden: true; | |
| channel: string; | |
| channel_type: ChannelTypes; | |
| ts: string; | |
| message: MessageEvent; | |
| previous_message: MessageEvent; | |
| } |
Am curious about this and think we could adjust to a separate enum if this is found elsewhere? 👻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: @zimeg and I chatted offline to make sure that message_changed events from bots will include the streaming_state arg. This arg isn't available on non-bot message changes, so it's looking good.
note: We won't bother turning the values into an enum until we repeat it in 2 places. 👌🏻
|
Merge away @zimeg! 🚀 |
|
@mwbrooks Thanks so much for sharing in these discussions and the kind review! I like what we have for now but am so open to revisiting in a change of events. |
Summary
This PR adds
streaming_stateto thebot_messagemessage event subtype.Reviewers
Confirm that this field appears in message changed event 🤖
Requirements