-
Notifications
You must be signed in to change notification settings - Fork 678
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
base: feat-ai-apps-thinking-steps
Are you sure you want to change the base?
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!
| export * from './block-kit/composition-objects'; | ||
| export * from './block-kit/extensions'; | ||
| export * from './calls'; | ||
| export * from './chunk'; |
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: Right now I notice CI is failing for a missing export... Am curious if this is specific to our testing workflows or this PR, but will explore this adjacent!
| const chunkObj = chunk as Record<string, unknown>; | ||
|
|
||
| if (!('type' in chunkObj) || typeof chunkObj.type !== 'string') { | ||
| console.warn('Unknown chunk detected and skipped (missing type)', chunk); |
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.
🪵 issue: We should prefer the @slack/logger package for these outputs!
| 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.
| typeof taskChunk.status === 'string' && | ||
| ['pending', 'in_progress', 'complete', 'error'].includes(taskChunk.status) | ||
| ) { | ||
| return chunkObj as unknown as TaskUpdateChunk; |
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.
👾 thought: Morphing to unknown then a type might have an alternative with type "narrowing"! IIRC @WilliamBergamin is most familiar with that approach and has shared these docs with me at a point:
📚 https://www.typescriptlang.org/docs/handbook/2/narrowing.html#the-in-operator-narrowing
| { | ||
| channel: 'C1234', | ||
| ts: '1234.56', | ||
| markdown_text: 'hello', |
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.
| markdown_text: 'hello', |
🪓 issue: Let's remove this to match expected API behavior!
| chunks: [ | ||
| { | ||
| type: 'markdown_text', | ||
| text: 'Hello world', | ||
| }, | ||
| { | ||
| type: 'plan_update', | ||
| title: 'Analyzing request', | ||
| }, | ||
| { | ||
| type: 'task_update', | ||
| id: 'task-1', | ||
| title: 'Processing request', | ||
| status: 'in_progress', | ||
| details: 'Working on it...', | ||
| }, | ||
| ], |
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.
| chunks: [ | |
| { | |
| type: 'markdown_text', | |
| text: 'Hello world', | |
| }, | |
| { | |
| type: 'plan_update', | |
| title: 'Analyzing request', | |
| }, | |
| { | |
| type: 'task_update', | |
| id: 'task-1', | |
| title: 'Processing request', | |
| status: 'in_progress', | |
| details: 'Working on it...', | |
| }, | |
| ], |
🪓 issue: Similar to the above, I'm not sure if we should error in typescript if both "markdown_text" and "chunks" are provided, but let's not showcase that here IMHO
| return null; | ||
| } | ||
|
|
||
| if(type === 'plan_update') { |
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.
| if(type === 'plan_update') { | |
| if (type === 'plan_update') { |
👁️🗨️ issue: This is causing the linter to error and packages to not build as expected!
src/chunk.ts format ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
× Formatter would have printed the following content:
82 │ ··if·(type·===·'plan_update')·{
Checked 48 files in 26ms. No fixes applied.
│ +
Found 1 error.
check ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
🔗 https://github.com/slackapi/node-slack-sdk/actions/runs/21079717525/job/60630237979#step:9:13
Summary
This PR introduces the chunks argument to the following methods:
chat.startStream
chat.appendStream
chat.stopStream
Code Snippet
Requirements (place an
xin each[ ])