[#2319] Fix Assistant utility types #2325
Conversation
misscoded
added
bug
| say: SayFn; | ||
| setStatus: SetStatusFn; | ||
| setSuggestedPrompts: SetSuggestedPromptsFn; | ||
| setTitle: SetTitleFn; | ||
| } | ||
|
|
||
| type GetThreadContextUtilFn = () => Promise<AssistantThreadContext>; |
There was a problem hiding this comment.
Despite the ongoing issue with non-ack'd events, it appears to work as expected. This update corresponds to the enrich function. I believe I've gotten the pass-through correct:
// Do not pass preparedArgs (ie, do not add utilities to get/save)
preparedArgs.getThreadContext = () => threadContextStore.get(args);
preparedArgs.saveThreadContext = () => threadContextStore.save(args);
There was a problem hiding this comment.
Looks good to me and this signature matches the assertion expectations in the matching tests 💯
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2325 +/- ##
=======================================
Coverage 91.08% 91.08%
=======================================
Files 22 22
Lines 6116 6116
Branches 655 655
=======================================
Hits 5571 5571
Misses 540 540
Partials 5 5 ☔ View full report in Codecov by Sentry. |
| @@ -310,7 +310,7 @@ function createSay(args: AllAssistantMiddlewareArgs): SayFn { | |||
| const { channelId: channel, threadTs: thread_ts, context } = extractThreadInfo(payload); | |||
|
|
|||
| return async (message: Parameters<SayFn>[0]) => { | |||
| const threadContext = context.channel_id ? context : await args.getThreadContext(args); | |||
There was a problem hiding this comment.
Since args is passed through in the enrich function, the removal of this additional pass-through shouldn't have an effect.
There was a problem hiding this comment.
Ohhh nice catch. This took me a while to parse - I kept confusing the middleware arguments' getThreadContext method with ThreadContextStore.get.
| }, | ||
| }); | ||
|
|
||
| // threadContextStore tests |
| }, | ||
| }); | ||
|
|
||
| // threadContextStore tests |
There was a problem hiding this comment.
These tests look great, they cover all the utility functions mentioned in #2319 so I'm happy 👍
| say: SayFn; | ||
| setStatus: SetStatusFn; | ||
| setSuggestedPrompts: SetSuggestedPromptsFn; | ||
| setTitle: SetTitleFn; | ||
| } | ||
|
|
||
| type GetThreadContextUtilFn = () => Promise<AssistantThreadContext>; |
There was a problem hiding this comment.
Looks good to me and this signature matches the assertion expectations in the matching tests 💯
| @@ -310,7 +310,7 @@ function createSay(args: AllAssistantMiddlewareArgs): SayFn { | |||
| const { channelId: channel, threadTs: thread_ts, context } = extractThreadInfo(payload); | |||
|
|
|||
| return async (message: Parameters<SayFn>[0]) => { | |||
| const threadContext = context.channel_id ? context : await args.getThreadContext(args); | |||
There was a problem hiding this comment.
Ohhh nice catch. This took me a while to parse - I kept confusing the middleware arguments' getThreadContext method with ThreadContextStore.get.
Summary
Fixes #2319. Adds type tests (courtesy of @filmaj).
Requirements (place an
xin each[ ])