In Agent mode: Auto scroll to prompts like 'Continue'.#26
In Agent mode: Auto scroll to prompts like 'Continue'.#26raghucssit wants to merge 1 commit intomicrosoft:mainfrom
Conversation
User must be made aware some action is needed from them to continue the work by agent. Usually this happens when "Too many requests" or "Command line run prompt" etc. see https://github.com/microsoft/copilot-eclipse-feedback/issues/184
raghucssit
requested review from
duzitong,
ethanyhou,
jdneo and
xinyi-gong
as code owners
There was a problem hiding this comment.
Pull request overview
This PR improves the Agent-mode chat UX by ensuring the chat view scrolls to the bottom when interactive prompts (e.g., “Continue”, tool confirmations, command-line prompts) appear, so users notice they need to take action.
Changes:
- Force-scroll
ChatContentViewerto bottom for Agent-mode (agent rounds) updates, regardless of prior manual scroll state. - Expose
scrollToBottom()publicly and introduceforceScrollToBottom()with delayed follow-up scrolling to handle layout timing. - Trigger scrolling when tool confirmation dialogs are created/closed so newly revealed controls are visible.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/InvokeToolConfirmationDialog.java | Scrolls the chat viewer after Continue/Cancel actions so follow-up UI is visible. |
| com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ChatContentViewer.java | Adds forced scrolling behavior for agent rounds; makes scrolling APIs available to child widgets. |
| com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/BaseTurnWidget.java | Scrolls the chat viewer to reveal newly created tool confirmation dialogs (prefer showControl). |
| * Force the view to scroll to the bottom regardless of the user's manual scroll state. This is used for important UI | ||
| * prompts (like tool confirmations) to ensure they are visible even if the user had scrolled away. The implementation | ||
| * performs a two-phase scroll to be robust against layout timing issues. |
There was a problem hiding this comment.
The Javadoc for forceScrollToBottom() is written as very long single lines and is likely to violate the repo's Checkstyle LineLength (max 120). Please wrap the Javadoc text onto multiple lines to satisfy the 120-char limit.
| * Force the view to scroll to the bottom regardless of the user's manual scroll state. This is used for important UI | |
| * prompts (like tool confirmations) to ensure they are visible even if the user had scrolled away. The implementation | |
| * performs a two-phase scroll to be robust against layout timing issues. | |
| * Force the view to scroll to the bottom regardless of the user's manual | |
| * scroll state. This is used for important UI prompts (like tool | |
| * confirmations) to ensure they are visible even if the user had scrolled | |
| * away. The implementation performs a two-phase scroll to be robust against | |
| * layout timing issues. |
| // Ensure the chat content viewer scrolls to show the newly created confirmation | ||
| // dialog/footer area. Walk up the composite hierarchy to find a ChatContentViewer | ||
| // and request scrolling. Use async exec because layout needs to complete first. | ||
| SwtUtils.invokeOnDisplayThreadAsync(() -> { | ||
| Control parent = this.getParent(); | ||
| while (parent != null && !(parent instanceof ChatContentViewer)) { | ||
| parent = parent.getParent(); | ||
| } |
There was a problem hiding this comment.
There are now multiple copies of the same "walk up the parent chain to find ChatContentViewer" logic (here and in InvokeToolConfirmationDialog.scrollToCancel). To reduce duplication and make future scrolling changes safer, consider extracting a shared helper (e.g., SwtUtils.findParentOfType(Control, Class) or a ChatContentViewer.findAncestor(Control) utility).
| // For agent-mode responses (agent rounds/tool calls) we always force the view | ||
| // to scroll to the bottom so prompts that require user action (e.g. Continue, | ||
| // permission dialogs) are visible even if the user previously scrolled away. | ||
| if (value.getAgentRounds() != null && !value.getAgentRounds().isEmpty()) { | ||
| // Use a forced scroll to ensure visibility regardless of manual scroll state. | ||
| forceScrollToBottom(); | ||
| } else { |
There was a problem hiding this comment.
processTurnEvent calls forceScrollToBottom() for every agent-round progress event. Since forceScrollToBottom() schedules delayed timerExec callbacks, this can enqueue a large number of UI-thread tasks during streaming/rapid progress updates and cause jank. Consider debouncing/coalescing forced scroll requests (e.g., keep one pending scheduled scroll per viewer/turn) or only forcing scroll when an actual user-action prompt is rendered.
There was a problem hiding this comment.
| // Temporarily enable auto-scroll to allow setOrigin to take effect. | ||
| boolean previousAuto = this.autoScrollEnabled; | ||
| this.autoScrollEnabled = true; | ||
| // Run layout on content to ensure sizes are accurate before measuring. | ||
| cmpContent.layout(true, true); |
There was a problem hiding this comment.
forceScrollToBottom() temporarily overwrites autoScrollEnabled and restores it later via a delayed timer. This can clobber the user's scroll state if they manually scroll during the delay, and it can also restore the wrong value if forceScrollToBottom() is invoked again before the 350ms timer runs. Consider avoiding mutation of autoScrollEnabled here, or restoring conditionally (e.g., only if it wasn't changed by user input) / using a request counter so only the latest invocation restores state.
| try { | ||
| if (!this.isDisposed()) { | ||
| doScrollToBottom(); | ||
| } | ||
| } catch (Exception ignore) { | ||
| // ignore | ||
| } finally { | ||
| // Restore previous state after the delayed scrolls |
There was a problem hiding this comment.
forceScrollToBottom() schedules delayed timerExec callbacks but the 350ms callback catches a generic Exception and ignores it. This makes real SWT issues harder to diagnose and can mask failures. Since doScrollToBottom() already guards disposal, consider removing the catch entirely or at least logging unexpected exceptions (and also ensure restoration of autoScrollEnabled happens even if the timer callback is never reached).
| try { | |
| if (!this.isDisposed()) { | |
| doScrollToBottom(); | |
| } | |
| } catch (Exception ignore) { | |
| // ignore | |
| } finally { | |
| // Restore previous state after the delayed scrolls | |
| if (!this.isDisposed()) { | |
| doScrollToBottom(); | |
| } | |
| }); | |
| SwtUtils.getDisplay().timerExec(350, () -> { | |
| if (!this.isDisposed()) { | |
| // Restore previous state after the delayed scrolls. |
User must be made aware some action is needed from them to continue the work by agent. Usually this happens when "Too many requests" or "Command line run prompt" etc.
see #120