-
Notifications
You must be signed in to change notification settings - Fork 65
strip out old streams when they are no longer present #186
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: main
Are you sure you want to change the base?
Conversation
commit: |
2cb8acb to
c2db7f0
Compare
cf2fb4d to
50d5d69
Compare
WalkthroughThis change optimizes cursor state updates in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
example/convex/workflows/chaining.ts (1)
58-82: Based on my verification, I've found critical issues with the review comment. The web search and codebase analysis reveal:
- API change direction is backwards: Version 0.3.2 changed the parameter FROM
ctxTOstep(not the reverse as stated)- Codebase incompatibility: The entire codebase uniformly uses the OLD
ctxAPI, but the package.json specifies@convex-dev/workflow@0.3.2, which requires the NEWstepAPI- No handlers use the correct API: I found zero occurrences of handlers using the
stepparameterThe review comment is factually incorrect and the actual issue is more severe. Here is the rewritten review comment:
Update workflow handlers to use
stepparameter for @convex-dev/workflow@0.3.2 compatibility.Version 0.3.2 of @convex-dev/workflow introduces a breaking API change requiring handlers to use a
stepparameter instead ofctx. The code currently uses the pre-0.3.2 API (e.g.,ctx.runAction()at line 67-70), which is incompatible with the installed version. All handlers must be migrated: changeasync (ctx, { ... })toasync (step, { ... })and replacectx.runAction()calls withstep.runAction().
🧹 Nitpick comments (3)
src/client/streamText.ts (3)
77-77: Unnecessary nullish coalescing on required parameter.The
optionsparameter is required in the function signature, so the?? {}fallback is never reached.- const { threadId } = options ?? {}; + const { threadId } = options;
100-100: Inconsistent optional chaining on required parameter.
optionsis a required parameter, so optional chaining is unnecessary here.- agentName: options?.agentName, + agentName: options.agentName,
129-133: Address or document the commented-out streamer metadata update.This commented-out code suggests an incomplete feature for updating streamer metadata when the model changes during multi-step execution. Consider either implementing this or adding a TODO comment explaining why it's deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)example/convex/workflows/chaining.ts(1 hunks)package.json(2 hunks)src/client/index.ts(4 hunks)src/client/saveInputMessages.ts(1 hunks)src/client/start.ts(3 hunks)src/client/streamText.ts(1 hunks)src/client/utils.ts(1 hunks)src/deltas.ts(1 hunks)src/react/useDeltaStreams.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
example/convex/workflows/chaining.ts (1)
src/client/index.ts (2)
saveMessage(139-139)saveMessage(694-723)
src/client/streamText.ts (5)
src/client/types.ts (3)
AgentPrompt(41-81)Options(588-609)GenerationOutputMetadata(232-253)src/client/streaming.ts (2)
StreamingOptions(134-156)mergeTransforms(171-192)src/client/start.ts (1)
startGeneration(30-310)src/shared.ts (2)
getModelName(94-104)getProviderName(106-111)src/client/utils.ts (1)
errorToString(22-27)
src/client/saveInputMessages.ts (1)
playground/bin/agent-playground.cjs (1)
args(15-15)
src/client/index.ts (1)
src/client/streamText.ts (1)
streamText(33-163)
🔇 Additional comments (15)
package.json (1)
10-10: LGTM! Version bumps are consistent.The package version bump to 0.3.1 aligns with the CHANGELOG entries, and the workflow dependency update to 0.3.2 matches the API changes in the workflow example file.
Also applies to: 90-90
src/deltas.ts (1)
80-83: Excellent fix for the performance issue!This change directly addresses issue #187 by removing the eager
JSON.stringifyfrom the assertion message. The simplified message still conveys the intent without the expensive serialization that was causing UI slowdowns during streaming.src/client/saveInputMessages.ts (1)
82-93: LGTM! Correctly propagates promptMessageId.This change ensures that
promptMessageIdis preserved when saving messages, which aligns with the CHANGELOG entry about preventing messages from being saved as newest whenpromptMessageIdwasn't latest.CHANGELOG.md (1)
3-9: LGTM! Changelog accurately describes the changes.The 0.3.1 entry clearly documents both the performance fix (avoiding JSON.stringify in render path) and the promptMessageId preservation enhancement.
src/react/useDeltaStreams.ts (1)
106-150: Excellent fix for issue #185!This refactor properly handles the cleanup of streaming messages when streams are no longer present, which directly addresses the duplicate message problem described in issue #185. The key improvements are:
- Stream removal detection (lines 142-147): Detects when streams disappear from the active set and marks state as changed, ensuring stale stream deltas are removed from the UI.
- Conditional state updates (lines 148-150): Only calls
setCursorswhen actual changes occur, reducing unnecessary re-renders.- Proper guard ordering (line 106, 108): Processes delta updates when
streamMessagesexist, with nested guard for actual deltas.The logic correctly handles the lifecycle: when a stream completes and the backend no longer returns it in the stream list, this code will detect the removal and trigger a state update to clean it up.
src/client/start.ts (1)
93-93: LGTM! Good use of optional parameter with runtime validation.Making
languageModeloptional in the API while asserting at runtime (line 158) provides flexibility while maintaining safety. The model is resolved from eitherargs.modeloropts.languageModel, with a clear error if neither is provided.Also applies to: 157-158
src/client/utils.ts (2)
22-27: LGTM! Simple and robust error conversion.The
errorToStringutility correctly handles bothErrorinstances and other types, providing a reliable way to convert any thrown value to a string.
3-20: I'll help you verify the concerns about thewillContinuefunction's continuation logic. Let me start by searching for usage patterns and understanding the intended behavior.
<function_calls>
#!/bin/bashSearch for usage patterns of willContinue to understand expected behavior
rg -n -C3 'willContinue\s*(' --type=ts
</function_calls>Now let me also search for any test files or documentation related to this function:
<function_calls>
#!/bin/bashSearch for test files that might test willContinue
fd -e test.ts -e spec.ts -e test.js | head -20
Also search for any documentation or comments about stopWhen
rg -n 'stopWhen' --type=ts -C2 | head -50
</function_calls><function_calls>
#!/bin/bashCheck the type definitions for StopCondition to understand the contract
rg -n 'StopCondition' --type=ts -B2 -A5
</function_calls><function_calls>
#!/bin/bashLook at the full context of the utils.ts file to understand the function better
wc -l src/client/utils.ts
</function_calls><function_calls>
#!/bin/bashRead the full utils.ts file to get complete context
cat -n src/client/utils.ts
</function_calls>src/client/streamText.ts (3)
1-24: LGTM!Imports are well-organized, pulling types and utilities from the appropriate modules. The delegation to
startGeneration, utility functions (errorToString,willContinue), and streaming constructs aligns with the modular design described in the PR.
26-76: LGTM!The function signature and documentation are clear. The type parameters and option types properly support the streaming use case with optional delta persistence.
145-162: LGTM!The stream consumption logic correctly handles the
returnImmediatelyoption and builds the appropriate metadata. The conditional await pattern ensures the stream is fully consumed when required.src/client/index.ts (4)
19-19: LGTM!The
streamTextimport from "ai" is correctly removed as the local wrapper is now used instead.
66-66: LGTM!Import correctly updated to pull only what's needed from the streaming module.
90-91: LGTM!The imports for the extracted
streamTextfunction and utilities (errorToString,willContinue) correctly support the refactored code structure.
543-562: LGTM!The delegation to the extracted
streamTextfunction is clean. The argument merging correctly prioritizes explicitstreamTextArgsvalues over agent defaults, and the options spread order is appropriate.
50d5d69 to
00d3861
Compare
Cleans up streaming messages from the list when no new deltas arrive but a stream is no longer present.
Fixes #185
Fixes #187
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.