Fix editor hang when positioned above viewport (#45077)
Fixes the hang introduced in #44995 (which was reverted in #45011) and re-enables the optimization. ## Background PR #44995 introduced an optimization to skip rendering lines that are clipped by parent containers (e.g., when a large AutoHeight editor is inside a scrollable List). This significantly improved performance for large diffs in the Agent Panel. However, #45011 reverted this change because it caused the main thread to hang for 100+ seconds in certain scenarios, requiring a force quit to recover. ## Root Cause The original analysis in #45011 suggested that visible_bounds wasn’t being intersected properly, but that was incorrect—the intersection via with_content_mask works correctly. The actual bug: when an editor is positioned above the visible viewport (e.g., scrolled past in a List), the clipping calculation produces a start_row that exceeds max_row: 1. Editor’s bounds.origin.y becomes very negative (e.g., -10000px) 2. After intersection, visible_bounds.origin.y is at the viewport top (e.g., 0) 3. clipped_top_in_lines = (0 - (-10000)) / line_height = huge number 4. start_row = huge number, but end_row is clamped to max_row 5. This creates an invalid range where start_row > end_row This caused two different failures depending on build mode: - Debug mode: Panic from subtraction overflow in Range<DisplayRow>::len() - Release mode: Integer wraparound causing blocks_in_range to enter an infinite loop (the 100+ second hang) ## Fix Simply clamp start_row to max_row, ensuring the row range is always valid: ```rs let start_row = cmp::min( DisplayRow((scroll_position.y + clipped_top_in_lines).floor() as u32), max_row, ); ``` ## Testing Added a regression test that draws an editor at y=-10000 to simulate an editor that’s been scrolled past in a List. This would panic in debug mode (and hang in release mode) before the fix. Release Notes: - Improved agent panel performance when rendering large diffs.
This commit is contained in:
committed by
GitHub
parent
c5b3b06b94
commit
637ff34254
@@ -29530,3 +29530,38 @@ async fn test_local_worktree_trust(cx: &mut TestAppContext) {
|
||||
trusted_worktrees.update(cx, |store, cx| store.can_trust(worktree_id, cx));
|
||||
assert!(can_trust_after, "worktree should be trusted after trust()");
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
fn test_editor_rendering_when_positioned_above_viewport(cx: &mut TestAppContext) {
|
||||
// This test reproduces a bug where drawing an editor at a position above the viewport
|
||||
// (simulating what happens when an AutoHeight editor inside a List is scrolled past)
|
||||
// causes an infinite loop in blocks_in_range.
|
||||
//
|
||||
// The issue: when the editor's bounds.origin.y is very negative (above the viewport),
|
||||
// the content mask intersection produces visible_bounds with origin at the viewport top.
|
||||
// This makes clipped_top_in_lines very large, causing start_row to exceed max_row.
|
||||
// When blocks_in_range is called with start_row > max_row, the cursor seeks to the end
|
||||
// but the while loop after seek never terminates because cursor.next() is a no-op at end.
|
||||
init_test(cx, |_| {});
|
||||
|
||||
let window = cx.add_window(|_, _| gpui::Empty);
|
||||
let mut cx = VisualTestContext::from_window(*window, cx);
|
||||
|
||||
let buffer = cx.update(|_, cx| MultiBuffer::build_simple("a\nb\nc\nd\ne\nf\ng\nh\ni\nj\n", cx));
|
||||
let editor = cx.new_window_entity(|window, cx| build_editor(buffer, window, cx));
|
||||
|
||||
// Simulate a small viewport (500x500 pixels at origin 0,0)
|
||||
cx.simulate_resize(gpui::size(px(500.), px(500.)));
|
||||
|
||||
// Draw the editor at a very negative Y position, simulating an editor that's been
|
||||
// scrolled way above the visible viewport (like in a List that has scrolled past it).
|
||||
// The editor is 3000px tall but positioned at y=-10000, so it's entirely above the viewport.
|
||||
// This should NOT hang - it should just render nothing.
|
||||
cx.draw(
|
||||
gpui::point(px(0.), px(-10000.)),
|
||||
gpui::size(px(500.), px(3000.)),
|
||||
|_, _| editor.clone(),
|
||||
);
|
||||
|
||||
// If we get here without hanging, the test passes
|
||||
}
|
||||
|
||||
@@ -9164,6 +9164,15 @@ impl Element for EditorElement {
|
||||
let height_in_lines = f64::from(bounds.size.height / line_height);
|
||||
let max_row = snapshot.max_point().row().as_f64();
|
||||
|
||||
// Calculate how much of the editor is clipped by parent containers (e.g., List).
|
||||
// This allows us to only render lines that are actually visible, which is
|
||||
// critical for performance when large AutoHeight editors are inside Lists.
|
||||
let visible_bounds = window.content_mask().bounds;
|
||||
let clipped_top = (visible_bounds.origin.y - bounds.origin.y).max(px(0.));
|
||||
let clipped_top_in_lines = f64::from(clipped_top / line_height);
|
||||
let visible_height_in_lines =
|
||||
f64::from(visible_bounds.size.height / line_height);
|
||||
|
||||
// The max scroll position for the top of the window
|
||||
let max_scroll_top = if matches!(
|
||||
snapshot.mode,
|
||||
@@ -9220,10 +9229,16 @@ impl Element for EditorElement {
|
||||
let mut scroll_position = snapshot.scroll_position();
|
||||
// The scroll position is a fractional point, the whole number of which represents
|
||||
// the top of the window in terms of display rows.
|
||||
let start_row = DisplayRow(scroll_position.y as u32);
|
||||
// We add clipped_top_in_lines to skip rows that are clipped by parent containers,
|
||||
// but we don't modify scroll_position itself since the parent handles positioning.
|
||||
let max_row = snapshot.max_point().row();
|
||||
let start_row = cmp::min(
|
||||
DisplayRow((scroll_position.y + clipped_top_in_lines).floor() as u32),
|
||||
max_row,
|
||||
);
|
||||
let end_row = cmp::min(
|
||||
(scroll_position.y + height_in_lines).ceil() as u32,
|
||||
(scroll_position.y + clipped_top_in_lines + visible_height_in_lines).ceil()
|
||||
as u32,
|
||||
max_row.next_row().0,
|
||||
);
|
||||
let end_row = DisplayRow(end_row);
|
||||
|
||||
@@ -176,11 +176,9 @@ pub fn block_content_for_tests(
|
||||
}
|
||||
|
||||
pub fn editor_content_with_blocks(editor: &Entity<Editor>, cx: &mut VisualTestContext) -> String {
|
||||
cx.draw(
|
||||
gpui::Point::default(),
|
||||
size(px(3000.0), px(3000.0)),
|
||||
|_, _| editor.clone(),
|
||||
);
|
||||
let draw_size = size(px(3000.0), px(3000.0));
|
||||
cx.simulate_resize(draw_size);
|
||||
cx.draw(gpui::Point::default(), draw_size, |_, _| editor.clone());
|
||||
let (snapshot, mut lines, blocks) = editor.update_in(cx, |editor, window, cx| {
|
||||
let snapshot = editor.snapshot(window, cx);
|
||||
let text = editor.display_text(cx);
|
||||
|
||||
Reference in New Issue
Block a user