-
Notifications
You must be signed in to change notification settings - Fork 171
Fix chunk newline. #1017
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
Fix chunk newline. #1017
Changes from 5 commits
c2d4039
47d4c5d
1897044
8b8221c
ef5ddb4
ed66138
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 |
|---|---|---|
|
|
@@ -121,7 +121,7 @@ sealed class ChatSession extends ChangeNotifier { | |
| _currentAiMessage = Message(isUser: false, text: ''); | ||
| _messages.add(_currentAiMessage!); | ||
| } | ||
| _currentAiMessage!.text = (_currentAiMessage!.text ?? '') + chunk; | ||
| _currentAiMessage!.text = '${_currentAiMessage!.text ?? ''}\n$chunk'; | ||
|
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. Are you sure this works as expected? My understanding is that the LLM emits a sequence of chunks structured however it wants, and there are no guarantees that the chunk boundary coincides with a newline, so therefore we should not inject additional newlines automatically, because they might be in the wrong place. I don't fully understand the problem you're fixing, but if it's a lack of whitespace in the layout, perhaps this could be accomplished by post-processing the messages at render time to non-destructively add a newline at the end, rather than destructively adding it here in the chunk processing logic.
Collaborator
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. Check screensots and comments on the issue to better understand both issue and fix: a2ui-project/a2ui#1891
Collaborator
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. new line in most cases is noop for markdown, that makes me think the fix is right
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. Hey Polina! Sorry I should have looked at the linked issue first to get more context - I'll do that next time. I do think that it could cause problems to add a newline here because it's not always a no-op in markdown and could accidentally break words. Analysis from GeminiIt turns out the issue isn't that the LLM is omitting spaces between chunks, but rather that Specifically, in @override
Stream<String> get incomingText => _pipeline
.where((e) => e is TextEvent)
.cast<TextEvent>()
.map((e) => e.text.trim()) // <-- Strips leading/trailing whitespace from every chunk!
.where((text) => text.isNotEmpty);Because .trim() is called on every TextEvent chunk before it reaches _updateAiMessage, any trailing or leading whitespace in streamed chunks (e.g. "ty of friendly ") gets stripped. When _updateAiMessage concatenates these trimmed chunks, words smash together across boundaries (e.g. "friendlyspots").Adding \n before every chunk in chat_session.dart forces literal line breaks inside sentences when chunks split mid-sentence or mid-word, leading to broken streaming rendering and unexpected formatting in Markdown (such as inside code blocks, tables, or lists). Suggested FixInstead of appending in chat_session.dart, we should remove
Collaborator
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. that's cool! I am impressed. which model did you use? applied the fix
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. I think that was just flash actually! I'll send you a screenshot of my workflow :-D |
||
| notifyListeners(); | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.