Fix three UI / streaming / search bugs#260
Conversation
commit: |
- createUserUIMessage: spread partCommon on string content path so providerMetadata and streaming state survive onto text parts (#234). - willContinue: count tool-error parts as completed outputs so the agent doesn't stop mid-step when a tool throws under AI SDK v6 (#240). - textSearch: skip the targetMessage.order filter for messages from other threads when searching across a userId, since order is only meaningful within a single thread (#256).
sethconvex
force-pushed
the
bugfix-batch-ui-streaming
branch
from
d940286 to
8d389bc
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d389bc163
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (order && !args.searchAllMessagesForUserId) { | ||
| return q.and(qq, q.lte(q.field("order"), order)); | ||
| } | ||
| return qq; | ||
| }) | ||
| .take(args.limit); |
There was a problem hiding this comment.
When searchAllMessagesForUserId is set, this condition skips the DB-level order predicate, but .take(args.limit) still runs before the in-memory filter that removes messages after targetMessage in the same thread. In workloads where many high-relevance hits are newer messages from the target thread, those hits can consume the limit and then be discarded, causing valid older/cross-thread matches to be omitted entirely. This regresses search recall for targetMessageId queries and can return far fewer than the requested limit even when enough valid matches exist.
Useful? React with 👍 / 👎.
- Adds a regression test for #256 that constructs a target message at order 2 in one thread and a matching message at order 5 in another thread under the same userId. Verified that the test fails against the pre-fix code (which dropped the cross-thread match via the DB-level lte(order) filter) and passes after the fix. - Adds a comment in textSearch noting that with the DB-level order filter removed for cross-thread search, in-thread matches with order >= target.order can be edged out of the take-limit by cross-thread matches. Acceptable tradeoff for now; documented for follow-up.
ianmacartney
left a comment
ianmacartney
left a comment
There was a problem hiding this comment.
lgtm - I had started branch issue-256 and will copy over one addition from there (where it does the same order fix, but for vector search too). I had also updated tests and fixed some lint errors will update this branch for you to inspect & land if you're happy with it
The vector-search code path in _fetchSearchMessages had the same cross-thread defect as textSearch: when searching across threads with searchAllMessagesForUserId, results were filtered against the target message's order, which only makes sense within a single thread. Mirror the textSearch fix so cross-thread vector hits pass through the beforeMessage order check.
2 participants
Summary
Two small, independent bug fixes — each with the exact root cause already identified in the issue report.
createUserUIMessagewas not spreadingpartCommonon the string-content path, soproviderMetadataand thestreaming/donestate were dropped when a user message was saved with a plain stringprompt. The array-content path already spreadpartCommon; the assistant and system variants of this function did so on both paths. Fix mirrors them.textSearchapplied thetargetMessage.orderupper-bound to all search hits, including messages from other threads whensearchAllMessagesForUserIdwas set. Sinceorderis only meaningful within a single thread, valid cross-thread matches were dropped whenever the new thread's current message had a lowerorder. The DB-levellte(order)predicate is now skipped for cross-thread searches, and the post-filter only enforces the order constraint within the samethreadId.Closes #257
Test plan
npm test— all 262 tests pass.src/toUIMessages.test.tsupdated: the user-message-with-string-content test was asserting the buggy shape (nostate); now matches the assistant path which already expectedstate: "done".saveMessage({ prompt, metadata: { providerMetadata } })call.Notes
willContinueignoringtool-errorparts); that fix landed in fix: handle tool validation errors in stopWhen and willContinue #241 and has been dropped from this branch via rebase.npm run typecheckerrors inexample/convex/setup.test.tsare present onmainand unrelated to this change.