-
Notifications
You must be signed in to change notification settings - Fork 48.1k
chore: upgrade @assistant-ui to 0.14 and replace all custom streaming-rendering code with built-in APIs #51653
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
Changes from all commits
6376c69
6a88a08
0d92a99
cab0f69
24a940f
fbe98ca
4b12864
94756bd
27f8d78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,8 @@ import { | |
| import { | ||
| type AssistantRuntime, | ||
| type ExternalStoreAdapter, | ||
| fromThreadMessageLike, | ||
| generateId, | ||
| type ThreadMessage, | ||
| useRuntimeAdapters | ||
| } from '@assistant-ui/react' | ||
|
|
@@ -134,11 +136,19 @@ class IncrementalExternalStoreThreadRuntimeCore extends ExternalStoreThreadRunti | |
| self._notifyEventSubscribers(store.isRunning ? 'runStart' : 'runEnd', {}) | ||
| } | ||
|
|
||
| // metadata.isOptimistic keeps this placeholder ephemeral: core evicts | ||
| // off-branch optimistic messages on head moves and omits them from export(). | ||
| if (hasUpcomingMessage(isRunning, messages)) { | ||
| self._assistantOptimisticId = this.repository.appendOptimisticMessage(messages.at(-1)?.id ?? null, { | ||
| role: 'assistant', | ||
| content: [] | ||
| }) | ||
| const optimisticId = generateId() | ||
| this.repository.addOrUpdateMessage( | ||
| messages.at(-1)?.id ?? null, | ||
| fromThreadMessageLike( | ||
| { role: 'assistant', content: [], metadata: { isOptimistic: true } }, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The inlining of the removed This is the highest-risk spot since it's the one place we hand-reimplement a core method against a core that jumped two minors. Questions:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. faithful, yes. on whether isOptimistic is a contract or an implementation detail: it's a documented public field, not something we're reaching past. core's published types carry the JSDoc on on eviction (Q1): verified clean against the installed 0.2.18. stop bypasses core's |
||
| optimisticId, | ||
| { type: 'running' } | ||
| ) | ||
| ) | ||
| self._assistantOptimisticId = optimisticId | ||
| } | ||
|
|
||
| this.repository.resetHead(self._assistantOptimisticId ?? messages.at(-1)?.id ?? 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.
The pipeline reorder (
smooth → defer → preprocess⟶preprocess → smooth → defer) is not intermediate-frame identical for reasoning text, even though the final render is. In the built-inStreamdownTextPrimitive,useSmoothruns on the already-remend-repaired full text, and the revealed prefix is then rendered withparseIncompleteMarkdown: falseand no per-prefix repair.Consequence (reasoning only — this
MarkdownTextContentpath withsmooth; the body-textMarkdownTextisdefer-only and genuinely equivalent): the OLD code re-rantailBoundedRemendon each revealed prefix, so an incomplete**boldshowed up already-styled during the reveal. The NEW code reveals a prefix of the repaired text, so the opener is shown before its closer →**, backtick,$briefly flash as literal syntax at the typewriter frontier until the reveal catches up.Questions:
SMOOTH_OPTIONSmatches the old constants, but the built-inTextStreamAnimatoruses a different rate algorithm than the deleteduseSmoothReveal(incl. amaxCharIntervalMsdefault of 5ms). Did the reveal cadence visibly match the old feel side-by-side, or just approximately?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.
confirmed, and you're right that this is the one place "end result is identical" overclaims. it's intermediate-frame divergent on the reasoning path only (
MarkdownTextContentwithsmooth; body text isdefer-only and genuinely equivalent). the mechanism is exactly as you describe: the built-in runspreprocesson the full text,useSmoothslices a prefix of the already-repaired string, and because we passparseMarkdownIntoBlocksFnthe built-in tail-remend is gated off (!parseMarkdownIntoBlocksFn) withparseIncompleteMarkdownfalse, so the reveal frontier is never re-repaired and an unclosed**, backtick, or$shows raw until its closer is revealed.treating it as acceptable: cosmetic and reasoning-only. narrowed the PR body to final-state equivalence and fixed the misleading inline comment in fbe98ca. if we decide the flicker isn't shippable, the fix is either dropping
parseMarkdownIntoBlocksFnon the smooth surface (costs the reasoning block-parse cache) or settingparseIncompleteMarkdownnon-false there (full remend per flush on reasoning).on cadence (Q2): approximately, not exact.
maxCharIntervalMsis unset so it defaults to 5ms against the old ~33ms floor, so the tail reveals faster. if the feel is off we can setmaxCharIntervalMsexplicitly to match.