Skip to content

fix: refresh scroller layout when tool confirmation dialog is cancelled#254

Merged
jdneo merged 2 commits into
microsoft:mainfrom
rsd-darshan:fix/subagent-panel-size-cancel-dialog-2
May 26, 2026
Merged

fix: refresh scroller layout when tool confirmation dialog is cancelled#254
jdneo merged 2 commits into
microsoft:mainfrom
rsd-darshan:fix/subagent-panel-size-cancel-dialog-2

Conversation

@rsd-darshan

@rsd-darshan rsd-darshan commented May 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • When the user cancels a subagent tool confirmation dialog, the subagent panel stays expanded at its pre-cancel height
  • Root cause: cancelConfirmation() calls parent.layout() (relays the turn widget's children) but never updates the ScrolledComposite's minimum size via refreshScrollerLayout()
  • On the Continue path this goes unnoticed because subsequent subagent messages trigger a layout refresh; on the Cancel path no further messages arrive, so the panel is stuck

Fix

Pass a Runnable callback from BaseTurnWidget into InvokeToolConfirmationDialog at construction. BaseTurnWidget walks up the ancestor chain once to find ChatContentViewer and captures refreshScrollerLayout() as the callback. cancelConfirmation() calls it after disposal, which recomputes setMinHeight() in a single layout pass and collapses the panel correctly. This also removes the redundant parent.layout() call that preceded the deep layout(true,true) inside refreshScrollerLayout().

Test plan

  • Invoke a subagent (e.g. "run ls /Users/darshanpoudel/Desktop in the terminal")
  • Wait for the tool confirmation dialog to appear
  • Click Cancel
  • Verify the subagent panel resizes/collapses correctly and does not stay stuck at the expanded height
  • Click Continue on a separate invocation and verify that path is unaffected

Fixes #169

@rsd-darshan rsd-darshan requested a review from jdneo as a code owner May 23, 2026 14:05
Copilot AI review requested due to automatic review settings May 23, 2026 14:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates the cancel-confirmation UI flow so the chat panel properly collapses after a tool invocation is canceled, even when no further messages arrive to trigger another layout pass.

Changes:

  • After cancellation, traverses ancestor composites to find the ChatContentViewer.
  • Calls ChatContentViewer.refreshScrollerLayout() to recompute the scroller’s minimum size so the panel can shrink.

@jdneo

jdneo commented May 25, 2026

Copy link
Copy Markdown
Member

Thanks for tracking down the root cause. I agree that the missing piece is recomputing the ScrolledComposite min height after the cancel path, since no later message arrives to trigger refreshScrollerLayout().

I would prefer not to inject a layout callback into InvokeToolConfirmationDialog, though. That makes a leaf confirmation widget aware of the chat container's layout policy. Could we split the responsibilities instead?

  • Keep InvokeToolConfirmationDialog self-contained: both accept and cancel paths can use a private helper like disposeAndRequestParentLayout(), which disposes the dialog and calls parent.requestLayout() on its immediate parent.
  • Let ChatContentViewer own a coalesced async API such as requestRefreshScrollerLayout(), which schedules a single refreshScrollerLayout() after pending SWT mutations settle.
  • In BaseTurnWidget.requestToolExecutionConfirmation(...), where the dialog is created, add a dispose listener on the dialog that walks to the nearest ChatContentViewer and calls requestRefreshScrollerLayout().

That should fix the stuck subagent panel for cancel, accept, and programmatic cancellation uniformly, while keeping scroller-specific behavior out of InvokeToolConfirmationDialog.

When the user cancels a subagent tool confirmation dialog, the subagent
panel stays expanded at its pre-cancel height. This happens because
cancelConfirmation() only called parent.layout() (relaying the turn
widget's children), but never updated the ScrolledComposite's minimum
size via refreshScrollerLayout(). On the Continue path this goes
unnoticed because subsequent subagent messages trigger a layout refresh;
on the Cancel path no further messages arrive, so the panel is stuck.

Fix by passing a Runnable callback from BaseTurnWidget into the dialog at
construction time. BaseTurnWidget walks up the ancestor chain once to
find ChatContentViewer and captures refreshScrollerLayout() as the
callback. cancelConfirmation() calls it after disposal, which recomputes
setMinHeight() in a single layout pass and collapses the panel correctly.
This also removes the redundant parent.layout() call that preceded the
deep layout(true,true) inside refreshScrollerLayout().

Fixes microsoft#169
@rsd-darshan rsd-darshan force-pushed the fix/subagent-panel-size-cancel-dialog-2 branch from 40f0998 to 82643d2 Compare May 25, 2026 11:17
When the user cancels a subagent tool confirmation dialog, the subagent
panel stays expanded at its pre-cancel height. This happens because
cancelConfirmation() only called parent.layout() (relaying the turn
widget's children), but never updated the ScrolledComposite's minimum
size via refreshScrollerLayout(). On the Continue path this goes
unnoticed because subsequent subagent messages trigger a layout refresh;
on the Cancel path no further messages arrive, so the panel is stuck.

Fix by adding a dispose listener on the dialog in BaseTurnWidget that
walks up to the nearest ChatContentViewer and calls
requestRefreshScrollerLayout(), a new coalesced async helper that
schedules a single refreshScrollerLayout() after pending SWT mutations
settle. InvokeToolConfirmationDialog remains self-contained: both the
accept and cancel paths use a private disposeAndRequestParentLayout()
helper, keeping scroller-specific behaviour out of the dialog.

Fixes microsoft#169
@rsd-darshan

rsd-darshan commented May 25, 2026

Copy link
Copy Markdown
Contributor Author

@jdneo Thanks for the detailed feedback — that's a much cleaner split of responsibilities.

I've updated the implementation:

  • InvokeToolConfirmationDialog is now self-contained: both accept and cancel paths use a private disposeAndRequestParentLayout() helper that disposes the dialog and calls parent.requestLayout() on its immediate parent. No layout policy leaks out of the dialog.
  • ChatContentViewer gains a requestRefreshScrollerLayout() method that schedules a single async refreshScrollerLayout() call, coalescing multiple dispose/layout events that arrive in the same event-loop tick.
  • BaseTurnWidget.requestToolExecutionConfirmation() attaches a dispose listener on the dialog that walks up to the nearest ChatContentViewer and calls requestRefreshScrollerLayout(). This covers cancel, accept, and programmatic cancellation uniformly.

@jdneo jdneo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!

@jdneo jdneo merged commit cddd5da into microsoft:main May 26, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Subagent panel size is not updated if cancel in the middle

3 participants