-
Notifications
You must be signed in to change notification settings - Fork 679
feat: accept chunks as arguments to chat.{start,append,stop}Stream methods #2467
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 @@
## feat-ai-apps-thinking-steps #2467 +/- ##
============================================================
Coverage 93.09% 93.09%
============================================================
Files 40 40
Lines 11240 11240
Branches 713 713
============================================================
Hits 10464 10464
Misses 764 764
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
zimeg
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.
@srtaalej Woooo! This'll be nice to land soon but I'm leaving a few comments on changes perhaps to make beforehand.
The tests are most confusing to me since I think all changes are contained in this PR... Am looking into this separate but do let me know if questions on these comments appear please!
packages/types/src/chunk.ts
Outdated
| if(type === 'plan_update') { | ||
| if (typeof chunkObj.title === 'string') { | ||
| return chunkObj as unknown as PlanUpdateChunk; | ||
| } | ||
| console.warn('Invalid PlanUpdateChunk (missing title property)', chunk); | ||
| return null; | ||
| } |
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.
Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
Co-authored-by: Eden Zimbelman <eden.zimbelman@salesforce.com>
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.
🙌🏻 This is looking great @srtaalej! Thanks for aligning so closely with the Python SDK implement as well.
🧪 I've double-checked that the params are correct and thanks for adding the detailed tests to chat.test-d.ts. 🙇🏻
Here is my modification to our sample app's listeners/assistant/message.ts method to test the chat.startStream, chat.appendStream, and chat.stopStream with markdown_text, and task_update. I don't think I can test plan_update until the PR that adds task_display_mode.
listeners/assistant/message.ts
import { DEFAULT_SYSTEM_CONTENT, openai } from '../../ai/index.js';
import { feedbackBlock } from '../views/feedback_block.js';
const delay = (ms) => new Promise(resolve => setTimeout(resolve, ms));
/**
* The `message` event is sent when the user direct messages the app in a DM or Assistant container.
*
* @param {Object} params
* @param {import("@slack/web-api").WebClient} params.client - Slack web client.
* @param {import("@slack/bolt").Context} params.context - Event context.
* @param {import("@slack/logger").Logger} params.logger - Logger instance.
* @param {import("@slack/types").MessageEvent} params.message - The incoming message.
* @param {Function} params.getThreadContext - Function to get thread context.
* @param {import("@slack/bolt").SayFn} params.say - Function to send messages.
* @param {Function} params.setTitle - Function to set assistant thread title.
* @param {Function} params.setStatus - Function to set assistant status.
*
* @see {@link https://docs.slack.dev/reference/events/message}
*/
export const message = async ({ client, context, logger, message, getThreadContext, say, setTitle, setStatus }) => {
/**
* Messages sent to the Assistant can have a specific message subtype.
*
* Here we check that the message has "text" and was sent to a thread to
* skip unexpected message subtypes.
*
* @see {@link https://docs.slack.dev/reference/events/message#subtypes}
*/
if (!('text' in message) || !('thread_ts' in message) || !message.text || !message.thread_ts) {
return;
}
const { channel, thread_ts } = message;
const { userId, teamId } = context;
try {
/**
* Set the title of the Assistant thread to capture the initial topic/question
* as a way to facilitate future reference by the user.
*
* @see {@link https://docs.slack.dev/reference/methods/assistant.threads.setTitle}
*/
await setTitle(message.text);
/**
* Set the status of the Assistant to give the appearance of active processing.
*
* @see {@link https://docs.slack.dev/reference/methods/assistant.threads.setStatus}
*/
await setStatus({
status: 'thinking...',
loading_messages: [
'Teaching the hamsters to type faster…',
'Untangling the internet cables…',
'Consulting the office goldfish…',
'Polishing up the response just for you…',
'Convincing the AI to stop overthinking…',
],
});
const streamResponse = await client.chat.startStream({
channel: channel,
thread_ts: thread_ts,
recipient_team_id: teamId,
recipient_user_id: userId,
chunks: [{
type: 'markdown_text',
text: '**Hello!** I am starting to process your request...\n\n',
}, {
type: 'task_update',
id: '12',
title: 'counting bytes...',
status: 'in_progress',
}],
});
await delay(4000);
await client.chat.appendStream({
channel: channel,
ts: streamResponse.ts,
// markdown_text: '',
chunks: [{
type: 'task_update',
id: '12',
title: 'adding numbers...',
status: 'in_progress',
details: 'sums have increased',
}],
});
await delay(4000);
await client.chat.stopStream({
channel: channel,
ts: streamResponse.ts,
chunks: [{
type: 'task_update',
id: '12',
title: 'solved equation!',
status: 'complete',
sources: [{
type: 'url',
url: 'https://oeis.org',
text: 'The On-Line Encyclopedia of Integer Sequences (OEIS)',
}],
}, {
type: 'markdown_text',
text: 'that computes.',
}],
});
} catch (e) {
logger.error(e);
// Send message to advise user and clear processing status if a failure occurs
await say({ text: `Sorry, something went wrong! ${e}` });
}
};📝 I left a suggestion to remove the parseChunk and npm dependency. After that's done, I think we can ✅ and merge! ![]()
packages/types/src/chunk.ts
Outdated
| /** | ||
| * Parse a chunk object and return the appropriate typed chunk. | ||
| * Returns null if the chunk is invalid or unknown. | ||
| */ | ||
| export function parseChunk(chunk: unknown): AnyChunk | null { |
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.
suggestion: This is a nice implementation, but I don't think we need it for the @slack/types package. This parsing was necessary for Python, but in TypeScript I think the AnyChunk union that you created will serve well! :two-cents:
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.
ahh okay thank you for the clarification 🙌
packages/types/package.json
Outdated
| "dependencies": { | ||
| "@slack/logger": "^4.0.0" | ||
| }, |
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.
suggestion: If we remove the parseChunk method, then we can also remove this dependency 🎉
| expectError( | ||
| web.chat.appendStream({ | ||
| channel: 'C1234', // missing_markdown_text | ||
| ts: '1234.56', | ||
| }), | ||
| ); |
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.
suggestion: @srtaalej I manually linked the new @slack/types into this package and ran npm test. It looks like this test now no longer errors because markdown_text is now optional. However, I'd still expect it to error when markdown_text and chunks are both missing. 🤔 @zimeg Do you think this error should come from the Slack API or should our SDK check that at least one of these two params exist?
> @slack/web-api@7.13.0 test:types
> tsd
test/types/methods/chat.test-d.ts:21:0
✖ 21:0 Expected an error, but found none.
1 errorThere 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.
👁️🗨️ @mwbrooks Nice catch! I'd lean toward documenting that either markdown_text or chunks is required and letting the API error instead of leaning on typescript options.
🗣️ ramble: That might be personal preference, but IMHO these options don't catch runtime nuance that the call to an API would anyways. An empty markdown_text string IIRC causes a no_text error from API but the required markdown_text appears alright.
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 for the update @srtaalej! 🙇🏻
🧪 Things look good on my side and tests pass locally when I npm link the @slack/types package in the package/web-api.
Let's merge this and keep moving forward! 🚀
Summary
This PR introduces the chunks argument to the following methods:
chat.startStream
chat.appendStream
chat.stopStream
Code Snippet
Requirements (place an
xin each[ ])