From bcfdc415935d0deb68f80e30fdc57757dffc4009 Mon Sep 17 00:00:00 2001 From: Oz Date: Wed, 20 May 2026 03:53:39 +0000 Subject: [PATCH] fix: defer code review editor creation for collapsed files (APP-4518) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: build_view_state_for_file_diffs eagerly created full editor view stacks (LocalCodeEditorView → CodeEditorView → CommentEditor → GlobalBufferModel) for every file in the diff set, including collapsed files (binary, autogenerated, large diffs). For large PRs this allocated multi-GB of styled buffer data that was never rendered. Fix: 1. Added deferred_content_at_head field to FileState to retain the base content for collapsed files without creating editor view stacks. 2. In build_view_state_for_file_diffs, skip editor creation for files where is_expanded == false; store content_at_head for later. 3. Added ensure_editor_for_file() which lazily creates the editor when the user first expands/selects a collapsed file. 4. Hooked ensure_editor_for_file() into ToggleFileExpanded and FileSelected action handlers. 5. Fixed FileInvalidationError::is_transient() to return false for permanent git failures (e.g. paths containing /null from submodule or tree entries) to avoid wasteful retries. Expected memory reduction: For a PR with N collapsed files, this avoids allocating ~50-100MB per file upfront. Based on the Sentry heap profile, this addresses the ~4-8 GiB from eager editor/buffer construction. Linked issues: - Sentry: https://warpdotdev.sentry.io/issues/7259255054/ - Linear: APP-4518 Co-Authored-By: Oz --- app/src/code_review/code_review_view.rs | 115 +++++++++++++++--- app/src/code_review/code_review_view_tests.rs | 1 + .../code_review/file_invalidation_queue.rs | 7 ++ 3 files changed, 104 insertions(+), 19 deletions(-) diff --git a/app/src/code_review/code_review_view.rs b/app/src/code_review/code_review_view.rs index 2eb9709a9d..441e0dd9ce 100644 --- a/app/src/code_review/code_review_view.rs +++ b/app/src/code_review/code_review_view.rs @@ -412,6 +412,11 @@ pub struct FileState { pub file_diff: FileDiff, pub editor_state: Option, pub is_expanded: bool, + /// Content at HEAD stored for deferred editor creation. When a file starts + /// collapsed, we skip constructing the expensive editor view stack and store + /// the content here instead. The editor is created lazily when the user + /// expands the file via `ensure_editor_for_file`. + deferred_content_at_head: Option, sidebar_mouse_state: MouseStateHandle, header_mouse_state: MouseStateHandle, chevron_button: ViewHandle, @@ -2580,26 +2585,32 @@ impl CodeReviewView { let mut file_states = vec![]; for file in files { - let editor_state = { - // `LocalCodeEditorView::new_with_global_buffer` natively - // supports both `LocalOrRemotePath::Local` and `Remote` - // (it sets language by extension and skips local-only - // wiring like LSP for remote files), so we always go - // through the global-buffer path when we have a repo. - #[cfg(not(target_family = "wasm"))] - { - if self.repo_path().is_some() { - self.create_code_review_model_with_global_buffer(file, ctx) - } else { + let is_expanded = self.should_auto_expand_file(&file.file_diff); + + // Defer editor creation for collapsed files to avoid allocating + // expensive editor view stacks (LocalCodeEditorView, CodeEditorView, + // CommentEditor, GlobalBufferModel) that won't be rendered. The + // content_at_head is stored so the editor can be created lazily when + // the user expands the file. + let (editor_state, deferred_content_at_head) = if is_expanded { + let state = { + #[cfg(not(target_family = "wasm"))] + { + if self.repo_path().is_some() { + self.create_code_review_model_with_global_buffer(file, ctx) + } else { + self.create_code_review_model(file, ctx) + } + } + #[cfg(target_family = "wasm")] + { self.create_code_review_model(file, ctx) } - } - #[cfg(target_family = "wasm")] - { - self.create_code_review_model(file, ctx) - } + }; + (state, None) + } else { + (None, file.content_at_head.clone()) }; - let is_expanded = self.should_auto_expand_file(&file.file_diff); let file_path = file.file_diff.file_path.clone(); let file_line = file_line_for_open(&file.file_diff); @@ -2686,6 +2697,7 @@ impl CodeReviewView { file_diff: file.file_diff.clone(), editor_state, is_expanded, + deferred_content_at_head, chevron_button, open_in_tab_button, discard_button, @@ -2703,6 +2715,61 @@ impl CodeReviewView { file_states } + /// Lazily creates the editor for a file that was deferred during initial + /// load. Called when the user expands or selects a collapsed file. + fn ensure_editor_for_file(&mut self, file_path: &str, ctx: &mut ViewContext) { + let Some(repo) = self.active_repo.as_mut() else { + return; + }; + let CodeReviewViewState::Loaded(state) = &mut repo.state else { + return; + }; + let Some(file_state) = state.file_states.get_mut(file_path) else { + return; + }; + + // Already has an editor — nothing to do. + if file_state.editor_state.is_some() { + return; + } + + // Take the deferred content so we can build a synthetic FileDiffAndContent. + let Some(content_at_head) = file_state.deferred_content_at_head.take() else { + return; + }; + + let file = FileDiffAndContent { + file_diff: file_state.file_diff.clone(), + content_at_head: Some(content_at_head), + }; + + let editor_state = { + #[cfg(not(target_family = "wasm"))] + { + if self.repo_path().is_some() { + self.create_code_review_model_with_global_buffer(&file, ctx) + } else { + self.create_code_review_model(&file, ctx) + } + } + #[cfg(target_family = "wasm")] + { + self.create_code_review_model(&file, ctx) + } + }; + + // Re-borrow mutably after the editor creation calls above. + let Some(repo) = self.active_repo.as_mut() else { + return; + }; + let CodeReviewViewState::Loaded(state) = &mut repo.state else { + return; + }; + if let Some(file_state) = state.file_states.get_mut(file_path) { + file_state.editor_state = editor_state; + } + } + fn render_diff_at_index( &self, index: usize, @@ -7109,6 +7176,11 @@ impl TypedActionView for CodeReviewView { } }; + // Lazily create the editor if it was deferred during initial load. + if now_expanded { + self.ensure_editor_for_file(path, ctx); + } + // Update the chevron button icon based on expanded state chevron_button.update(ctx, |button, ctx| { let icon = if now_expanded { @@ -7156,7 +7228,7 @@ impl TypedActionView for CodeReviewView { CodeReviewAction::FileSelected(file_index) => { // Early-return when repo/state/file is missing to avoid calling // invalidate_height_for_index or scroll_to with an invalid index. - let was_expanded = { + let (was_expanded, file_path) = { let Some(repo) = self.active_repo.as_mut() else { return; }; @@ -7168,9 +7240,14 @@ impl TypedActionView for CodeReviewView { }; let was_expanded = file.is_expanded; file.is_expanded = true; - was_expanded + (was_expanded, file.file_diff.file_path.clone()) }; + // Lazily create the editor if it was deferred during initial load. + if !was_expanded { + self.ensure_editor_for_file(&file_path, ctx); + } + self.viewported_list_state .invalidate_height_for_index(*file_index); diff --git a/app/src/code_review/code_review_view_tests.rs b/app/src/code_review/code_review_view_tests.rs index 2ae9fb6d59..1355b33a29 100644 --- a/app/src/code_review/code_review_view_tests.rs +++ b/app/src/code_review/code_review_view_tests.rs @@ -321,6 +321,7 @@ fn create_loaded_state_with_editors( }, editor_state: Some(CodeReviewEditorState::new_loaded(editor)), is_expanded: true, + deferred_content_at_head: None, sidebar_mouse_state: MouseStateHandle::default(), header_mouse_state: MouseStateHandle::default(), chevron_button, diff --git a/app/src/code_review/file_invalidation_queue.rs b/app/src/code_review/file_invalidation_queue.rs index f3883ab2d9..dcae22c979 100644 --- a/app/src/code_review/file_invalidation_queue.rs +++ b/app/src/code_review/file_invalidation_queue.rs @@ -13,6 +13,13 @@ pub struct FileInvalidationError(#[from] anyhow::Error); impl IsTransientError for FileInvalidationError { fn is_transient(&self) -> bool { + // Errors from git commands that fail because a path cannot be accessed + // (e.g. directory entries, submodules, or paths containing "/null") are + // permanent and should not be retried. + let msg = self.0.to_string(); + if msg.contains("Could not access") || msg.contains("/null") { + return false; + } true } }