fix(ui): avoid rerendering chat thread on draft changes#54905
Closed
a77ming wants to merge 1 commit into
Closed
Conversation
Contributor
Greptile SummaryThis PR introduces a Lit Confidence Score: 5/5Safe to merge; the optimisation is well-targeted and the regression test covers the primary concern. The change is narrowly scoped, correctly designed, and backed by new tests. The only finding (stale callback props in guard keys) is a low-probability edge case that won't manifest unless callers recreate those specific function references independently of any message/session state change — and even then it degrades gracefully rather than crashing. No data-loss or security risk is introduced. ui/src/ui/views/chat.ts — verify the guard key array is kept in sync if new callback props are added to ChatProps in future.
|
| Filename | Overview |
|---|---|
| ui/src/ui/chat/deleted-messages.ts | Adds a _version counter and version getter to DeletedMessages. bumpVersion() is called unconditionally at the end of both load() and save(), meaning the initial constructor load sets version to 1 and every subsequent mutation increments it — correct behaviour for use as a guard dependency key. |
| ui/src/ui/views/chat.ts | Wraps the entire chat thread template in a Lit guard() directive keyed on all meaningful state except some callback props (onChatScroll, onOpenSidebar, onRequestUpdate), which are captured by the factory closure but absent from the key array — a minor stale-handler risk. All other guard key choices look correct. |
| ui/src/ui/views/chat.thread-cache.test.ts | New test file with two focused cases: draft-only change does not rebuild the message thread, and a message-list change does. Uses vi.hoisted + vi.mock correctly and cleans up module state via cleanupChatModuleState() in afterEach. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: ui/src/ui/views/chat.ts
Line: 921-1019
Comment:
**Callback props not included in guard keys**
`props.onChatScroll` (line 925), `props.onOpenSidebar` (lines 989, 999), and the `requestUpdate` closure derived from `props.onRequestUpdate` (line 1009) are captured inside the guard factory but are **not** listed in the guard's dependency keys. Lit's `guard()` returns `noChange` — skipping all DOM diffing — when the keys are identical, which means updated handler references will be silently ignored until some other key changes.
In practice this is low-risk if the calling component passes stable method references, but if `renderChat` is ever called with freshly-created arrow functions for these props (common in hook-based patterns), the bound handlers will become stale. Adding them to the guard key array keeps the optimisation intact while removing the latent footgun:
```suggestion
const thread = guard(
[
props.loading,
props.messages,
props.toolMessages,
props.streamSegments,
props.stream,
props.streamStartedAt,
props.showToolCalls,
showReasoning,
props.sessionKey,
props.assistantName,
assistantIdentity.avatar,
props.basePath,
activeSession?.contextTokens ?? null,
props.sessions?.defaults?.contextTokens ?? null,
vs.searchOpen,
vs.searchQuery,
deleted.version,
props.onChatScroll,
props.onOpenSidebar,
props.onRequestUpdate,
],
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(ui): avoid rerendering chat thread o..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+921
to
+1019
| <div | ||
| class="chat-thread" | ||
| role="log" | ||
| aria-live="polite" | ||
| @scroll=${props.onChatScroll} | ||
| @click=${handleCodeBlockCopy} | ||
| > | ||
| <div class="chat-thread-inner"> | ||
| ${ | ||
| props.loading | ||
| ? html` | ||
| <div class="chat-loading-skeleton" aria-label="Loading chat"> | ||
| <div class="chat-line assistant"> | ||
| <div class="chat-msg"> | ||
| <div class="chat-bubble"> | ||
| <div class="skeleton skeleton-line skeleton-line--long" style="margin-bottom: 8px"></div> | ||
| <div class="skeleton skeleton-line skeleton-line--medium" style="margin-bottom: 8px"></div> | ||
| <div class="skeleton skeleton-line skeleton-line--short"></div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div class="chat-line user" style="margin-top: 12px"> | ||
| <div class="chat-msg"> | ||
| <div class="chat-bubble"> | ||
| <div class="skeleton skeleton-line skeleton-line--medium"></div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div class="chat-line assistant" style="margin-top: 12px"> | ||
| <div class="chat-msg"> | ||
| <div class="chat-bubble"> | ||
| <div class="skeleton skeleton-line skeleton-line--long" style="margin-bottom: 8px"></div> | ||
| <div class="skeleton skeleton-line skeleton-line--short"></div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ` | ||
| : nothing | ||
| } | ||
| ${isEmpty && !vs.searchOpen ? renderWelcomeState(props) : nothing} | ||
| ${ | ||
| isEmpty && vs.searchOpen | ||
| ? html` | ||
| <div class="agent-chat__empty">No matching messages</div> | ||
| ` | ||
| : nothing | ||
| } | ||
| ${repeat( | ||
| chatItems, | ||
| (item) => item.key, | ||
| (item) => { | ||
| if (item.kind === "divider") { | ||
| return html` | ||
| <div class="chat-divider" role="separator" data-ts=${String(item.timestamp)}> | ||
| <span class="chat-divider__line"></span> | ||
| <span class="chat-divider__label">${item.label}</span> | ||
| <span class="chat-divider__line"></span> | ||
| </div> | ||
| `; | ||
| } | ||
| if (item.kind === "reading-indicator") { | ||
| return renderReadingIndicatorGroup(assistantIdentity, props.basePath); | ||
| } | ||
| if (item.kind === "stream") { | ||
| return renderStreamingGroup( | ||
| item.text, | ||
| item.startedAt, | ||
| props.onOpenSidebar, | ||
| assistantIdentity, | ||
| props.basePath, | ||
| ); | ||
| } | ||
| if (item.kind === "group") { | ||
| if (deleted.has(item.key)) { | ||
| return nothing; | ||
| ` | ||
| : nothing | ||
| } | ||
| return renderMessageGroup(item, { | ||
| onOpenSidebar: props.onOpenSidebar, | ||
| showReasoning, | ||
| showToolCalls: props.showToolCalls, | ||
| assistantName: props.assistantName, | ||
| assistantAvatar: assistantIdentity.avatar, | ||
| basePath: props.basePath, | ||
| contextWindow: | ||
| activeSession?.contextTokens ?? props.sessions?.defaults?.contextTokens ?? null, | ||
| onDelete: () => { | ||
| deleted.delete(item.key); | ||
| requestUpdate(); | ||
| ${isEmpty && !vs.searchOpen ? renderWelcomeState(props) : nothing} | ||
| ${ | ||
| isEmpty && vs.searchOpen | ||
| ? html` | ||
| <div class="agent-chat__empty">No matching messages</div> | ||
| ` | ||
| : nothing | ||
| } | ||
| ${repeat( | ||
| chatItems, | ||
| (item) => item.key, | ||
| (item) => { | ||
| if (item.kind === "divider") { | ||
| return html` | ||
| <div class="chat-divider" role="separator" data-ts=${String(item.timestamp)}> | ||
| <span class="chat-divider__line"></span> | ||
| <span class="chat-divider__label">${item.label}</span> | ||
| <span class="chat-divider__line"></span> | ||
| </div> | ||
| `; | ||
| } | ||
| if (item.kind === "reading-indicator") { | ||
| return renderReadingIndicatorGroup(assistantIdentity, props.basePath); | ||
| } | ||
| if (item.kind === "stream") { | ||
| return renderStreamingGroup( | ||
| item.text, | ||
| item.startedAt, | ||
| props.onOpenSidebar, | ||
| assistantIdentity, | ||
| props.basePath, | ||
| ); | ||
| } | ||
| if (item.kind === "group") { | ||
| if (deleted.has(item.key)) { | ||
| return nothing; | ||
| } | ||
| return renderMessageGroup(item, { | ||
| onOpenSidebar: props.onOpenSidebar, | ||
| showReasoning, | ||
| showToolCalls: props.showToolCalls, | ||
| assistantName: props.assistantName, | ||
| assistantAvatar: assistantIdentity.avatar, | ||
| basePath: props.basePath, | ||
| contextWindow: | ||
| activeSession?.contextTokens ?? props.sessions?.defaults?.contextTokens ?? null, | ||
| onDelete: () => { | ||
| deleted.delete(item.key); | ||
| requestUpdate(); | ||
| }, | ||
| }); | ||
| } | ||
| return nothing; | ||
| }, | ||
| }); | ||
| } | ||
| return nothing; | ||
| }, | ||
| )} | ||
| </div> | ||
| </div> | ||
| `; | ||
| )} | ||
| </div> | ||
| </div> | ||
| `; | ||
| }, |
Contributor
There was a problem hiding this comment.
props.onChatScroll (line 925), props.onOpenSidebar (lines 989, 999), and the requestUpdate closure derived from props.onRequestUpdate (line 1009) are captured inside the guard factory but are not listed in the guard's dependency keys. Lit's guard() returns noChange — skipping all DOM diffing — when the keys are identical, which means updated handler references will be silently ignored until some other key changes.
In practice this is low-risk if the calling component passes stable method references, but if renderChat is ever called with freshly-created arrow functions for these props (common in hook-based patterns), the bound handlers will become stale. Adding them to the guard key array keeps the optimisation intact while removing the latent footgun:
Suggested change
| <div | |
| class="chat-thread" | |
| role="log" | |
| aria-live="polite" | |
| @scroll=${props.onChatScroll} | |
| @click=${handleCodeBlockCopy} | |
| > | |
| <div class="chat-thread-inner"> | |
| ${ | |
| props.loading | |
| ? html` | |
| <div class="chat-loading-skeleton" aria-label="Loading chat"> | |
| <div class="chat-line assistant"> | |
| <div class="chat-msg"> | |
| <div class="chat-bubble"> | |
| <div class="skeleton skeleton-line skeleton-line--long" style="margin-bottom: 8px"></div> | |
| <div class="skeleton skeleton-line skeleton-line--medium" style="margin-bottom: 8px"></div> | |
| <div class="skeleton skeleton-line skeleton-line--short"></div> | |
| </div> | |
| </div> | |
| </div> | |
| <div class="chat-line user" style="margin-top: 12px"> | |
| <div class="chat-msg"> | |
| <div class="chat-bubble"> | |
| <div class="skeleton skeleton-line skeleton-line--medium"></div> | |
| </div> | |
| </div> | |
| </div> | |
| <div class="chat-line assistant" style="margin-top: 12px"> | |
| <div class="chat-msg"> | |
| <div class="chat-bubble"> | |
| <div class="skeleton skeleton-line skeleton-line--long" style="margin-bottom: 8px"></div> | |
| <div class="skeleton skeleton-line skeleton-line--short"></div> | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| ` | |
| : nothing | |
| } | |
| ${isEmpty && !vs.searchOpen ? renderWelcomeState(props) : nothing} | |
| ${ | |
| isEmpty && vs.searchOpen | |
| ? html` | |
| <div class="agent-chat__empty">No matching messages</div> | |
| ` | |
| : nothing | |
| } | |
| ${repeat( | |
| chatItems, | |
| (item) => item.key, | |
| (item) => { | |
| if (item.kind === "divider") { | |
| return html` | |
| <div class="chat-divider" role="separator" data-ts=${String(item.timestamp)}> | |
| <span class="chat-divider__line"></span> | |
| <span class="chat-divider__label">${item.label}</span> | |
| <span class="chat-divider__line"></span> | |
| </div> | |
| `; | |
| } | |
| if (item.kind === "reading-indicator") { | |
| return renderReadingIndicatorGroup(assistantIdentity, props.basePath); | |
| } | |
| if (item.kind === "stream") { | |
| return renderStreamingGroup( | |
| item.text, | |
| item.startedAt, | |
| props.onOpenSidebar, | |
| assistantIdentity, | |
| props.basePath, | |
| ); | |
| } | |
| if (item.kind === "group") { | |
| if (deleted.has(item.key)) { | |
| return nothing; | |
| ` | |
| : nothing | |
| } | |
| return renderMessageGroup(item, { | |
| onOpenSidebar: props.onOpenSidebar, | |
| showReasoning, | |
| showToolCalls: props.showToolCalls, | |
| assistantName: props.assistantName, | |
| assistantAvatar: assistantIdentity.avatar, | |
| basePath: props.basePath, | |
| contextWindow: | |
| activeSession?.contextTokens ?? props.sessions?.defaults?.contextTokens ?? null, | |
| onDelete: () => { | |
| deleted.delete(item.key); | |
| requestUpdate(); | |
| ${isEmpty && !vs.searchOpen ? renderWelcomeState(props) : nothing} | |
| ${ | |
| isEmpty && vs.searchOpen | |
| ? html` | |
| <div class="agent-chat__empty">No matching messages</div> | |
| ` | |
| : nothing | |
| } | |
| ${repeat( | |
| chatItems, | |
| (item) => item.key, | |
| (item) => { | |
| if (item.kind === "divider") { | |
| return html` | |
| <div class="chat-divider" role="separator" data-ts=${String(item.timestamp)}> | |
| <span class="chat-divider__line"></span> | |
| <span class="chat-divider__label">${item.label}</span> | |
| <span class="chat-divider__line"></span> | |
| </div> | |
| `; | |
| } | |
| if (item.kind === "reading-indicator") { | |
| return renderReadingIndicatorGroup(assistantIdentity, props.basePath); | |
| } | |
| if (item.kind === "stream") { | |
| return renderStreamingGroup( | |
| item.text, | |
| item.startedAt, | |
| props.onOpenSidebar, | |
| assistantIdentity, | |
| props.basePath, | |
| ); | |
| } | |
| if (item.kind === "group") { | |
| if (deleted.has(item.key)) { | |
| return nothing; | |
| } | |
| return renderMessageGroup(item, { | |
| onOpenSidebar: props.onOpenSidebar, | |
| showReasoning, | |
| showToolCalls: props.showToolCalls, | |
| assistantName: props.assistantName, | |
| assistantAvatar: assistantIdentity.avatar, | |
| basePath: props.basePath, | |
| contextWindow: | |
| activeSession?.contextTokens ?? props.sessions?.defaults?.contextTokens ?? null, | |
| onDelete: () => { | |
| deleted.delete(item.key); | |
| requestUpdate(); | |
| }, | |
| }); | |
| } | |
| return nothing; | |
| }, | |
| }); | |
| } | |
| return nothing; | |
| }, | |
| )} | |
| </div> | |
| </div> | |
| `; | |
| )} | |
| </div> | |
| </div> | |
| `; | |
| }, | |
| const thread = guard( | |
| [ | |
| props.loading, | |
| props.messages, | |
| props.toolMessages, | |
| props.streamSegments, | |
| props.stream, | |
| props.streamStartedAt, | |
| props.showToolCalls, | |
| showReasoning, | |
| props.sessionKey, | |
| props.assistantName, | |
| assistantIdentity.avatar, | |
| props.basePath, | |
| activeSession?.contextTokens ?? null, | |
| props.sessions?.defaults?.contextTokens ?? null, | |
| vs.searchOpen, | |
| vs.searchQuery, | |
| deleted.version, | |
| props.onChatScroll, | |
| props.onOpenSidebar, | |
| props.onRequestUpdate, | |
| ], |
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/views/chat.ts
Line: 921-1019
Comment:
**Callback props not included in guard keys**
`props.onChatScroll` (line 925), `props.onOpenSidebar` (lines 989, 999), and the `requestUpdate` closure derived from `props.onRequestUpdate` (line 1009) are captured inside the guard factory but are **not** listed in the guard's dependency keys. Lit's `guard()` returns `noChange` — skipping all DOM diffing — when the keys are identical, which means updated handler references will be silently ignored until some other key changes.
In practice this is low-risk if the calling component passes stable method references, but if `renderChat` is ever called with freshly-created arrow functions for these props (common in hook-based patterns), the bound handlers will become stale. Adding them to the guard key array keeps the optimisation intact while removing the latent footgun:
```suggestion
const thread = guard(
[
props.loading,
props.messages,
props.toolMessages,
props.streamSegments,
props.stream,
props.streamStartedAt,
props.showToolCalls,
showReasoning,
props.sessionKey,
props.assistantName,
assistantIdentity.avatar,
props.basePath,
activeSession?.contextTokens ?? null,
props.sessions?.defaults?.contextTokens ?? null,
vs.searchOpen,
vs.searchQuery,
deleted.version,
props.onChatScroll,
props.onOpenSidebar,
props.onRequestUpdate,
],
```
How can I resolve this? If you propose a fix, please make it concise.
Author
|
Closing per author request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #54874
Summary
guard()so draft-only updates do not rebuild the full message treeTesting