Fix webchat ghost bubble when model replies with NO_REPLY#15118
Fix webchat ghost bubble when model replies with NO_REPLY#15118jwchmodx wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Resolves openclaw#15060 When the model returns NO_REPLY during streaming, the webchat was rendering an empty bubble. This fix detects NO_REPLY text in the renderGroupedMessage function and returns nothing to prevent the ghost bubble from appearing. Changes: - Added isSilentReplyText() helper to detect NO_REPLY messages - Modified renderGroupedMessage() to skip rendering when text is NO_REPLY - Exported renderGroupedMessage() to enable unit testing - Added comprehensive tests for NO_REPLY scenarios Test plan: - ✅ All 7 new NO_REPLY tests pass - ✅ All existing UI tests pass (199/204 total, 5 pre-existing failures) - ✅ pnpm build succeeds - ✅ pnpm check passes (formatting, type-checking, linting) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| /** | ||
| * Checks if the text represents a silent reply (NO_REPLY). | ||
| * This is a UI-compatible implementation matching the server-side isSilentReplyText function. | ||
| */ | ||
| function isSilentReplyText(text: string | null | undefined): boolean { | ||
| if (!text) { | ||
| return false; | ||
| } | ||
| const token = "NO_REPLY"; | ||
| // Check if NO_REPLY appears at the start or end of the text | ||
| const trimmed = text.trim(); | ||
| if (trimmed.startsWith(token)) { | ||
| return true; | ||
| } | ||
| return trimmed.endsWith(token); | ||
| } |
There was a problem hiding this comment.
Silent token mismatch
isSilentReplyText() here doesn't match the server implementation it references. Server-side src/auto-reply/tokens.ts requires word/non-word boundaries (e.g. ^\s*NO_REPLY(?=$|\W) or \bNO_REPLY\b\W*$), but this UI version suppresses any text whose trimmed content merely startsWith/endsWith NO_REPLY. That will incorrectly hide messages like "NO_REPLYx" or "fooNO_REPLY" that the server would not treat as silent, potentially dropping real assistant output in webchat.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/chat/grouped-render.ts
Line: 15:30
Comment:
**Silent token mismatch**
`isSilentReplyText()` here doesn't match the server implementation it references. Server-side `src/auto-reply/tokens.ts` requires word/non-word boundaries (e.g. `^\s*NO_REPLY(?=$|\W)` or `\bNO_REPLY\b\W*$`), but this UI version suppresses any text whose trimmed content merely `startsWith`/`endsWith` `NO_REPLY`. That will incorrectly hide messages like `"NO_REPLYx"` or `"fooNO_REPLY"` that the server would not treat as silent, potentially dropping real assistant output in webchat.
How can I resolve this? If you propose a fix, please make it concise.| }); | ||
|
|
||
| it("returns nothing when message text has NO_REPLY at start", () => { | ||
| const message = { | ||
| role: "assistant", | ||
| content: [{ type: "text", text: "NO_REPLY (system message)" }], | ||
| timestamp: Date.now(), | ||
| }; | ||
| const result = renderGroupedMessage(message, { isStreaming: false, showReasoning: false }); | ||
| expect(result).toBe(nothing); | ||
| }); |
There was a problem hiding this comment.
Tests lock wrong behavior
This suite asserts suppression for strings like "NO_REPLY (system message)", but the referenced server-side silent-reply check is boundary-based (see src/auto-reply/tokens.ts) and is not equivalent to a simple prefix/suffix substring match. If the intent is to mirror server semantics, add boundary-focused cases (e.g. NO_REPLYx / xNO_REPLY) and align the helper accordingly; otherwise these tests will cement UI-only behavior divergence.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/chat/grouped-render.test.ts
Line: 34:44
Comment:
**Tests lock wrong behavior**
This suite asserts suppression for strings like `"NO_REPLY (system message)"`, but the referenced server-side silent-reply check is boundary-based (see `src/auto-reply/tokens.ts`) and is not equivalent to a simple prefix/suffix substring match. If the intent is to mirror server semantics, add boundary-focused cases (e.g. `NO_REPLYx` / `xNO_REPLY`) and align the helper accordingly; otherwise these tests will cement UI-only behavior divergence.
How can I resolve this? If you propose a fix, please make it concise.- Use regex with word boundaries instead of simple startsWith/endsWith - Matches server-side implementation in src/auto-reply/tokens.ts - Add tests for word boundary cases (NO_REPLYx, xNO_REPLY should render) - Fixes false suppression of valid assistant messages
|
Hi! The macOS and Windows CI jobs seem to have timed out (5h+ and 1h+ respectively). Could a maintainer please re-run the failed jobs? The code changes are unrelated to these platform-specific tests. Thanks! 🙏 |
bfc1ccb to
f92900f
Compare
Summary
Fixes #15060
When the model returns
NO_REPLYduring streaming, the webchat was rendering an empty bubble artifact. This PR detectsNO_REPLYtext in the message rendering logic and prevents the ghost bubble from appearing.Changes
isSilentReplyText()helper function to detect NO_REPLY messages in the UIrenderGroupedMessage()to skip rendering when message text is NO_REPLYrenderGroupedMessage()to enable unit testingTest plan
pnpm buildsucceedspnpm checkpasses (formatting, type-checking, linting)Technical details
The fix works by checking the extracted text before rendering and returning
nothingfrom Lit when the text matchesNO_REPLY. This prevents the bubble from being created in the DOM.🤖 Generated with Claude Code
Greptile Overview
Greptile Summary
This PR prevents “ghost” webchat bubbles by treating
NO_REPLYassistant outputs as silent and returning Lit’snothingfromrenderGroupedMessage().Changes are localized to
ui/src/ui/chat/grouped-render.ts(newisSilentReplyText()and exportingrenderGroupedMessage()for testing) plus a new Vitest suite covering severalNO_REPLY/empty-text scenarios.One correctness concern: the UI
isSilentReplyText()implementation diverges from the server’s boundary-basedisSilentReplyTextinsrc/auto-reply/tokens.ts, which can cause the UI to suppress messages the server would not consider silent and the new tests currently lock in that divergent behavior.Confidence Score: 3/5
isSilentReplyText()does not match the server’s boundary-based definition, so it can suppress non-silent outputs; the accompanying tests currently encode that behavior.Last reviewed commit: d950f43
(4/5) You can add custom instructions or style guidelines for the agent here!