mirror of
https://github.com/zed-industries/zed.git
synced 2026-05-31 19:05:00 +07:00
Fix flicker of pushed-off sticky project header in threads sidebar (#57529)
Some checks are pending
Congratsbot / check-author (push) Waiting to run
Congratsbot / congrats (push) Blocked by required conditions
run_tests / orchestrate (push) Waiting to run
run_tests / check_style (push) Waiting to run
run_tests / clippy_windows (push) Blocked by required conditions
run_tests / clippy_linux (push) Blocked by required conditions
run_tests / clippy_mac (push) Blocked by required conditions
run_tests / clippy_mac_x86_64 (push) Blocked by required conditions
run_tests / run_tests_windows (push) Blocked by required conditions
run_tests / run_tests_linux (push) Blocked by required conditions
run_tests / run_tests_mac (push) Blocked by required conditions
run_tests / miri_scheduler (push) Blocked by required conditions
run_tests / doctests (push) Blocked by required conditions
run_tests / check_workspace_binaries (push) Blocked by required conditions
run_tests / build_visual_tests_binary (push) Blocked by required conditions
run_tests / check_wasm (push) Blocked by required conditions
run_tests / check_dependencies (push) Blocked by required conditions
run_tests / check_docs (push) Blocked by required conditions
run_tests / check_licenses (push) Blocked by required conditions
run_tests / check_scripts (push) Blocked by required conditions
run_tests / check_postgres_and_protobuf_migrations (push) Blocked by required conditions
run_tests / extension_tests (push) Blocked by required conditions
run_tests / tests_pass (push) Blocked by required conditions
deploy_nightly_docs / deploy_docs (push) Has been skipped
Some checks are pending
Congratsbot / check-author (push) Waiting to run
Congratsbot / congrats (push) Blocked by required conditions
run_tests / orchestrate (push) Waiting to run
run_tests / check_style (push) Waiting to run
run_tests / clippy_windows (push) Blocked by required conditions
run_tests / clippy_linux (push) Blocked by required conditions
run_tests / clippy_mac (push) Blocked by required conditions
run_tests / clippy_mac_x86_64 (push) Blocked by required conditions
run_tests / run_tests_windows (push) Blocked by required conditions
run_tests / run_tests_linux (push) Blocked by required conditions
run_tests / run_tests_mac (push) Blocked by required conditions
run_tests / miri_scheduler (push) Blocked by required conditions
run_tests / doctests (push) Blocked by required conditions
run_tests / check_workspace_binaries (push) Blocked by required conditions
run_tests / build_visual_tests_binary (push) Blocked by required conditions
run_tests / check_wasm (push) Blocked by required conditions
run_tests / check_dependencies (push) Blocked by required conditions
run_tests / check_docs (push) Blocked by required conditions
run_tests / check_licenses (push) Blocked by required conditions
run_tests / check_scripts (push) Blocked by required conditions
run_tests / check_postgres_and_protobuf_migrations (push) Blocked by required conditions
run_tests / extension_tests (push) Blocked by required conditions
run_tests / tests_pass (push) Blocked by required conditions
deploy_nightly_docs / deploy_docs (push) Has been skipped
The threads sidebar rebuilds its `Vec<ListEntry>` from scratch on events that touch thread/sidebar state (status changes, title generation, new live info, sending a message, etc.). It previously called `ListState::reset` after every rebuild, which rewrote every list item to `Unmeasured`. On the next render frame, the sticky project header had no measured bounds for the next project header. The sticky project header uses `ListState::bounds_for_item` for the next project header to compute how far it should be pushed off screen. When those measurements were missing, it temporarily fell back to `top_offset = 0`, snapped fully into view for one frame, then popped back once the list was remeasured. Fix: preserve list measurements for entries whose identity and layout shape did not change. `EntryShape` captures each entry's identity plus height-affecting project-header flags. `update_entries` snapshots the old shapes, rebuilds contents, then splices only the changed shape range into `ListState`. Unchanged items keep their measured bounds, so the sticky header remains in its pushed-off position across same-shape updates. This also adds a regression test that renders a two-project sidebar, scrolls into the sticky-header push-off state, performs a same-shape thread metadata update, and verifies the next header's measured bounds are preserved. Closes AI-196 Release Notes: - Fixed the project section header flickering in the agent threads sidebar when sending a message while the header was partially scrolled off screen.
This commit is contained in:
parent
eb944cfd7a
commit
a8966695ee
2 changed files with 218 additions and 3 deletions
|
|
@ -422,6 +422,23 @@ struct SidebarContents {
|
|||
has_open_projects: bool,
|
||||
}
|
||||
|
||||
/// Identity-and-layout key for a [`ListEntry`] used to preserve measured list items
|
||||
/// across rebuilds. Equal shapes must render to the same height; add any new
|
||||
/// height-affecting state here.
|
||||
#[derive(Debug, PartialEq, Eq)]
|
||||
enum EntryShape {
|
||||
ProjectHeader {
|
||||
key: ProjectGroupKey,
|
||||
// Toggles the "No threads yet" empty-state row when not collapsed.
|
||||
has_threads: bool,
|
||||
// Determines whether the "No threads yet" row is rendered (only shown when
|
||||
// `!is_collapsed && !has_threads`).
|
||||
is_collapsed: bool,
|
||||
},
|
||||
Thread(ThreadId),
|
||||
Terminal(TerminalId),
|
||||
}
|
||||
|
||||
impl SidebarContents {
|
||||
fn is_thread_notified(&self, thread_id: &agent_ui::ThreadId) -> bool {
|
||||
self.notified_threads.contains(thread_id)
|
||||
|
|
@ -1837,13 +1854,14 @@ impl Sidebar {
|
|||
}
|
||||
|
||||
let had_notifications = self.has_notifications(cx);
|
||||
let scroll_position = self.list_state.logical_scroll_top();
|
||||
let previous_shapes: Vec<EntryShape> =
|
||||
self.entry_shapes(multi_workspace.read(cx)).collect();
|
||||
|
||||
self.rebuild_contents(cx);
|
||||
self.refresh_draft_editor_observations(cx);
|
||||
|
||||
self.list_state.reset(self.contents.entries.len());
|
||||
self.list_state.scroll_to(scroll_position);
|
||||
// Preserve measurements for unchanged entries so sticky headers do not flicker.
|
||||
self.apply_list_state_diff(&previous_shapes, multi_workspace.read(cx));
|
||||
|
||||
if had_notifications != self.has_notifications(cx) {
|
||||
multi_workspace.update(cx, |_, cx| {
|
||||
|
|
@ -1854,6 +1872,56 @@ impl Sidebar {
|
|||
cx.notify();
|
||||
}
|
||||
|
||||
/// Splices only the changed entry range, leaving unchanged item measurements intact.
|
||||
fn apply_list_state_diff(
|
||||
&self,
|
||||
previous_shapes: &[EntryShape],
|
||||
multi_workspace: &MultiWorkspace,
|
||||
) {
|
||||
let mut new_iter = self.entry_shapes(multi_workspace);
|
||||
let mut prefix_len = 0;
|
||||
let leading_new = loop {
|
||||
match (previous_shapes.get(prefix_len), new_iter.next()) {
|
||||
(Some(prev), Some(next)) if *prev == next => prefix_len += 1,
|
||||
(None, None) => return,
|
||||
(_, leading) => break leading,
|
||||
}
|
||||
};
|
||||
|
||||
let new_tail: Vec<EntryShape> = leading_new.into_iter().chain(new_iter).collect();
|
||||
let prev_tail = &previous_shapes[prefix_len..];
|
||||
let suffix_len = prev_tail
|
||||
.iter()
|
||||
.rev()
|
||||
.zip(new_tail.iter().rev())
|
||||
.take_while(|(prev, next)| prev == next)
|
||||
.count();
|
||||
|
||||
let old_changed = prefix_len..previous_shapes.len() - suffix_len;
|
||||
let new_changed_count = new_tail.len() - suffix_len;
|
||||
self.list_state.splice(old_changed, new_changed_count);
|
||||
}
|
||||
|
||||
fn entry_shapes<'a>(
|
||||
&'a self,
|
||||
multi_workspace: &'a MultiWorkspace,
|
||||
) -> impl Iterator<Item = EntryShape> + 'a {
|
||||
self.contents.entries.iter().map(move |entry| match entry {
|
||||
ListEntry::ProjectHeader {
|
||||
key, has_threads, ..
|
||||
} => EntryShape::ProjectHeader {
|
||||
key: key.clone(),
|
||||
has_threads: *has_threads,
|
||||
is_collapsed: multi_workspace
|
||||
.group_state_by_key(key)
|
||||
.map(|state| !state.expanded)
|
||||
.unwrap_or(false),
|
||||
},
|
||||
ListEntry::Thread(thread) => EntryShape::Thread(thread.metadata.thread_id),
|
||||
ListEntry::Terminal(terminal) => EntryShape::Terminal(terminal.metadata.terminal_id),
|
||||
})
|
||||
}
|
||||
|
||||
/// Re-establishes subscriptions to each visible draft's message editor
|
||||
/// so we rebuild entries (and their displayed titles) as the user types.
|
||||
fn refresh_draft_editor_observations(&mut self, cx: &mut Context<Self>) {
|
||||
|
|
|
|||
|
|
@ -605,6 +605,153 @@ fn visible_entries_as_strings(
|
|||
})
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_thread_metadata_update_preserves_sticky_header_measurements(cx: &mut TestAppContext) {
|
||||
let (fs, project_a) = init_multi_project_test(&["/project-a", "/project-b"], cx).await;
|
||||
let (multi_workspace, cx) =
|
||||
cx.add_window_view(|window, cx| MultiWorkspace::test_new(project_a.clone(), window, cx));
|
||||
let sidebar = setup_sidebar(&multi_workspace, cx);
|
||||
add_test_project("/project-b", &fs, &multi_workspace, cx).await;
|
||||
|
||||
save_thread_metadata(
|
||||
acp::SessionId::new(Arc::from("project-a-thread")),
|
||||
Some("Project A Thread".into()),
|
||||
chrono::TimeZone::with_ymd_and_hms(&Utc, 2024, 1, 1, 0, 0, 0).unwrap(),
|
||||
None,
|
||||
None,
|
||||
&project_a,
|
||||
cx,
|
||||
);
|
||||
save_thread_metadata_with_main_paths(
|
||||
"project-b-thread",
|
||||
"Project B Thread",
|
||||
PathList::new(&[PathBuf::from("/project-b")]),
|
||||
PathList::new(&[PathBuf::from("/project-b")]),
|
||||
chrono::TimeZone::with_ymd_and_hms(&Utc, 2024, 1, 2, 0, 0, 0).unwrap(),
|
||||
cx,
|
||||
);
|
||||
|
||||
cx.draw(
|
||||
gpui::point(px(0.), px(0.)),
|
||||
gpui::size(px(400.), px(240.)),
|
||||
|_, _| sidebar.clone().into_any_element(),
|
||||
);
|
||||
cx.run_until_parked();
|
||||
|
||||
let next_header_ix = sidebar.read_with(cx, |sidebar, _| {
|
||||
assert!(
|
||||
sidebar.contents.project_header_indices.len() == 2,
|
||||
"test setup should render exctly two project headers"
|
||||
);
|
||||
sidebar.contents.project_header_indices[1]
|
||||
});
|
||||
|
||||
sidebar.update_in(cx, |sidebar, _window, cx| {
|
||||
sidebar.list_state.scroll_to(gpui::ListOffset {
|
||||
item_ix: next_header_ix - 1,
|
||||
offset_in_item: px(24.),
|
||||
});
|
||||
cx.notify();
|
||||
});
|
||||
cx.draw(
|
||||
gpui::point(px(0.), px(0.)),
|
||||
gpui::size(px(400.), px(240.)),
|
||||
|_, _| sidebar.clone().into_any_element(),
|
||||
);
|
||||
cx.run_until_parked();
|
||||
|
||||
let bounds_before = sidebar.read_with(cx, |sidebar, _| {
|
||||
sidebar
|
||||
.list_state
|
||||
.bounds_for_item(next_header_ix)
|
||||
.expect("next project header should be measured before metadata update")
|
||||
});
|
||||
|
||||
save_thread_metadata(
|
||||
acp::SessionId::new(Arc::from("project-a-thread")),
|
||||
Some("Renamed Project A Thread".into()),
|
||||
chrono::TimeZone::with_ymd_and_hms(&Utc, 2024, 1, 1, 0, 1, 0).unwrap(),
|
||||
None,
|
||||
None,
|
||||
&project_a,
|
||||
cx,
|
||||
);
|
||||
|
||||
let bounds_after = sidebar.read_with(cx, |sidebar, _| {
|
||||
sidebar
|
||||
.list_state
|
||||
.bounds_for_item(next_header_ix)
|
||||
.expect("same-shape metadata update should preserve next header measurements")
|
||||
});
|
||||
assert_eq!(bounds_before, bounds_after);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_thread_status_update_does_not_reset_list_measurements(cx: &mut TestAppContext) {
|
||||
// When a thread's status changes (e.g. Running -> Completed after sending a message), the
|
||||
// shape sequence is unchanged, so `update_entries` should not reset the underlying
|
||||
// `ListState`. Resetting throws away measured item bounds for one frame, which makes the
|
||||
// sticky project header flicker between its pushed-off and fully-on-screen positions.
|
||||
let project = init_test_project("/my-project", cx).await;
|
||||
let (multi_workspace, cx) =
|
||||
cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx));
|
||||
let sidebar = setup_sidebar(&multi_workspace, cx);
|
||||
|
||||
save_n_test_threads(2, &project, cx).await;
|
||||
cx.run_until_parked();
|
||||
|
||||
let before = sidebar.read_with(cx, |sidebar, app| {
|
||||
sidebar
|
||||
.entry_shapes(multi_workspace.read(app))
|
||||
.collect::<Vec<_>>()
|
||||
});
|
||||
sidebar.update_in(cx, |sidebar, _window, cx| sidebar.update_entries(cx));
|
||||
cx.run_until_parked();
|
||||
let after = sidebar.read_with(cx, |sidebar, app| {
|
||||
sidebar
|
||||
.entry_shapes(multi_workspace.read(app))
|
||||
.collect::<Vec<_>>()
|
||||
});
|
||||
|
||||
assert_eq!(
|
||||
before, after,
|
||||
"a no-op rebuild should produce an identical shape sequence"
|
||||
);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_collapse_changes_entry_shape(cx: &mut TestAppContext) {
|
||||
let project = init_test_project("/my-project", cx).await;
|
||||
let (multi_workspace, cx) =
|
||||
cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx));
|
||||
let sidebar = setup_sidebar(&multi_workspace, cx);
|
||||
|
||||
save_n_test_threads(2, &project, cx).await;
|
||||
cx.run_until_parked();
|
||||
|
||||
let project_group_key = project.read_with(cx, |project, cx| project.project_group_key(cx));
|
||||
|
||||
let before = sidebar.read_with(cx, |sidebar, app| {
|
||||
sidebar
|
||||
.entry_shapes(multi_workspace.read(app))
|
||||
.collect::<Vec<_>>()
|
||||
});
|
||||
sidebar.update_in(cx, |sidebar, window, cx| {
|
||||
sidebar.toggle_collapse(&project_group_key, window, cx);
|
||||
});
|
||||
cx.run_until_parked();
|
||||
let after = sidebar.read_with(cx, |sidebar, app| {
|
||||
sidebar
|
||||
.entry_shapes(multi_workspace.read(app))
|
||||
.collect::<Vec<_>>()
|
||||
});
|
||||
|
||||
assert_ne!(
|
||||
before, after,
|
||||
"collapsing the project group should change the shape sequence so the list resets"
|
||||
);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_serialization_round_trip(cx: &mut TestAppContext) {
|
||||
let project = init_test_project("/my-project", cx).await;
|
||||
|
|
|
|||
Loading…
Reference in a new issue