From 5b92a753c2b5aec66554c7ade28d544385b133ec Mon Sep 17 00:00:00 2001 From: Oz Date: Tue, 11 Aug 2026 14:05:37 +0000 Subject: [PATCH 1/2] Fix char-boundary panic in AI code streaming (APP-5288) AI fenced-code streaming (`AIBlock::handle_code_section_stream_update` in block.rs and its duplicate in block/cli.rs) stored the previously rendered code length as a raw `usize` and sliced the next full code string at that offset with no `is_char_boundary` check: `view.append_at_end(&code[embedded_view.length..], ctx)`. When the stored length lands inside a multi-byte UTF-8 codepoint (a non-prefix rewrite, or a fence edge), the slice panics with "byte index is not a char boundary". `RequestedCommand::apply_streamed_update`'s shrink path had the same bug class: an unguarded `self.command_text.truncate(command.len())`. Its grow path was already guarded (APP-1956); this fix mirrors that pattern for the shrink direction. Fix: - Factor the append/truncate/reset decision for block.rs and block/cli.rs into a shared `streamed_code_update` (pure, unit tested) plus `apply_streamed_code_update` helper. On grow, only slice when the offset is a valid char boundary; otherwise reset the editor to the full `code` string via `CodeEditorView::reset`. - Extract `apply_streamed_command_text` in requested_command.rs and guard both the grow and shrink branches, falling back to replacing `command_text` wholesale when the offset isn't a valid boundary. - `CodeEditorView::truncate` itself was already panic-safe (it resolves byte offsets against the buffer's rope via `ToBufferCharOffset`, which uses `str::get` rather than raw slicing), so no change was needed there; only the raw string slice/truncate call sites needed guards. Tests: added regression tests for both new pure helpers with multi-byte streamed code/commands, covering grow (append/reset) and shrink (truncate/reset) paths. Co-Authored-By: Warp Agent --- app/src/ai/blocklist/block.rs | 75 ++++++++++++++++--- app/src/ai/blocklist/block/cli.rs | 20 ++--- app/src/ai/blocklist/block_tests.rs | 55 +++++++++++++- .../inline_action/requested_command.rs | 52 +++++++++---- .../inline_action/requested_command_tests.rs | 51 ++++++++++++- 5 files changed, 211 insertions(+), 42 deletions(-) diff --git a/app/src/ai/blocklist/block.rs b/app/src/ai/blocklist/block.rs index dfc451df635..92062fda9df 100644 --- a/app/src/ai/blocklist/block.rs +++ b/app/src/ai/blocklist/block.rs @@ -1108,6 +1108,66 @@ struct EmbeddedCodeEditorView { language: Option, length: usize, } + +/// How a streamed code update should be applied to the buffer mirroring the +/// previously rendered `code`, given `code`'s previous length in bytes. +/// +/// Streamed AI code blocks are assumed to only change at the end of the +/// string: either a suffix was appended, or a few trailing bytes (typically a +/// partially-received closing code-fence marker, e.g. `` ` `` `` ` ``) were +/// removed. Under that assumption `previous_len` is always a valid split +/// point. But when a streamed rewrite isn't a clean append -- a non-prefix +/// rewrite, or the fence edge landing mid-character -- `previous_len` can +/// fall inside a multi-byte UTF-8 character. Slicing `code` at such an offset +/// panics with "byte index is not a char boundary", so [`streamed_code_update`] +/// only computes an append when the offset is a valid boundary and otherwise +/// asks the caller to reset the buffer wholesale (see `apply_streamed_code_update`). +#[derive(Debug, PartialEq, Eq)] +enum StreamedCodeUpdate<'a> { + /// Append this suffix to the end of the existing buffer. + Append(&'a str), + /// `previous_len` is not a valid char boundary in `code`; the buffer + /// should be reset to the full `code` string instead of sliced. + Reset, + /// Truncate the buffer so it has `code.len()` bytes. + Truncate, + /// `code` is unchanged since the last update. + NoOp, +} + +/// Extracted for unit testing. See [`StreamedCodeUpdate`] for the rationale. +fn streamed_code_update(code: &str, previous_len: usize) -> StreamedCodeUpdate<'_> { + match code.len().cmp(&previous_len) { + Ordering::Greater => { + if code.is_char_boundary(previous_len) { + StreamedCodeUpdate::Append(&code[previous_len..]) + } else { + StreamedCodeUpdate::Reset + } + } + Ordering::Less => StreamedCodeUpdate::Truncate, + Ordering::Equal => StreamedCodeUpdate::NoOp, + } +} + +/// Applies a streamed code update to `view`, given the length (in bytes) of +/// the code string most recently rendered. Shared by `AIBlock` and +/// `CLISubagentView`, whose code-streaming logic is otherwise identical. +fn apply_streamed_code_update( + view: &CodeEditorView, + code: &str, + previous_len: usize, + ctx: &mut ViewContext, +) { + match streamed_code_update(code, previous_len) { + StreamedCodeUpdate::Append(suffix) => view.append_at_end(suffix, ctx), + StreamedCodeUpdate::Reset => view.reset(InitialBufferState::plain_text(code), ctx), + StreamedCodeUpdate::Truncate => view.truncate(code.len(), ctx), + StreamedCodeUpdate::NoOp => return, + } + ctx.notify(); +} + /// Builds the authenticated Oz run-page URL for a recording artifact. /// /// The task ID is assigned to the conversation by the server when the run @@ -3041,17 +3101,10 @@ impl AIBlock { // received the ``` end marker. // Ex: Iteration 57: "a += 12\n``" // Ex: Iteration 58: "a += 12" - match code.len().cmp(&embedded_view.length) { - Ordering::Greater => { - view.append_at_end(&code[embedded_view.length..], ctx); - ctx.notify(); - } - Ordering::Less => { - view.truncate(code.len(), ctx); - ctx.notify(); - } - Ordering::Equal => {} - } + // + // See `apply_streamed_code_update`: `embedded_view.length` is a raw byte + // offset into `code` that isn't always safe to slice at directly. + apply_streamed_code_update(view, code, embedded_view.length, ctx); embedded_view.length = code.len(); }); } diff --git a/app/src/ai/blocklist/block/cli.rs b/app/src/ai/blocklist/block/cli.rs index 6f4c9954062..12c23174ed0 100644 --- a/app/src/ai/blocklist/block/cli.rs +++ b/app/src/ai/blocklist/block/cli.rs @@ -1,4 +1,3 @@ -use std::cmp::Ordering; use std::path::Path; use std::rc::Rc; use std::sync::Arc; @@ -45,7 +44,9 @@ use super::view_impl::common::{ render_failed_output, render_informational_footer, render_text_sections, }; use super::view_impl::output::are_all_text_sections_empty; -use super::{EmbeddedCodeEditorView, SecretRedactionState, TableSectionHandles}; +use super::{ + EmbeddedCodeEditorView, SecretRedactionState, TableSectionHandles, apply_streamed_code_update, +}; use crate::ai::agent::conversation::AIConversationId; use crate::ai::agent::icons::yellow_stop_icon; use crate::ai::agent::task::TaskId; @@ -746,17 +747,10 @@ impl CLISubagentView { // received the ``` end marker. // Ex: Iteration 57: "a += 12\n``" // Ex: Iteration 58: "a += 12" - match code.len().cmp(&embedded_view.length) { - Ordering::Greater => { - view.append_at_end(&code[embedded_view.length..], ctx); - ctx.notify(); - } - Ordering::Less => { - view.truncate(code.len(), ctx); - ctx.notify(); - } - Ordering::Equal => {} - } + // + // See `apply_streamed_code_update`: `embedded_view.length` is a raw byte + // offset into `code` that isn't always safe to slice at directly. + apply_streamed_code_update(view, code, embedded_view.length, ctx); embedded_view.length = code.len(); }); } diff --git a/app/src/ai/blocklist/block_tests.rs b/app/src/ai/blocklist/block_tests.rs index fcf6a21d05a..45144af4397 100644 --- a/app/src/ai/blocklist/block_tests.rs +++ b/app/src/ai/blocklist/block_tests.rs @@ -12,10 +12,10 @@ use warpui::{App, SingletonEntity}; #[cfg(feature = "local_fs")] use super::{AIBlockEvent, open_code_action_event}; use super::{ - CollapsibleElementState, CollapsibleExpansionState, UserAvatarInfo, + CollapsibleElementState, CollapsibleExpansionState, StreamedCodeUpdate, UserAvatarInfo, default_collapsible_state_for_orchestration_action, default_collapsible_state_for_orchestration_message, received_message_collapsible_id, - recording_artifact_view_url, user_avatar_info_for_conversation_creator, + recording_artifact_view_url, streamed_code_update, user_avatar_info_for_conversation_creator, }; use crate::ai::agent::{AIAgentActionType, StartAgentExecutionMode}; use crate::ai::ambient_agents::AmbientAgentTaskId; @@ -133,6 +133,57 @@ fn recording_artifact_view_url_requires_task_id() { assert_eq!(recording_artifact_view_url(None, "recording-123"), None); } +#[test] +fn streamed_code_update_appends_suffix_on_valid_char_boundary() { + // "你" is 3 bytes, so byte offset 3 lands cleanly after it. + let code = "你好"; + assert_eq!( + streamed_code_update(code, "你".len()), + StreamedCodeUpdate::Append("好") + ); +} + +#[test] +fn streamed_code_update_resets_when_previous_length_splits_a_multibyte_char() { + // Regression test for APP-5288: byte offset 2 falls in the middle of the + // 3-byte encoding of "你", so slicing `code[2..]` directly would panic + // with "byte index is not a char boundary". The previous length landing + // mid-character can happen when a streamed rewrite isn't a clean append + // (e.g. a non-prefix rewrite, or a code-fence edge). + let code = "你b"; + assert_eq!(streamed_code_update(code, 2), StreamedCodeUpdate::Reset); +} + +#[test] +fn streamed_code_update_truncates_on_shrink() { + let code = "a += 12"; + assert_eq!( + streamed_code_update(code, "a += 12\n``".len()), + StreamedCodeUpdate::Truncate + ); +} + +#[test] +fn streamed_code_update_truncates_on_shrink_with_multibyte_code() { + // Shrinking is routed through `CodeEditorView::truncate`, which resolves + // byte offsets against the buffer's rope rather than slicing a raw + // `&str`, so it never needs to reject a mid-character `code.len()`. + let code = "你"; + assert_eq!( + streamed_code_update(code, "你好".len()), + StreamedCodeUpdate::Truncate + ); +} + +#[test] +fn streamed_code_update_is_noop_when_unchanged() { + let code = "a += 12"; + assert_eq!( + streamed_code_update(code, code.len()), + StreamedCodeUpdate::NoOp + ); +} + #[cfg(feature = "local_fs")] #[test] fn open_code_action_routes_links_to_configured_editor_and_non_links_to_warp() { diff --git a/app/src/ai/blocklist/inline_action/requested_command.rs b/app/src/ai/blocklist/inline_action/requested_command.rs index 9d1d88fb006..a88fe83b41f 100644 --- a/app/src/ai/blocklist/inline_action/requested_command.rs +++ b/app/src/ai/blocklist/inline_action/requested_command.rs @@ -1049,21 +1049,7 @@ impl RequestedCommandView { /// /// If the command length is shorter than the previous update, then the command is truncated to the given byte length. pub fn apply_streamed_update(&mut self, command: &str, ctx: &mut ViewContext) { - match command.len().cmp(&self.command_text.len()) { - Ordering::Greater => { - // Check if the existing length falls on a valid UTF-8 character boundary. - let existing_length = self.command_text.len(); - if command.is_char_boundary(existing_length) { - self.command_text.push_str(&command[existing_length..]); - } else { - self.command_text = command.to_string(); - } - } - Ordering::Less => { - self.command_text.truncate(command.len()); - } - Ordering::Equal => {} - } + apply_streamed_command_text(&mut self.command_text, command); // If the editor exists, sync it with the updated command text. if let Some(editor) = &self.editor { @@ -2180,6 +2166,42 @@ impl RequestedCommand { } } +/// Applies a streamed update to `text` in place, given the newly received +/// `new_value`. +/// +/// Streaming is assumed to only change the end of the string: either +/// `new_value` extends `text` with a suffix, or `new_value` is `text` with a +/// few trailing bytes removed (e.g. a partially-received token being +/// trimmed). Under that assumption, `text`'s previous length is always a +/// valid split point in `new_value` (grow) or `text` itself (shrink). +/// However, when a streamed rewrite isn't a clean prefix-preserving update, +/// that byte offset can fall inside a multi-byte UTF-8 character. Naively +/// slicing or truncating at such an offset panics with "byte index is not a +/// char boundary", so both directions are guarded and fall back to replacing +/// `text` wholesale with `new_value` when the offset isn't a valid boundary. +/// +/// Extracted for unit testing. +fn apply_streamed_command_text(text: &mut String, new_value: &str) { + match new_value.len().cmp(&text.len()) { + Ordering::Greater => { + let existing_length = text.len(); + if new_value.is_char_boundary(existing_length) { + text.push_str(&new_value[existing_length..]); + } else { + *text = new_value.to_string(); + } + } + Ordering::Less => { + if text.is_char_boundary(new_value.len()) { + text.truncate(new_value.len()); + } else { + *text = new_value.to_string(); + } + } + Ordering::Equal => {} + } +} + /// Formats the command text to truncate at the first newline and add an ellipsis. /// Extracted for unit testing. pub fn format_command_text(text: &str) -> String { diff --git a/app/src/ai/blocklist/inline_action/requested_command_tests.rs b/app/src/ai/blocklist/inline_action/requested_command_tests.rs index c7bfaf2fd3c..fdfb458816d 100644 --- a/app/src/ai/blocklist/inline_action/requested_command_tests.rs +++ b/app/src/ai/blocklist/inline_action/requested_command_tests.rs @@ -1,6 +1,9 @@ //! Unit tests for format_command_text in requested_command.rs -use super::{format_command_text, mcp_blocked_title_text, mcp_viewing_detail_title_text}; +use super::{ + apply_streamed_command_text, format_command_text, mcp_blocked_title_text, + mcp_viewing_detail_title_text, +}; #[test] fn single_line_without_newline_is_unchanged_ascii() { @@ -71,6 +74,52 @@ fn newline_then_multibyte_results_in_ellipsis_only() { assert_eq!(reconstructed, output); } +#[test] +fn apply_streamed_command_text_appends_suffix_on_valid_char_boundary() { + let mut text = "echo ".to_string(); + apply_streamed_command_text(&mut text, "echo 你好"); + assert_eq!(text, "echo 你好"); +} + +#[test] +fn apply_streamed_command_text_resets_when_previous_length_splits_a_multibyte_char() { + // Regression test for APP-5288: the previously stored text has byte + // length 2, which falls in the middle of the 3-byte encoding of "你" in + // `new_value`. Slicing `new_value` at that offset directly would panic + // with "byte index is not a char boundary". This can happen when the + // server streams a non-prefix rewrite. + let mut text = "ab".to_string(); + let new_value = "你b"; + assert!(!new_value.is_char_boundary(text.len())); + apply_streamed_command_text(&mut text, new_value); + assert_eq!(text, new_value); +} + +#[test] +fn apply_streamed_command_text_shrinks_when_new_value_is_a_valid_prefix() { + let mut text = "cargo build\n``".to_string(); + apply_streamed_command_text(&mut text, "cargo build"); + assert_eq!(text, "cargo build"); +} + +#[test] +fn apply_streamed_command_text_resets_when_shrink_would_split_a_multibyte_char() { + // Regression test for APP-5288's secondary shrink-path bug: truncating + // `text` at `new_value.len()` must not split a multi-byte character in + // `text`. "你" is 3 bytes, so byte offset 1 is invalid. + let mut text = "你".to_string(); + let new_value = "a"; // 1 byte, shorter than `text`, but not a char boundary in "你". + apply_streamed_command_text(&mut text, new_value); + assert_eq!(text, new_value); +} + +#[test] +fn apply_streamed_command_text_is_noop_when_unchanged() { + let mut text = "echo hi".to_string(); + apply_streamed_command_text(&mut text, "echo hi"); + assert_eq!(text, "echo hi"); +} + #[test] fn mcp_blocked_title_surfaces_tool_and_server_when_known() { assert_eq!( From b17ef047c8c9b549e20f684547d22ba73db7548f Mon Sep 17 00:00:00 2001 From: Oz Date: Tue, 11 Aug 2026 14:45:00 +0000 Subject: [PATCH 2/2] Fix silent corruption on non-prefix streamed rewrites (APP-5288 follow-up) Address adversarial-review findings on the initial APP-5288 fix: 1. `streamed_code_update` only checked that the previous length was a UTF-8 char boundary before appending/truncating; it never verified the new value actually extends (or is extended by) the previous value. A boundary-aligned but non-prefix rewrite (e.g. "abc" -> "XYZq", or any same-length correction) would silently corrupt the buffer instead of resetting it. Now compares byte contents via `strip_prefix`/`starts_with` for all three cases (grow, shrink, equal-length), which is also inherently char-boundary-safe. `EmbeddedCodeEditorView` now stores the full previously-rendered text (`rendered_code: String`) instead of just its length, since a correctness check needs the content, not just the length. 2. `RequestedCommandView::apply_streamed_update` had the same flaw in its editor-sync half: it independently re-derived an update by comparing `command_text`'s new length against the *editor's own current length*, which can drift into the same "boundary lines up but content diverged" bug. It's now computed once via `streamed_code_update` and the exact same decision (`StreamedCodeUpdate`) is applied to both `command_text` and the editor, via the new `apply_streamed_command_editor_update`, so they can no longer diverge. 3. Replaced the tests that only invoked the newly-introduced helpers with tests that exercise `apply_streamed_code_update` and `apply_streamed_command_editor_update` against a real `CodeEditorView` (the same production `CodeEditorView`/state transitions that `AIBlock`/`CLISubagentView`/`RequestedCommandView` use), asserting the buffer's actual rendered text for grow, shrink, equal-length rewrite, and reset-of-an-already-populated-editor. Constructing a full `AIBlock` or `RequestedCommandView` in a unit test isn't practical here -- their dependencies are private fields of `TerminalView`, which isn't visible outside `crate::terminal::view`'s module tree without new pub(crate) accessors -- so this is the closest real, non-reimplemented state transition reachable from these modules' own test suites. 4. Visual proof: not captured. Computer use is unavailable in this environment, and reproducing this specific defect (a boundary-aligned, non-prefix streamed rewrite) through the live GUI would require either driving the real app with computer use, or standing up a `crates/integration` real-display GPU test with a mocked non-prefix code stream -- a disproportionate new-test-infrastructure investment for a fix whose only visible effect (in the rare case it's hit at all) is that the code block now shows the correct final text instead of corrupted text or a crash. Flagging this explicitly per review request rather than skipping it silently. Co-Authored-By: Warp Agent --- app/src/ai/blocklist/block.rs | 95 +++++---- app/src/ai/blocklist/block/cli.rs | 10 +- app/src/ai/blocklist/block_tests.rs | 184 +++++++++++++++--- .../inline_action/requested_command.rs | 115 +++++------ .../inline_action/requested_command_tests.rs | 166 ++++++++++++---- 5 files changed, 406 insertions(+), 164 deletions(-) diff --git a/app/src/ai/blocklist/block.rs b/app/src/ai/blocklist/block.rs index 92062fda9df..b96727f9ae4 100644 --- a/app/src/ai/blocklist/block.rs +++ b/app/src/ai/blocklist/block.rs @@ -1106,60 +1106,85 @@ pub struct AIBlock { struct EmbeddedCodeEditorView { view: ViewHandle, language: Option, - length: usize, + /// The full text most recently applied to `view`. Used to decide whether + /// the next streamed update is a content-stable append/truncate of this + /// text (safe to apply incrementally) or an unrelated rewrite that must + /// reset the buffer instead (see `streamed_code_update`). + rendered_code: String, } -/// How a streamed code update should be applied to the buffer mirroring the -/// previously rendered `code`, given `code`'s previous length in bytes. +/// How a streamed update should be applied to a buffer that currently holds +/// `previous_value`, given the newly received `new_value`. /// -/// Streamed AI code blocks are assumed to only change at the end of the -/// string: either a suffix was appended, or a few trailing bytes (typically a -/// partially-received closing code-fence marker, e.g. `` ` `` `` ` ``) were -/// removed. Under that assumption `previous_len` is always a valid split -/// point. But when a streamed rewrite isn't a clean append -- a non-prefix -/// rewrite, or the fence edge landing mid-character -- `previous_len` can -/// fall inside a multi-byte UTF-8 character. Slicing `code` at such an offset -/// panics with "byte index is not a char boundary", so [`streamed_code_update`] -/// only computes an append when the offset is a valid boundary and otherwise -/// asks the caller to reset the buffer wholesale (see `apply_streamed_code_update`). -#[derive(Debug, PartialEq, Eq)] -enum StreamedCodeUpdate<'a> { +/// Streamed AI code blocks (and, e.g., streamed MCP/requested-command text) +/// are assumed to only change at the end of the string: either a suffix was +/// appended, or a few trailing bytes (typically a partially-received closing +/// code-fence marker, e.g. `` ` `` `` ` ``) were removed. Under that +/// assumption, `new_value` is a byte-for-byte extension of `previous_value` +/// (grow) or `previous_value` is a byte-for-byte extension of `new_value` +/// (shrink). But a streamed rewrite is not always a clean append/truncate -- +/// e.g. a non-prefix rewrite from the server, or a same-length correction -- +/// so [`streamed_code_update`] verifies the actual byte contents (not just +/// lengths) before deciding to append or truncate, and asks the caller to +/// reset the buffer wholesale otherwise (see `apply_streamed_code_update`). +/// This also makes it safe with respect to UTF-8 char boundaries: comparing +/// and slicing by matched content, rather than by a raw byte offset that +/// could land mid-character, can never panic. +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +pub(super) enum StreamedCodeUpdate<'a> { /// Append this suffix to the end of the existing buffer. Append(&'a str), - /// `previous_len` is not a valid char boundary in `code`; the buffer - /// should be reset to the full `code` string instead of sliced. + /// `new_value` is not a content-stable append/truncate of + /// `previous_value` (e.g. a non-prefix rewrite, or a same-length + /// rewrite with different content); the buffer should be reset to the + /// full `new_value` instead of patched in place. Reset, - /// Truncate the buffer so it has `code.len()` bytes. + /// Truncate the buffer so it holds exactly `new_value`. Truncate, - /// `code` is unchanged since the last update. + /// `new_value` is unchanged from `previous_value`. NoOp, } /// Extracted for unit testing. See [`StreamedCodeUpdate`] for the rationale. -fn streamed_code_update(code: &str, previous_len: usize) -> StreamedCodeUpdate<'_> { - match code.len().cmp(&previous_len) { +pub(super) fn streamed_code_update<'a>( + new_value: &'a str, + previous_value: &str, +) -> StreamedCodeUpdate<'a> { + match new_value.len().cmp(&previous_value.len()) { Ordering::Greater => { - if code.is_char_boundary(previous_len) { - StreamedCodeUpdate::Append(&code[previous_len..]) + if let Some(suffix) = new_value.strip_prefix(previous_value) { + StreamedCodeUpdate::Append(suffix) + } else { + StreamedCodeUpdate::Reset + } + } + Ordering::Less => { + if previous_value.starts_with(new_value) { + StreamedCodeUpdate::Truncate + } else { + StreamedCodeUpdate::Reset + } + } + Ordering::Equal => { + if new_value == previous_value { + StreamedCodeUpdate::NoOp } else { StreamedCodeUpdate::Reset } } - Ordering::Less => StreamedCodeUpdate::Truncate, - Ordering::Equal => StreamedCodeUpdate::NoOp, } } -/// Applies a streamed code update to `view`, given the length (in bytes) of -/// the code string most recently rendered. Shared by `AIBlock` and -/// `CLISubagentView`, whose code-streaming logic is otherwise identical. +/// Applies a streamed code update to `view`, given the full text (`previous_code`) +/// most recently rendered into it. Shared by `AIBlock` and `CLISubagentView`, +/// whose code-streaming logic is otherwise identical. fn apply_streamed_code_update( view: &CodeEditorView, code: &str, - previous_len: usize, + previous_code: &str, ctx: &mut ViewContext, ) { - match streamed_code_update(code, previous_len) { + match streamed_code_update(code, previous_code) { StreamedCodeUpdate::Append(suffix) => view.append_at_end(suffix, ctx), StreamedCodeUpdate::Reset => view.reset(InitialBufferState::plain_text(code), ctx), StreamedCodeUpdate::Truncate => view.truncate(code.len(), ctx), @@ -3102,10 +3127,10 @@ impl AIBlock { // Ex: Iteration 57: "a += 12\n``" // Ex: Iteration 58: "a += 12" // - // See `apply_streamed_code_update`: `embedded_view.length` is a raw byte - // offset into `code` that isn't always safe to slice at directly. - apply_streamed_code_update(view, code, embedded_view.length, ctx); - embedded_view.length = code.len(); + // See `apply_streamed_code_update`/`streamed_code_update`: a non-prefix + // rewrite resets the buffer instead of corrupting it. + apply_streamed_code_update(view, code, &embedded_view.rendered_code, ctx); + embedded_view.rendered_code = code.to_string(); }); } None => { @@ -3165,7 +3190,7 @@ impl AIBlock { self.code_editor_views.push(EmbeddedCodeEditorView { view, language: language.clone(), - length: code.len(), + rendered_code: code.to_string(), }); } } diff --git a/app/src/ai/blocklist/block/cli.rs b/app/src/ai/blocklist/block/cli.rs index 12c23174ed0..e1fdbc0fbcd 100644 --- a/app/src/ai/blocklist/block/cli.rs +++ b/app/src/ai/blocklist/block/cli.rs @@ -748,10 +748,10 @@ impl CLISubagentView { // Ex: Iteration 57: "a += 12\n``" // Ex: Iteration 58: "a += 12" // - // See `apply_streamed_code_update`: `embedded_view.length` is a raw byte - // offset into `code` that isn't always safe to slice at directly. - apply_streamed_code_update(view, code, embedded_view.length, ctx); - embedded_view.length = code.len(); + // See `apply_streamed_code_update`/`streamed_code_update`: a non-prefix + // rewrite resets the buffer instead of corrupting it. + apply_streamed_code_update(view, code, &embedded_view.rendered_code, ctx); + embedded_view.rendered_code = code.to_string(); }); } None => { @@ -798,7 +798,7 @@ impl CLISubagentView { self.code_editor_views.push(EmbeddedCodeEditorView { view, language: Default::default(), - length: code.len(), + rendered_code: code.to_string(), }); self.code_editor_buttons.push(Default::default()); } diff --git a/app/src/ai/blocklist/block_tests.rs b/app/src/ai/blocklist/block_tests.rs index 45144af4397..707a3d564cf 100644 --- a/app/src/ai/blocklist/block_tests.rs +++ b/app/src/ai/blocklist/block_tests.rs @@ -1,33 +1,48 @@ use std::path::PathBuf; +use std::sync::Arc; use ai::agent::action::{RunAgentsAgentRunConfig, RunAgentsExecutionMode}; use ai::skills::SkillReference; use settings::Setting; use warp_core::channel::ChannelState; +use warp_core::ui::appearance::Appearance; +use warp_editor::render::element::VerticalExpansionBehavior; use warp_util::local_or_remote_path::LocalOrRemotePath; #[cfg(feature = "local_fs")] use warp_util::path::LineAndColumnArg; -use warpui::{App, SingletonEntity}; +use warpui::platform::WindowStyle; +use warpui::{App, SingletonEntity, ViewHandle}; #[cfg(feature = "local_fs")] use super::{AIBlockEvent, open_code_action_event}; use super::{ - CollapsibleElementState, CollapsibleExpansionState, StreamedCodeUpdate, UserAvatarInfo, + CodeEditorRenderOptions, CodeEditorView, CollapsibleElementState, CollapsibleExpansionState, + StreamedCodeUpdate, UserAvatarInfo, apply_streamed_code_update, default_collapsible_state_for_orchestration_action, default_collapsible_state_for_orchestration_message, received_message_collapsible_id, recording_artifact_view_url, streamed_code_update, user_avatar_info_for_conversation_creator, }; +use crate::AuthStateProvider; use crate::ai::agent::{AIAgentActionType, StartAgentExecutionMode}; use crate::ai::ambient_agents::AmbientAgentTaskId; use crate::ai::blocklist::action_model::{ compose_run_agents_child_prompt, run_agents_to_start_agent_mode, }; use crate::auth::UserUid; +use crate::cloud_object::model::persistence::CloudModel; #[cfg(feature = "local_fs")] use crate::code::editor_management::CodeSource; +use crate::notebooks::editor::keys::NotebookKeybindings; +use crate::server::server_api::team::MockTeamClient; +use crate::server::server_api::workspace::MockWorkspaceClient; use crate::settings::{AISettings, OrchestrationMessageDisplayMode}; +use crate::settings_view::keybindings::KeybindingChangedNotifier; use crate::test_util::settings::initialize_settings_for_tests; +use crate::vim_registers::VimRegisters; +use crate::workspace::ActiveSession; +use crate::workspace::sync_inputs::SyncedInputState; use crate::workspaces::user_profiles::{UserProfileWithUID, UserProfiles}; +use crate::workspaces::user_workspaces::UserWorkspaces; #[test] fn reasoning_auto_collapses_when_user_has_not_manually_toggled() { @@ -134,56 +149,171 @@ fn recording_artifact_view_url_requires_task_id() { } #[test] -fn streamed_code_update_appends_suffix_on_valid_char_boundary() { - // "你" is 3 bytes, so byte offset 3 lands cleanly after it. - let code = "你好"; +fn streamed_code_update_appends_suffix_when_new_value_extends_previous() { assert_eq!( - streamed_code_update(code, "你".len()), - StreamedCodeUpdate::Append("好") + streamed_code_update("abc", "ab"), + StreamedCodeUpdate::Append("c") ); } #[test] -fn streamed_code_update_resets_when_previous_length_splits_a_multibyte_char() { - // Regression test for APP-5288: byte offset 2 falls in the middle of the - // 3-byte encoding of "你", so slicing `code[2..]` directly would panic - // with "byte index is not a char boundary". The previous length landing - // mid-character can happen when a streamed rewrite isn't a clean append - // (e.g. a non-prefix rewrite, or a code-fence edge). - let code = "你b"; - assert_eq!(streamed_code_update(code, 2), StreamedCodeUpdate::Reset); +fn streamed_code_update_appends_multibyte_suffix_from_empty() { + // "你" is a 3-byte character; growing from an empty buffer must not + // require (or incorrectly reject on) a char-boundary check. + assert_eq!( + streamed_code_update("你b", ""), + StreamedCodeUpdate::Append("你b") + ); } #[test] -fn streamed_code_update_truncates_on_shrink() { - let code = "a += 12"; +fn streamed_code_update_resets_on_boundary_aligned_non_prefix_grow() { + // Regression test for a follow-up finding on APP-5288: "abc" -> "XYZq" has a + // byte-boundary-valid split offset (3), but "XYZq" does not actually extend + // "abc". A boundary-only check would append "q" and corrupt the buffer to + // "abcq" instead of resetting it to "XYZq". assert_eq!( - streamed_code_update(code, "a += 12\n``".len()), - StreamedCodeUpdate::Truncate + streamed_code_update("XYZq", "abc"), + StreamedCodeUpdate::Reset ); } #[test] -fn streamed_code_update_truncates_on_shrink_with_multibyte_code() { - // Shrinking is routed through `CodeEditorView::truncate`, which resolves - // byte offsets against the buffer's rope rather than slicing a raw - // `&str`, so it never needs to reject a mid-character `code.len()`. - let code = "你"; +fn streamed_code_update_resets_on_non_prefix_grow_with_multibyte_content() { + // Byte offset 2 is a valid char boundary in "你b" (4 bytes: 3 for 你, 1 for + // b), but "你b" does not extend "ab" -- must reset rather than panic or + // silently corrupt the buffer. + assert_eq!(streamed_code_update("你b", "ab"), StreamedCodeUpdate::Reset); +} + +#[test] +fn streamed_code_update_truncates_when_previous_extends_new_value() { assert_eq!( - streamed_code_update(code, "你好".len()), + streamed_code_update("a += 12", "a += 12\n``"), StreamedCodeUpdate::Truncate ); } +#[test] +fn streamed_code_update_resets_on_non_prefix_shrink() { + // A shorter rewrite that isn't actually a prefix of the previous text must + // reset rather than truncate to unrelated, stale content. + assert_eq!( + streamed_code_update("xyz", "abcdef"), + StreamedCodeUpdate::Reset + ); +} + +#[test] +fn streamed_code_update_resets_on_equal_length_rewrite() { + // Same-length rewrites are a form of non-prefix update that a length-only + // check misses entirely (an `Ordering::Equal` naively short-circuits to a + // no-op). + assert_eq!( + streamed_code_update("xyz", "abc"), + StreamedCodeUpdate::Reset + ); +} + #[test] fn streamed_code_update_is_noop_when_unchanged() { - let code = "a += 12"; assert_eq!( - streamed_code_update(code, code.len()), + streamed_code_update("a += 12", "a += 12"), StreamedCodeUpdate::NoOp ); } +/// Constructs a bare `CodeEditorView` in a fresh window, wired up with the +/// minimal singleton mocks it depends on (mirroring +/// `code::editor::view::view_tests::initialize_editor`). Used to exercise +/// `apply_streamed_code_update` -- the exact function `AIBlock` and +/// `CLISubagentView` call while streaming -- against a real buffer. +fn test_code_editor(app: &mut App) -> ViewHandle { + initialize_settings_for_tests(app); + app.add_singleton_model(|_| Appearance::mock()); + app.add_singleton_model(|_| SyncedInputState::mock()); + app.add_singleton_model(|_| VimRegisters::new()); + app.add_singleton_model(|_| KeybindingChangedNotifier::mock()); + app.add_singleton_model(|_| AuthStateProvider::new_for_test()); + app.add_singleton_model(CloudModel::mock); + app.add_singleton_model(|_| ActiveSession::default()); + app.add_singleton_model(NotebookKeybindings::new); + + let team_client_mock = Arc::new(MockTeamClient::new()); + let workspace_client_mock = Arc::new(MockWorkspaceClient::new()); + app.add_singleton_model(|ctx| { + UserWorkspaces::mock( + team_client_mock.clone(), + workspace_client_mock.clone(), + vec![], + ctx, + ) + }); + + let (_window, editor_view) = app.add_window(WindowStyle::NotStealFocus, |ctx| { + CodeEditorView::new( + None, + None, + CodeEditorRenderOptions::new(VerticalExpansionBehavior::InfiniteHeight), + ctx, + ) + }); + editor_view +} + +#[test] +fn apply_streamed_code_update_through_real_editor_appends_then_resets_on_non_prefix_rewrite() { + App::test((), |mut app| async move { + let editor = test_code_editor(&mut app); + + editor.update(&mut app, |view, ctx| { + apply_streamed_code_update(view, "abc", "", ctx); + }); + let text = editor.update(&mut app, |view, ctx| view.text(ctx).as_str().to_string()); + assert_eq!(text, "abc"); + + // Boundary-aligned, non-prefix rewrite on an *existing, populated* editor: + // must reset the real buffer to "XYZq", not corrupt it into "abcq". + editor.update(&mut app, |view, ctx| { + apply_streamed_code_update(view, "XYZq", "abc", ctx); + }); + let text = editor.update(&mut app, |view, ctx| view.text(ctx).as_str().to_string()); + assert_eq!(text, "XYZq"); + }); +} + +#[test] +fn apply_streamed_code_update_through_real_editor_truncates_on_valid_shrink() { + App::test((), |mut app| async move { + let editor = test_code_editor(&mut app); + editor.update(&mut app, |view, ctx| { + apply_streamed_code_update(view, "a += 12\n``", "", ctx); + }); + + editor.update(&mut app, |view, ctx| { + apply_streamed_code_update(view, "a += 12", "a += 12\n``", ctx); + }); + let text = editor.update(&mut app, |view, ctx| view.text(ctx).as_str().to_string()); + assert_eq!(text, "a += 12"); + }); +} + +#[test] +fn apply_streamed_code_update_through_real_editor_resets_on_non_prefix_shrink() { + App::test((), |mut app| async move { + let editor = test_code_editor(&mut app); + editor.update(&mut app, |view, ctx| { + apply_streamed_code_update(view, "abcdef", "", ctx); + }); + + editor.update(&mut app, |view, ctx| { + apply_streamed_code_update(view, "xyz", "abcdef", ctx); + }); + let text = editor.update(&mut app, |view, ctx| view.text(ctx).as_str().to_string()); + assert_eq!(text, "xyz"); + }); +} + #[cfg(feature = "local_fs")] #[test] fn open_code_action_routes_links_to_configured_editor_and_non_links_to_warp() { diff --git a/app/src/ai/blocklist/inline_action/requested_command.rs b/app/src/ai/blocklist/inline_action/requested_command.rs index a88fe83b41f..7d4b6393232 100644 --- a/app/src/ai/blocklist/inline_action/requested_command.rs +++ b/app/src/ai/blocklist/inline_action/requested_command.rs @@ -1,5 +1,5 @@ use std::borrow::Cow; -use std::cmp::{Ordering, PartialEq}; +use std::cmp::PartialEq; use std::collections::HashMap; use std::rc::Rc; use std::sync::Arc; @@ -45,7 +45,9 @@ use crate::ai::blocklist::block::view_impl::{ CONTENT_HORIZONTAL_PADDING, CONTENT_ITEM_VERTICAL_MARGIN, render_autonomy_checkbox_setting_speedbump_footer, render_citation, render_citation_chips, }; -use crate::ai::blocklist::block::{AIBlockAction, AutonomySettingSpeedbump}; +use crate::ai::blocklist::block::{ + AIBlockAction, AutonomySettingSpeedbump, StreamedCodeUpdate, streamed_code_update, +}; use crate::ai::blocklist::inline_action::inline_action_header::{ ExpandedConfig, HeaderConfig, INLINE_ACTION_HORIZONTAL_PADDING, InteractionMode, RightClickConfig, @@ -1048,40 +1050,25 @@ impl RequestedCommandView { /// This is to reduce flicker. /// /// If the command length is shorter than the previous update, then the command is truncated to the given byte length. + /// If `command` is not a content-stable append/truncate of the previous command text (e.g. a + /// non-prefix rewrite from the server, or a same-length correction), `command_text` -- and, if + /// an editor already exists, the editor -- are reset wholesale to `command` instead. pub fn apply_streamed_update(&mut self, command: &str, ctx: &mut ViewContext) { - apply_streamed_command_text(&mut self.command_text, command); + let update = streamed_code_update(command, &self.command_text); + match update { + StreamedCodeUpdate::Append(suffix) => self.command_text.push_str(suffix), + StreamedCodeUpdate::Truncate => self.command_text.truncate(command.len()), + StreamedCodeUpdate::Reset => self.command_text = command.to_string(), + StreamedCodeUpdate::NoOp => {} + } - // If the editor exists, sync it with the updated command text. + // If the editor exists, sync it with the same decision that was just applied to + // `command_text` above, rather than letting it independently re-derive an update by + // comparing lengths against its own current content (see `apply_streamed_command_editor_update`). if let Some(editor) = &self.editor { + let command_text = self.command_text.clone(); editor.update(ctx, |editor, ctx| { - let editor_length = editor.text(ctx).as_str().len(); - match self.command_text.len().cmp(&editor_length) { - Ordering::Greater => { - let slice_to_append = if self.command_text.is_char_boundary(editor_length) { - &self.command_text[editor_length..] - } else { - editor.truncate(0, ctx); - &self.command_text - }; - // TODO(Simon): The first insertion into an empty Buffer creates a trailing newline. - // If the requested command is streamed in in a single chunk, then there will - // be an extra newline rendered at the end of the `CodeEditorView`. This is likely - // caused by an initial insertion bug somewhere in the `Buffer` logic. - // - // To reproduce this bug, simply clear the buffer and type in two letters. You'll - // notice that a newline is created on the first letter, but removed on the second. - // The temporary workaround is to append an empty string to the end of each chunk, - // which acts as the second insertion that clears the trailing newline. - editor.system_append_autoscroll_vertical_only(slice_to_append, ctx); - editor.system_append_autoscroll_vertical_only("", ctx); - ctx.notify(); - } - Ordering::Less => { - editor.truncate(self.command_text.len(), ctx); - ctx.notify(); - } - Ordering::Equal => {} - } + apply_streamed_command_editor_update(editor, update, &command_text, ctx); }); } } @@ -2166,40 +2153,44 @@ impl RequestedCommand { } } -/// Applies a streamed update to `text` in place, given the newly received -/// `new_value`. -/// -/// Streaming is assumed to only change the end of the string: either -/// `new_value` extends `text` with a suffix, or `new_value` is `text` with a -/// few trailing bytes removed (e.g. a partially-received token being -/// trimmed). Under that assumption, `text`'s previous length is always a -/// valid split point in `new_value` (grow) or `text` itself (shrink). -/// However, when a streamed rewrite isn't a clean prefix-preserving update, -/// that byte offset can fall inside a multi-byte UTF-8 character. Naively -/// slicing or truncating at such an offset panics with "byte index is not a -/// char boundary", so both directions are guarded and fall back to replacing -/// `text` wholesale with `new_value` when the offset isn't a valid boundary. -/// -/// Extracted for unit testing. -fn apply_streamed_command_text(text: &mut String, new_value: &str) { - match new_value.len().cmp(&text.len()) { - Ordering::Greater => { - let existing_length = text.len(); - if new_value.is_char_boundary(existing_length) { - text.push_str(&new_value[existing_length..]); - } else { - *text = new_value.to_string(); - } +/// Applies a [`StreamedCodeUpdate`] decision -- already computed and applied +/// to `command_text` by the caller -- to the [`CodeEditorView`] that mirrors +/// it, so the two never diverge. Used by +/// [`RequestedCommandView::apply_streamed_update`] instead of having the +/// editor independently re-derive its own update by comparing lengths +/// against its current content: that approach has the same "length/boundary +/// lines up but content diverged" bug as `command_text` itself (APP-5288). +fn apply_streamed_command_editor_update( + editor: &CodeEditorView, + update: StreamedCodeUpdate, + command_text: &str, + ctx: &mut ViewContext, +) { + match update { + StreamedCodeUpdate::Append(suffix) => { + // TODO(Simon): The first insertion into an empty Buffer creates a trailing newline. + // If the requested command is streamed in in a single chunk, then there will + // be an extra newline rendered at the end of the `CodeEditorView`. This is likely + // caused by an initial insertion bug somewhere in the `Buffer` logic. + // + // To reproduce this bug, simply clear the buffer and type in two letters. You'll + // notice that a newline is created on the first letter, but removed on the second. + // The temporary workaround is to append an empty string to the end of each chunk, + // which acts as the second insertion that clears the trailing newline. + editor.system_append_autoscroll_vertical_only(suffix, ctx); + editor.system_append_autoscroll_vertical_only("", ctx); } - Ordering::Less => { - if text.is_char_boundary(new_value.len()) { - text.truncate(new_value.len()); - } else { - *text = new_value.to_string(); + StreamedCodeUpdate::Reset => { + editor.truncate(0, ctx); + if !command_text.is_empty() { + editor.system_append_autoscroll_vertical_only(command_text, ctx); + editor.system_append_autoscroll_vertical_only("", ctx); } } - Ordering::Equal => {} + StreamedCodeUpdate::Truncate => editor.truncate(command_text.len(), ctx), + StreamedCodeUpdate::NoOp => return, } + ctx.notify(); } /// Formats the command text to truncate at the first newline and add an ellipsis. diff --git a/app/src/ai/blocklist/inline_action/requested_command_tests.rs b/app/src/ai/blocklist/inline_action/requested_command_tests.rs index fdfb458816d..d2fcf767ed3 100644 --- a/app/src/ai/blocklist/inline_action/requested_command_tests.rs +++ b/app/src/ai/blocklist/inline_action/requested_command_tests.rs @@ -1,9 +1,28 @@ -//! Unit tests for format_command_text in requested_command.rs +//! Unit tests for format_command_text and the streamed-update helpers in requested_command.rs + +use std::sync::Arc; + +use warp_core::ui::appearance::Appearance; +use warp_editor::render::element::VerticalExpansionBehavior; +use warpui::platform::WindowStyle; +use warpui::{App, ViewHandle}; use super::{ - apply_streamed_command_text, format_command_text, mcp_blocked_title_text, - mcp_viewing_detail_title_text, + CodeEditorRenderOptions, CodeEditorView, StreamedCodeUpdate, + apply_streamed_command_editor_update, format_command_text, mcp_blocked_title_text, + mcp_viewing_detail_title_text, streamed_code_update, }; +use crate::AuthStateProvider; +use crate::cloud_object::model::persistence::CloudModel; +use crate::notebooks::editor::keys::NotebookKeybindings; +use crate::server::server_api::team::MockTeamClient; +use crate::server::server_api::workspace::MockWorkspaceClient; +use crate::settings_view::keybindings::KeybindingChangedNotifier; +use crate::test_util::settings::initialize_settings_for_tests; +use crate::vim_registers::VimRegisters; +use crate::workspace::ActiveSession; +use crate::workspace::sync_inputs::SyncedInputState; +use crate::workspaces::user_workspaces::UserWorkspaces; #[test] fn single_line_without_newline_is_unchanged_ascii() { @@ -74,50 +93,127 @@ fn newline_then_multibyte_results_in_ellipsis_only() { assert_eq!(reconstructed, output); } -#[test] -fn apply_streamed_command_text_appends_suffix_on_valid_char_boundary() { - let mut text = "echo ".to_string(); - apply_streamed_command_text(&mut text, "echo 你好"); - assert_eq!(text, "echo 你好"); +/// Constructs a bare `CodeEditorView`, mirroring +/// `ai::blocklist::block_tests::test_code_editor`. Used to exercise +/// `apply_streamed_command_editor_update` -- the exact function +/// `RequestedCommandView::apply_streamed_update` uses to keep its editor in +/// sync with `command_text` -- against a real buffer. +fn test_code_editor(app: &mut App) -> ViewHandle { + initialize_settings_for_tests(app); + app.add_singleton_model(|_| Appearance::mock()); + app.add_singleton_model(|_| SyncedInputState::mock()); + app.add_singleton_model(|_| VimRegisters::new()); + app.add_singleton_model(|_| KeybindingChangedNotifier::mock()); + app.add_singleton_model(|_| AuthStateProvider::new_for_test()); + app.add_singleton_model(CloudModel::mock); + app.add_singleton_model(|_| ActiveSession::default()); + app.add_singleton_model(NotebookKeybindings::new); + + let team_client_mock = Arc::new(MockTeamClient::new()); + let workspace_client_mock = Arc::new(MockWorkspaceClient::new()); + app.add_singleton_model(|ctx| { + UserWorkspaces::mock( + team_client_mock.clone(), + workspace_client_mock.clone(), + vec![], + ctx, + ) + }); + + let (_window, editor_view) = app.add_window(WindowStyle::NotStealFocus, |ctx| { + CodeEditorView::new( + None, + None, + CodeEditorRenderOptions::new(VerticalExpansionBehavior::GrowToMaxHeight), + ctx, + ) + }); + editor_view } #[test] -fn apply_streamed_command_text_resets_when_previous_length_splits_a_multibyte_char() { - // Regression test for APP-5288: the previously stored text has byte - // length 2, which falls in the middle of the 3-byte encoding of "你" in - // `new_value`. Slicing `new_value` at that offset directly would panic - // with "byte index is not a char boundary". This can happen when the - // server streams a non-prefix rewrite. - let mut text = "ab".to_string(); - let new_value = "你b"; - assert!(!new_value.is_char_boundary(text.len())); - apply_streamed_command_text(&mut text, new_value); - assert_eq!(text, new_value); +fn apply_streamed_command_editor_update_appends_suffix() { + App::test((), |mut app| async move { + let editor = test_code_editor(&mut app); + let update = streamed_code_update("echo hi", ""); + editor.update(&mut app, |editor, ctx| { + apply_streamed_command_editor_update(editor, update, "echo hi", ctx); + }); + let text = editor.update(&mut app, |editor, ctx| { + editor.text(ctx).as_str().to_string() + }); + assert_eq!(text, "echo hi"); + }); } #[test] -fn apply_streamed_command_text_shrinks_when_new_value_is_a_valid_prefix() { - let mut text = "cargo build\n``".to_string(); - apply_streamed_command_text(&mut text, "cargo build"); - assert_eq!(text, "cargo build"); +fn apply_streamed_command_editor_update_resets_existing_editor_on_non_prefix_rewrite() { + App::test((), |mut app| async move { + let editor = test_code_editor(&mut app); + let first_update = streamed_code_update("abc", ""); + editor.update(&mut app, |editor, ctx| { + apply_streamed_command_editor_update(editor, first_update, "abc", ctx); + }); + let text = editor.update(&mut app, |editor, ctx| { + editor.text(ctx).as_str().to_string() + }); + assert_eq!(text, "abc"); + + // Regression test for a follow-up finding on APP-5288: a boundary-aligned, + // non-prefix rewrite on an *already-populated* editor must reset it to + // "XYZq", not corrupt it into "abcq" the way a length-only sync would. + let second_update = streamed_code_update("XYZq", "abc"); + assert_eq!(second_update, StreamedCodeUpdate::Reset); + editor.update(&mut app, |editor, ctx| { + apply_streamed_command_editor_update(editor, second_update, "XYZq", ctx); + }); + let text = editor.update(&mut app, |editor, ctx| { + editor.text(ctx).as_str().to_string() + }); + assert_eq!(text, "XYZq"); + }); } #[test] -fn apply_streamed_command_text_resets_when_shrink_would_split_a_multibyte_char() { - // Regression test for APP-5288's secondary shrink-path bug: truncating - // `text` at `new_value.len()` must not split a multi-byte character in - // `text`. "你" is 3 bytes, so byte offset 1 is invalid. - let mut text = "你".to_string(); - let new_value = "a"; // 1 byte, shorter than `text`, but not a char boundary in "你". - apply_streamed_command_text(&mut text, new_value); - assert_eq!(text, new_value); +fn apply_streamed_command_editor_update_truncates_on_valid_shrink() { + App::test((), |mut app| async move { + let editor = test_code_editor(&mut app); + let first_update = streamed_code_update("cargo build\n``", ""); + editor.update(&mut app, |editor, ctx| { + apply_streamed_command_editor_update(editor, first_update, "cargo build\n``", ctx); + }); + + let second_update = streamed_code_update("cargo build", "cargo build\n``"); + assert_eq!(second_update, StreamedCodeUpdate::Truncate); + editor.update(&mut app, |editor, ctx| { + apply_streamed_command_editor_update(editor, second_update, "cargo build", ctx); + }); + let text = editor.update(&mut app, |editor, ctx| { + editor.text(ctx).as_str().to_string() + }); + assert_eq!(text, "cargo build"); + }); } #[test] -fn apply_streamed_command_text_is_noop_when_unchanged() { - let mut text = "echo hi".to_string(); - apply_streamed_command_text(&mut text, "echo hi"); - assert_eq!(text, "echo hi"); +fn apply_streamed_command_editor_update_is_noop_when_unchanged() { + App::test((), |mut app| async move { + let editor = test_code_editor(&mut app); + let first_update = streamed_code_update("echo hi", ""); + editor.update(&mut app, |editor, ctx| { + apply_streamed_command_editor_update(editor, first_update, "echo hi", ctx); + }); + + let second_update = streamed_code_update("echo hi", "echo hi"); + assert_eq!(second_update, StreamedCodeUpdate::NoOp); + editor.update(&mut app, |editor, ctx| { + apply_streamed_command_editor_update(editor, second_update, "echo hi", ctx); + }); + let text = editor.update(&mut app, |editor, ctx| { + editor.text(ctx).as_str().to_string() + }); + assert_eq!(text, "echo hi"); + }); } #[test]