agent_ui: Remeasure changed entries in the thread view list (#53017)

Danilo Leal and Eric Holk created 1 week ago

This PR fixes an issue where, sometimes, you couldn't scroll all the way
to the bottom of the thread's content. The scrollbar would show up at
the bottom of the scrollable container but the content was visibly cut
off. Turns out that's a consequence of the top-down thread generation
introduced in https://github.com/zed-industries/zed/pull/52440, where
changing the list alignment to `Top` made it visible that sometimes, the
maximum scroll area would get underestimated because the items in the
thread view's list would have a stale height measurement. So, the way
this PR fixes the issue is by calling `splice_focusable` in the
`EntryUpdated` event, too, so that the height of the items in the
overdraw area get marked as unmeasured, triggering a list re-render and
re-measuring.

We started by writing a test at the list level that would reproduce the
regression but then later figured out that this is not an inherent list
problem; it was rather a problem with its use within the thread view
layer. Then, we explored writing a test that documented the regression,
but it turned out to be very hard to simulate this sort of set up in
which certain elements would have its height changed during streaming,
which would be how you'd get to a mismatched height situation.

Therefore, given `AcpThreadEvent::NewEntry` already called
`splice_focusable` and don't have a test for it, we figure it'd be safe
to move forward without one, too. We then introduced a helper that's now
shared between `AcpThreadEvent::NewEntry` and
`AcpThreadEvent::EntryUpdated`.

Release Notes:

- Agent: Fixed an issue where sometimes you couldn't scroll all the way
to the bottom of the thread even though there's visibly more content
below the fold.

Co-authored-by: Eric Holk <[e-mail]>

Change summary

crates/agent_ui/src/conversation_view.rs | 57 ++++++++++++++++++++-----
1 file changed, 45 insertions(+), 12 deletions(-)

Detailed changes

crates/agent_ui/src/conversation_view.rs

@@ -1240,15 +1240,15 @@ impl ConversationView {
                 if let Some(active) = self.thread_view(&thread_id) {
                     let entry_view_state = active.read(cx).entry_view_state.clone();
                     let list_state = active.read(cx).list_state.clone();
-                    entry_view_state.update(cx, |view_state, cx| {
-                        view_state.sync_entry(index, thread, window, cx);
-                        list_state.splice_focusable(
-                            index..index,
-                            [view_state
-                                .entry(index)
-                                .and_then(|entry| entry.focus_handle(cx))],
-                        );
-                    });
+                    notify_entry_changed(
+                        &entry_view_state,
+                        &list_state,
+                        index..index,
+                        index,
+                        thread,
+                        window,
+                        cx,
+                    );
                     active.update(cx, |active, cx| {
                         active.sync_editor_mode_for_empty_state(cx);
                     });
@@ -1257,9 +1257,16 @@ impl ConversationView {
             AcpThreadEvent::EntryUpdated(index) => {
                 if let Some(active) = self.thread_view(&thread_id) {
                     let entry_view_state = active.read(cx).entry_view_state.clone();
-                    entry_view_state.update(cx, |view_state, cx| {
-                        view_state.sync_entry(*index, thread, window, cx)
-                    });
+                    let list_state = active.read(cx).list_state.clone();
+                    notify_entry_changed(
+                        &entry_view_state,
+                        &list_state,
+                        *index..*index + 1,
+                        *index,
+                        thread,
+                        window,
+                        cx,
+                    );
                     active.update(cx, |active, cx| {
                         active.auto_expand_streaming_thought(cx);
                     });
@@ -2598,6 +2605,32 @@ impl ConversationView {
     }
 }
 
+/// Syncs an entry's view state with the latest thread data and splices
+/// the list item so the list knows to re-measure it on the next paint.
+///
+/// Used by both `NewEntry` (splice range `index..index` to insert) and
+/// `EntryUpdated` (splice range `index..index+1` to replace), which is
+/// why the caller provides the splice range.
+fn notify_entry_changed(
+    entry_view_state: &Entity<EntryViewState>,
+    list_state: &ListState,
+    splice_range: std::ops::Range<usize>,
+    index: usize,
+    thread: &Entity<AcpThread>,
+    window: &mut Window,
+    cx: &mut App,
+) {
+    entry_view_state.update(cx, |view_state, cx| {
+        view_state.sync_entry(index, thread, window, cx);
+        list_state.splice_focusable(
+            splice_range,
+            [view_state
+                .entry(index)
+                .and_then(|entry| entry.focus_handle(cx))],
+        );
+    });
+}
+
 fn loading_contents_spinner(size: IconSize) -> AnyElement {
     Icon::new(IconName::LoadCircle)
         .size(size)