Highlight a clicked user message even with the log panes closed - #486
Merged
Conversation
Clicking a user message bailed out entirely when the persistent log was not open, so the click produced no feedback at all. The highlight now always lands - it marks the message being inspected - while pointing the panes at its entry stays conditional on them being open. Also stops the panes' settling pin from overriding that jump: a scroll aimed at a specific entry now cancels the pin, which otherwise dragged the pane back to the tail moments after the click. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR Summary by QodoHighlight selected messages when persistent logs are closed
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Cancel event lost during load
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1693
to
+1695
| // Tell a freshly opened log pane to stop pinning itself to the tail, or it | ||
| // would pull straight back down from the entry we are about to show. | ||
| window.dispatchEvent(new CustomEvent('persist-log:cancel-pin')); |
There was a problem hiding this comment.
1. Cancel event lost during load 🐞 Bug ≡ Correctness
autoScrollToTargetLog dispatches cancellation synchronously, but the persist-log listener is installed only after both asynchronous log requests and a DOM tick complete. A click during that interval can scroll an already-rendered target, then the later settling pin starts and drags the pane back to the tail.
Agent Prompt
## Issue description
The cancel-pin event can fire before the persist-log pane finishes asynchronous initialization and registers its listener. The later settling pin then starts despite the cancellation and can override the message-targeted scroll.
## Issue Context
PersistLog waits for both log requests and `tick()` before calling `pinToBottomWhileSettling()`, which currently owns listener registration. Register cancellation synchronously during component mounting and retain a cancellation flag so delayed initialization does not start the settling pin after an earlier event.
## Fix Focus Areas
- src/routes/chat/[agentId]/[conversationId]/chat-box.svelte[1692-1695]
- src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[86-101]
- src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[182-192]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+190
to
+192
| const cancelPin = () => stops.forEach(stop => stop()); | ||
| window.addEventListener(CANCEL_PIN_EVENT, cancelPin); | ||
| stops.push(() => window.removeEventListener(CANCEL_PIN_EVENT, cancelPin)); |
There was a problem hiding this comment.
2. Cancel listener leaks after pin 🐞 Bug ☼ Reliability
pinToBottomWhileSettling registers a window listener whose removal callback runs only when a future cancel event iterates stops; normal timeouts, user interaction, and component teardown never remove it. Repeatedly opening and closing logs therefore retains stale component and scrollbar closures indefinitely.
Agent Prompt
## Issue description
The global `persist-log:cancel-pin` listener remains registered after the settling period ends and after PersistLog is destroyed. Each pane mount can therefore leak another listener and its captured DOM and scrollbar state.
## Issue Context
Local `stop` callbacks disconnect observers and clear timers, but they do not remove the window listener. Ensure listener removal occurs when all pins stop, when cancellation occurs, and during component teardown, including teardown before asynchronous initialization completes.
## Fix Focus Areas
- src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[86-101]
- src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[182-192]
- src/routes/chat/[agentId]/[conversationId]/persist-log/persist-log.svelte[205-218]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Clicking a user message bailed out entirely when the persistent log was not open, so the click produced no feedback at all. The highlight now always lands - it marks the message being inspected - while pointing the panes at its entry stays conditional on them being open.
Also stops the panes' settling pin from overriding that jump: a scroll aimed at a specific entry now cancels the pin, which otherwise dragged the pane back to the tail moments after the click.