Clean up formatting code and add testing for formatting with multiple formatters (including code actions!) (#28457)

Release Notes:

- N/A

---------

Co-authored-by: Max Brunsfeld <maxbrunsfeld@gmail.com>

crates/editor/src/editor_tests.rs

@@ -7756,13 +7756,9 @@ async fn test_document_format_during_save(cx: &mut TestAppContext) {
     cx.executor().start_waiting();
     let fake_server = fake_servers.next().await.unwrap();
 
-    let save = editor
+    {
-        .update_in(cx, |editor, window, cx| {
+        fake_server.set_request_handler::<lsp::request::Formatting, _, _>(
-            editor.save(true, project.clone(), window, cx)
+            move |params, _| async move {
-        })
-        .unwrap();
-    fake_server
-        .set_request_handler::<lsp::request::Formatting, _, _>(move |params, _| async move {
                 assert_eq!(
                     params.text_document.uri,
                     lsp::Url::from_file_path(path!("/file.rs")).unwrap()
@@ -7772,9 +7768,13 @@ async fn test_document_format_during_save(cx: &mut TestAppContext) {
                     lsp::Range::new(lsp::Position::new(0, 3), lsp::Position::new(1, 0)),
                     ", ".to_string(),
                 )]))
+            },
+        );
+        let save = editor
+            .update_in(cx, |editor, window, cx| {
+                editor.save(true, project.clone(), window, cx)
             })
-        .next()
+            .unwrap();
-        .await;
         cx.executor().start_waiting();
         save.await;
 
@@ -7783,7 +7783,9 @@ async fn test_document_format_during_save(cx: &mut TestAppContext) {
             "one, two\nthree\n"
         );
         assert!(!cx.read(|cx| editor.is_dirty(cx)));
+    }
 
+    {
         editor.update_in(cx, |editor, window, cx| {
             editor.set_text("one\ntwo\nthree\n", window, cx)
         });
@@ -7812,21 +7814,23 @@ async fn test_document_format_during_save(cx: &mut TestAppContext) {
             editor.update(cx, |editor, cx| editor.text(cx)),
             "one\ntwo\nthree\n"
         );
-    assert!(!cx.read(|cx| editor.is_dirty(cx)));
+    }
 
     // For non-dirty buffer, no formatting request should be sent
+    {
+        assert!(!cx.read(|cx| editor.is_dirty(cx)));
+
+        fake_server.set_request_handler::<lsp::request::Formatting, _, _>(move |_, _| async move {
+            panic!("Should not be invoked on non-dirty buffer");
+        });
         let save = editor
             .update_in(cx, |editor, window, cx| {
                 editor.save(true, project.clone(), window, cx)
             })
             .unwrap();
-    let _pending_format_request = fake_server
-        .set_request_handler::<lsp::request::RangeFormatting, _, _>(move |_, _| async move {
-            panic!("Should not be invoked on non-dirty buffer");
-        })
-        .next();
         cx.executor().start_waiting();
         save.await;
+    }
 
     // Set rust language override and assert overridden tabsize is sent to language server
     update_test_language_settings(cx, |settings| {
@@ -7839,16 +7843,12 @@ async fn test_document_format_during_save(cx: &mut TestAppContext) {
         );
     });
 
+    {
         editor.update_in(cx, |editor, window, cx| {
             editor.set_text("somehting_new\n", window, cx)
         });
         assert!(cx.read(|cx| editor.is_dirty(cx)));
-    let save = editor
+        let _formatting_request_signal = fake_server
-        .update_in(cx, |editor, window, cx| {
-            editor.save(true, project.clone(), window, cx)
-        })
-        .unwrap();
-    fake_server
             .set_request_handler::<lsp::request::Formatting, _, _>(move |params, _| async move {
                 assert_eq!(
                     params.text_document.uri,
@@ -7856,12 +7856,16 @@ async fn test_document_format_during_save(cx: &mut TestAppContext) {
                 );
                 assert_eq!(params.options.tab_size, 8);
                 Ok(Some(vec![]))
+            });
+        let save = editor
+            .update_in(cx, |editor, window, cx| {
+                editor.save(true, project.clone(), window, cx)
             })
-        .next()
+            .unwrap();
-        .await;
         cx.executor().start_waiting();
         save.await;
     }
+}
 
 #[gpui::test]
 async fn test_multibuffer_format_during_save(cx: &mut TestAppContext) {
@@ -8342,6 +8346,272 @@ async fn test_document_format_manual_trigger(cx: &mut TestAppContext) {
     );
 }
 
+#[gpui::test]
+async fn test_multiple_formatters(cx: &mut TestAppContext) {
+    init_test(cx, |settings| {
+        settings.defaults.remove_trailing_whitespace_on_save = Some(true);
+        settings.defaults.formatter =
+            Some(language_settings::SelectedFormatter::List(FormatterList(
+                vec![
+                    Formatter::LanguageServer { name: None },
+                    Formatter::CodeActions(
+                        [
+                            ("code-action-1".into(), true),
+                            ("code-action-2".into(), true),
+                        ]
+                        .into_iter()
+                        .collect(),
+                    ),
+                ]
+                .into(),
+            )))
+    });
+
+    let fs = FakeFs::new(cx.executor());
+    fs.insert_file(path!("/file.rs"), "one  \ntwo   \nthree".into())
+        .await;
+
+    let project = Project::test(fs, [path!("/").as_ref()], cx).await;
+    let language_registry = project.read_with(cx, |project, _| project.languages().clone());
+    language_registry.add(rust_lang());
+
+    let mut fake_servers = language_registry.register_fake_lsp(
+        "Rust",
+        FakeLspAdapter {
+            capabilities: lsp::ServerCapabilities {
+                document_formatting_provider: Some(lsp::OneOf::Left(true)),
+                execute_command_provider: Some(lsp::ExecuteCommandOptions {
+                    commands: vec!["the-command-for-code-action-1".into()],
+                    ..Default::default()
+                }),
+                code_action_provider: Some(lsp::CodeActionProviderCapability::Simple(true)),
+                ..Default::default()
+            },
+            ..Default::default()
+        },
+    );
+
+    let buffer = project
+        .update(cx, |project, cx| {
+            project.open_local_buffer(path!("/file.rs"), cx)
+        })
+        .await
+        .unwrap();
+
+    let buffer = cx.new(|cx| MultiBuffer::singleton(buffer, cx));
+    let (editor, cx) = cx.add_window_view(|window, cx| {
+        build_editor_with_project(project.clone(), buffer, window, cx)
+    });
+
+    cx.executor().start_waiting();
+
+    let fake_server = fake_servers.next().await.unwrap();
+    fake_server.set_request_handler::<lsp::request::Formatting, _, _>(
+        move |_params, _| async move {
+            Ok(Some(vec![lsp::TextEdit::new(
+                lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)),
+                "applied-formatting\n".to_string(),
+            )]))
+        },
+    );
+    fake_server.set_request_handler::<lsp::request::CodeActionRequest, _, _>(
+        move |params, _| async move {
+            assert_eq!(
+                params.context.only,
+                Some(vec!["code-action-1".into(), "code-action-2".into()])
+            );
+            let uri = lsp::Url::from_file_path(path!("/file.rs")).unwrap();
+            Ok(Some(vec![
+                lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction {
+                    kind: Some("code-action-1".into()),
+                    edit: Some(lsp::WorkspaceEdit::new(
+                        [(
+                            uri.clone(),
+                            vec![lsp::TextEdit::new(
+                                lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)),
+                                "applied-code-action-1-edit\n".to_string(),
+                            )],
+                        )]
+                        .into_iter()
+                        .collect(),
+                    )),
+                    command: Some(lsp::Command {
+                        command: "the-command-for-code-action-1".into(),
+                        ..Default::default()
+                    }),
+                    ..Default::default()
+                }),
+                lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction {
+                    kind: Some("code-action-2".into()),
+                    edit: Some(lsp::WorkspaceEdit::new(
+                        [(
+                            uri.clone(),
+                            vec![lsp::TextEdit::new(
+                                lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 0)),
+                                "applied-code-action-2-edit\n".to_string(),
+                            )],
+                        )]
+                        .into_iter()
+                        .collect(),
+                    )),
+                    ..Default::default()
+                }),
+            ]))
+        },
+    );
+
+    fake_server.set_request_handler::<lsp::request::CodeActionResolveRequest, _, _>({
+        move |params, _| async move { Ok(params) }
+    });
+
+    let command_lock = Arc::new(futures::lock::Mutex::new(()));
+    fake_server.set_request_handler::<lsp::request::ExecuteCommand, _, _>({
+        let fake = fake_server.clone();
+        let lock = command_lock.clone();
+        move |params, _| {
+            assert_eq!(params.command, "the-command-for-code-action-1");
+            let fake = fake.clone();
+            let lock = lock.clone();
+            async move {
+                lock.lock().await;
+                fake.server
+                    .request::<lsp::request::ApplyWorkspaceEdit>(lsp::ApplyWorkspaceEditParams {
+                        label: None,
+                        edit: lsp::WorkspaceEdit {
+                            changes: Some(
+                                [(
+                                    lsp::Url::from_file_path(path!("/file.rs")).unwrap(),
+                                    vec![lsp::TextEdit {
+                                        range: lsp::Range::new(
+                                            lsp::Position::new(0, 0),
+                                            lsp::Position::new(0, 0),
+                                        ),
+                                        new_text: "applied-code-action-1-command\n".into(),
+                                    }],
+                                )]
+                                .into_iter()
+                                .collect(),
+                            ),
+                            ..Default::default()
+                        },
+                    })
+                    .await
+                    .unwrap();
+                Ok(Some(json!(null)))
+            }
+        }
+    });
+
+    cx.executor().start_waiting();
+    editor
+        .update_in(cx, |editor, window, cx| {
+            editor.perform_format(
+                project.clone(),
+                FormatTrigger::Manual,
+                FormatTarget::Buffers,
+                window,
+                cx,
+            )
+        })
+        .unwrap()
+        .await;
+    editor.update(cx, |editor, cx| {
+        assert_eq!(
+            editor.text(cx),
+            r#"
+                applied-code-action-2-edit
+                applied-code-action-1-command
+                applied-code-action-1-edit
+                applied-formatting
+                one
+                two
+                three
+            "#
+            .unindent()
+        );
+    });
+
+    editor.update_in(cx, |editor, window, cx| {
+        editor.undo(&Default::default(), window, cx);
+        assert_eq!(editor.text(cx), "one  \ntwo   \nthree");
+    });
+
+    // Perform a manual edit while waiting for an LSP command
+    // that's being run as part of a formatting code action.
+    let lock_guard = command_lock.lock().await;
+    let format = editor
+        .update_in(cx, |editor, window, cx| {
+            editor.perform_format(
+                project.clone(),
+                FormatTrigger::Manual,
+                FormatTarget::Buffers,
+                window,
+                cx,
+            )
+        })
+        .unwrap();
+    cx.run_until_parked();
+    editor.update(cx, |editor, cx| {
+        assert_eq!(
+            editor.text(cx),
+            r#"
+                applied-code-action-1-edit
+                applied-formatting
+                one
+                two
+                three
+            "#
+            .unindent()
+        );
+
+        editor.buffer.update(cx, |buffer, cx| {
+            let ix = buffer.len(cx);
+            buffer.edit([(ix..ix, "edited\n")], None, cx);
+        });
+    });
+
+    // Allow the LSP command to proceed. Because the buffer was edited,
+    // the second code action will not be run.
+    drop(lock_guard);
+    format.await;
+    editor.update_in(cx, |editor, window, cx| {
+        assert_eq!(
+            editor.text(cx),
+            r#"
+                applied-code-action-1-command
+                applied-code-action-1-edit
+                applied-formatting
+                one
+                two
+                three
+                edited
+            "#
+            .unindent()
+        );
+
+        // The manual edit is undone first, because it is the last thing the user did
+        // (even though the command completed afterwards).
+        editor.undo(&Default::default(), window, cx);
+        assert_eq!(
+            editor.text(cx),
+            r#"
+                applied-code-action-1-command
+                applied-code-action-1-edit
+                applied-formatting
+                one
+                two
+                three
+            "#
+            .unindent()
+        );
+
+        // All the formatting (including the command, which completed after the manual edit)
+        // is undone together.
+        editor.undo(&Default::default(), window, cx);
+        assert_eq!(editor.text(cx), "one  \ntwo   \nthree");
+    });
+}
+
 #[gpui::test]
 async fn test_organize_imports_manual_trigger(cx: &mut TestAppContext) {
     init_test(cx, |settings| {

crates/project/src/lsp_store.rs

@@ -1082,17 +1082,6 @@ impl LocalLspStore {
             })
     }
 
-    fn primary_language_server_for_buffer<'a>(
-        &'a self,
-        buffer: &'a Buffer,
-        cx: &'a mut App,
-    ) -> Option<(&'a Arc<CachedLspAdapter>, &'a Arc<LanguageServer>)> {
-        // The list of language servers is ordered based on the `language_servers` setting
-        // for each language, thus we can consider the first one in the list to be the
-        // primary one.
-        self.language_servers_for_buffer(buffer, cx).next()
-    }
-
     async fn execute_code_action_kind_locally(
         lsp_store: WeakEntity<LspStore>,
         mut buffers: Vec<Entity<Buffer>>,
@@ -1198,139 +1187,144 @@ impl LocalLspStore {
         let mut project_transaction = ProjectTransaction::default();
 
         for buffer in &buffers {
-            let adapters_and_servers = lsp_store.update(cx, |lsp_store, cx| {
+            // Create an empty transaction to hold all of the formatting edits.
+            let formatting_transaction_id = buffer.handle.update(cx, |buffer, cx| {
+                // ensure no transactions created while formatting are
+                // grouped with the previous transaction in the history
+                // based on the transaction group interval
+                buffer.finalize_last_transaction();
+                let transaction_id = buffer
+                    .start_transaction()
+                    .ok_or_else(|| anyhow!("transaction already open"))?;
+                let transaction = buffer
+                    .get_transaction(transaction_id)
+                    .expect("transaction started")
+                    .clone();
+                buffer.end_transaction(cx);
+                buffer.push_transaction(transaction, cx.background_executor().now());
+                buffer.finalize_last_transaction();
+                anyhow::Ok(transaction_id)
+            })??;
+
+            let result = Self::format_buffer_locally(
+                lsp_store.clone(),
+                buffer,
+                formatting_transaction_id,
+                trigger,
+                logger,
+                cx,
+            )
+            .await;
+
             buffer.handle.update(cx, |buffer, cx| {
-                    lsp_store
+                let Some(formatting_transaction) =
+                    buffer.get_transaction(formatting_transaction_id).cloned()
+                else {
+                    zlog::warn!(logger => "no formatting transaction");
+                    return;
+                };
+                if formatting_transaction.edit_ids.is_empty() {
+                    buffer.forget_transaction(formatting_transaction_id);
+                    return;
+                }
+                if !push_to_history {
+                    zlog::trace!(logger => "forgetting format transaction");
+                    buffer.forget_transaction(formatting_transaction.id);
+                }
+                project_transaction
+                    .0
+                    .insert(cx.entity(), formatting_transaction);
+            })?;
+
+            result?;
+        }
+
+        Ok(project_transaction)
+    }
+
+    async fn format_buffer_locally(
+        lsp_store: WeakEntity<LspStore>,
+        buffer: &FormattableBuffer,
+        formatting_transaction_id: clock::Lamport,
+        trigger: FormatTrigger,
+        logger: zlog::Logger,
+        cx: &mut AsyncApp,
+    ) -> Result<()> {
+        let (adapters_and_servers, settings) = lsp_store.update(cx, |lsp_store, cx| {
+            buffer.handle.update(cx, |buffer, cx| {
+                let adapters_and_servers = lsp_store
                     .as_local()
                     .unwrap()
                     .language_servers_for_buffer(buffer, cx)
                     .map(|(adapter, lsp)| (adapter.clone(), lsp.clone()))
-                        .collect::<Vec<_>>()
+                    .collect::<Vec<_>>();
-                })
+                let settings =
-            })?;
-
-            let settings = buffer.handle.read_with(cx, |buffer, cx| {
                     language_settings(buffer.language().map(|l| l.name()), buffer.file(), cx)
-                    .into_owned()
+                        .into_owned();
-            })?;
+                (adapters_and_servers, settings)
-
-            let mut transaction_id_format = None;
-
-            // ensure no transactions created while formatting are
-            // grouped with the previous transaction in the history
-            // based on the transaction group interval
-            buffer.handle.update(cx, |buffer, _| {
-                buffer.finalize_last_transaction();
-            })?;
-
-            // helper function to avoid duplicate logic between formatter handlers below
-            // We want to avoid continuing to format the buffer if it has been edited since the start
-            // so we check that the last transaction id on the undo stack matches the one we expect
-            // This check should be done after each "gather" step where we generate a diff or edits to apply,
-            // and before applying them to the buffer to avoid messing up the user's buffer
-            fn err_if_buffer_edited_since_start(
-                buffer: &FormattableBuffer,
-                transaction_id_format: Option<text::TransactionId>,
-                cx: &AsyncApp,
-            ) -> Option<anyhow::Error> {
-                let transaction_id_last = buffer
-                    .handle
-                    .read_with(cx, |buffer, _| {
-                        buffer.peek_undo_stack().map(|t| t.transaction_id())
             })
-                    .ok()
-                    .flatten();
-                let should_continue_formatting = match (transaction_id_format, transaction_id_last)
-                {
-                    (Some(format), Some(last)) => format == last,
-                    (Some(_), None) => false,
-                    (_, _) => true,
-                };
-                if !should_continue_formatting {
-                    return Some(anyhow::anyhow!("Buffer edited while formatting. Aborting"));
-                }
-                return None;
-            }
-
-            // variable used to track errors that occur during the formatting process below,
-            // but that need to not be returned right away (with `?` for example) because we
-            // still need to clean up the transaction history and update the project transaction
-            // at the very end
-            let mut result = anyhow::Ok(());
-
-            // see handling of code action formatting for why there might already be transactions
-            // in project_transaction for this buffer
-            if let Some(transaction_existing) = project_transaction.0.remove(&buffer.handle) {
-                transaction_id_format = Some(transaction_existing.id);
-                buffer.handle.update(cx, |buffer, _| {
-                    // ensure the transaction is in the history so we can group with it
-                    if buffer.get_transaction(transaction_existing.id).is_none() {
-                        buffer.push_transaction(transaction_existing, Instant::now());
-                    }
         })?;
-            }
 
-            if let Some(err) = err_if_buffer_edited_since_start(buffer, transaction_id_format, &cx)
+        // Apply edits to the buffer that will become part of the formatting transaction.
-            {
+        // Fails if the buffer has been edited since the start of that transaction.
-                zlog::warn!(logger => "Buffer edited while formatting. Aborting");
+        fn extend_formatting_transaction(
-                result = Err(err);
+            buffer: &FormattableBuffer,
+            formatting_transaction_id: text::TransactionId,
+            cx: &mut AsyncApp,
+            operation: impl FnOnce(&mut Buffer, &mut Context<Buffer>),
+        ) -> anyhow::Result<()> {
+            buffer.handle.update(cx, |buffer, cx| {
+                let last_transaction_id = buffer.peek_undo_stack().map(|t| t.transaction_id());
+                if last_transaction_id != Some(formatting_transaction_id) {
+                    anyhow::bail!("Buffer edited while formatting. Aborting")
+                }
+                buffer.start_transaction();
+                operation(buffer, cx);
+                if let Some(transaction_id) = buffer.end_transaction(cx) {
+                    buffer.merge_transactions(transaction_id, formatting_transaction_id);
+                }
+                Ok(())
+            })?
         }
 
         // handle whitespace formatting
-            if result.is_ok() {
         if settings.remove_trailing_whitespace_on_save {
             zlog::trace!(logger => "removing trailing whitespace");
             let diff = buffer
                 .handle
                 .read_with(cx, |buffer, cx| buffer.remove_trailing_whitespace(cx))?
                 .await;
-                    buffer.handle.update(cx, |buffer, cx| {
+            extend_formatting_transaction(buffer, formatting_transaction_id, cx, |buffer, cx| {
-                        buffer.start_transaction();
                 buffer.apply_diff(diff, cx);
-                        transaction_id_format =
-                            transaction_id_format.or(buffer.end_transaction(cx));
-                        if let Some(transaction_id) = transaction_id_format {
-                            buffer.group_until_transaction(transaction_id);
-                        }
             })?;
         }
 
         if settings.ensure_final_newline_on_save {
             zlog::trace!(logger => "ensuring final newline");
-                    buffer.handle.update(cx, |buffer, cx| {
+            extend_formatting_transaction(buffer, formatting_transaction_id, cx, |buffer, cx| {
-                        buffer.start_transaction();
                 buffer.ensure_final_newline(cx);
-                        transaction_id_format =
-                            transaction_id_format.or(buffer.end_transaction(cx));
-                        if let Some(transaction_id) = transaction_id_format {
-                            buffer.group_until_transaction(transaction_id);
-                        }
             })?;
         }
-            }
 
         // Formatter for `code_actions_on_format` that runs before
         // the rest of the formatters
-            let code_actions_on_format_formatter = 'ca_formatter: {
+        let mut code_actions_on_format_formatter = None;
         let should_run_code_actions_on_format = !matches!(
             (trigger, &settings.format_on_save),
             (FormatTrigger::Save, &FormatOnSave::Off)
         );
+        if should_run_code_actions_on_format {
             let have_code_actions_to_run_on_format = settings
                 .code_actions_on_format
                 .values()
                 .any(|enabled| *enabled);
-
-                if should_run_code_actions_on_format {
             if have_code_actions_to_run_on_format {
                 zlog::trace!(logger => "going to run code actions on format");
-                        break 'ca_formatter Some(Formatter::CodeActions(
+                code_actions_on_format_formatter = Some(Formatter::CodeActions(
                     settings.code_actions_on_format.clone(),
                 ));
             }
         }
-                break 'ca_formatter None;
-            };
 
         let formatters = match (trigger, &settings.format_on_save) {
             (FormatTrigger::Save, FormatOnSave::Off) => &[],
@@ -1353,12 +1347,7 @@ impl LocalLspStore {
 
         let formatters = code_actions_on_format_formatter.iter().chain(formatters);
 
-            'formatters: for formatter in formatters {
+        for formatter in formatters {
-                if result.is_err() {
-                    // may have been set above, instead of indenting this whole block with an if, just don't do anything
-                    // destructive if we're aborting
-                    continue;
-                }
             match formatter {
                 Formatter::Prettier => {
                     let logger = zlog::scoped!(logger => "prettier");
@@ -1368,44 +1357,29 @@ impl LocalLspStore {
                     let prettier = lsp_store.read_with(cx, |lsp_store, _cx| {
                         lsp_store.prettier_store().unwrap().downgrade()
                     })?;
-                        let diff_result =
+                    let diff = prettier_store::format_with_prettier(&prettier, &buffer.handle, cx)
-                            prettier_store::format_with_prettier(&prettier, &buffer.handle, cx)
                         .await
-                                .transpose();
+                        .transpose()?;
-                        let Ok(diff) = diff_result else {
-                            result = Err(diff_result.unwrap_err());
-                            zlog::error!(logger => "failed, reason: {:?}", result.as_ref());
-                            break 'formatters;
-                        };
                     let Some(diff) = diff else {
                         zlog::trace!(logger => "No changes");
-                            continue 'formatters;
+                        continue;
                     };
-                        if let Some(err) =
+
-                            err_if_buffer_edited_since_start(buffer, transaction_id_format, &cx)
+                    extend_formatting_transaction(
-                        {
+                        buffer,
-                            zlog::warn!(logger => "Buffer edited while formatting. Aborting");
+                        formatting_transaction_id,
-                            result = Err(err);
+                        cx,
-                            break 'formatters;
+                        |buffer, cx| {
-                        }
-                        zlog::trace!(logger => "Applying changes");
-                        buffer.handle.update(cx, |buffer, cx| {
-                            buffer.start_transaction();
                             buffer.apply_diff(diff, cx);
-                            transaction_id_format =
+                        },
-                                transaction_id_format.or(buffer.end_transaction(cx));
+                    )?;
-                            if let Some(transaction_id) = transaction_id_format {
-                                buffer.group_until_transaction(transaction_id);
-                            }
-                        })?;
                 }
                 Formatter::External { command, arguments } => {
                     let logger = zlog::scoped!(logger => "command");
                     zlog::trace!(logger => "formatting");
-                        let _timer =
+                    let _timer = zlog::time!(logger => "Formatting buffer via external command");
-                            zlog::time!(logger => "Formatting buffer via external command");
 
-                        let diff_result = Self::format_via_external_command(
+                    let diff = Self::format_via_external_command(
                         buffer,
                         command.as_ref(),
                         arguments.as_deref(),
@@ -1414,75 +1388,49 @@ impl LocalLspStore {
                     .await
                     .with_context(|| {
                         format!("Failed to format buffer via external command: {}", command)
-                        });
+                    })?;
-                        let Ok(diff) = diff_result else {
-                            result = Err(diff_result.unwrap_err());
-                            zlog::error!(logger => "failed, reason: {:?}", result.as_ref());
-                            break 'formatters;
-                        };
                     let Some(diff) = diff else {
                         zlog::trace!(logger => "No changes");
-                            continue 'formatters;
+                        continue;
                     };
-                        if let Some(err) =
+
-                            err_if_buffer_edited_since_start(buffer, transaction_id_format, &cx)
+                    extend_formatting_transaction(
-                        {
+                        buffer,
-                            zlog::warn!(logger => "Buffer edited while formatting. Aborting");
+                        formatting_transaction_id,
-                            result = Err(err);
+                        cx,
-                            break 'formatters;
+                        |buffer, cx| {
-                        }
-                        zlog::trace!(logger => "Applying changes");
-                        buffer.handle.update(cx, |buffer, cx| {
-                            buffer.start_transaction();
                             buffer.apply_diff(diff, cx);
-                            transaction_id_format =
+                        },
-                                transaction_id_format.or(buffer.end_transaction(cx));
+                    )?;
-                            if let Some(transaction_id) = transaction_id_format {
-                                buffer.group_until_transaction(transaction_id);
-                            }
-                        })?;
                 }
                 Formatter::LanguageServer { name } => {
                     let logger = zlog::scoped!(logger => "language-server");
                     zlog::trace!(logger => "formatting");
-                        let _timer =
+                    let _timer = zlog::time!(logger => "Formatting buffer using language server");
-                            zlog::time!(logger => "Formatting buffer using language server");
 
                     let Some(buffer_path_abs) = buffer.abs_path.as_ref() else {
                         zlog::warn!(logger => "Cannot format buffer that is not backed by a file on disk using language servers. Skipping");
-                            continue 'formatters;
+                        continue;
                     };
 
-                        let language_server = 'language_server: {
+                    let language_server = if let Some(name) = name.as_deref() {
-                            // if a name was provided, try to find the server with that name
+                        adapters_and_servers.iter().find_map(|(adapter, server)| {
-                            if let Some(name) = name.as_deref() {
-                                for (adapter, server) in &adapters_and_servers {
                             if adapter.name.0.as_ref() == name {
-                                        break 'language_server server.clone();
+                                Some(server.clone())
-                                    }
-                                }
-                            }
-
-                            // otherwise, fall back to the primary language server for the buffer if one exists
-                            let default_lsp = lsp_store.update(cx, |lsp_store, cx| {
-                                buffer.handle.update(cx, |buffer, cx| {
-                                    lsp_store
-                                        .as_local()
-                                        .unwrap()
-                                        .primary_language_server_for_buffer(buffer, cx)
-                                        .map(|(_, lsp)| lsp.clone())
-                                })
-                            })?;
-
-                            if let Some(default_lsp) = default_lsp {
-                                break 'language_server default_lsp;
                             } else {
+                                None
+                            }
+                        })
+                    } else {
+                        adapters_and_servers.first().map(|e| e.1.clone())
+                    };
+
+                    let Some(language_server) = language_server else {
                         log::warn!(
                             "No language server found to format buffer '{:?}'. Skipping",
                             buffer_path_abs.as_path().to_string_lossy()
                         );
-                                continue 'formatters;
+                        continue;
-                            }
                     };
 
                     zlog::trace!(
@@ -1492,7 +1440,7 @@ impl LocalLspStore {
                         language_server.name()
                     );
 
-                        let edits_result = if let Some(ranges) = buffer.ranges.as_ref() {
+                    let edits = if let Some(ranges) = buffer.ranges.as_ref() {
                         zlog::trace!(logger => "formatting ranges");
                         Self::format_ranges_via_lsp(
                             &lsp_store,
@@ -1504,7 +1452,7 @@ impl LocalLspStore {
                             cx,
                         )
                         .await
-                            .context("Failed to format ranges via language server")
+                        .context("Failed to format ranges via language server")?
                     } else {
                         zlog::trace!(logger => "formatting full");
                         Self::format_via_lsp(
@@ -1516,36 +1464,21 @@ impl LocalLspStore {
                             cx,
                         )
                         .await
-                            .context("failed to format via language server")
+                        .context("failed to format via language server")?
-                        };
-
-                        let Ok(edits) = edits_result else {
-                            result = Err(edits_result.unwrap_err());
-                            zlog::error!(logger => "Failed, reason: {:?}", result.as_ref());
-                            break 'formatters;
                     };
 
                     if edits.is_empty() {
                         zlog::trace!(logger => "No changes");
-                            continue 'formatters;
+                        continue;
                     }
-                        if let Some(err) =
+                    extend_formatting_transaction(
-                            err_if_buffer_edited_since_start(buffer, transaction_id_format, &cx)
+                        buffer,
-                        {
+                        formatting_transaction_id,
-                            zlog::warn!(logger => "Buffer edited while formatting. Aborting");
+                        cx,
-                            result = Err(err);
+                        |buffer, cx| {
-                            break 'formatters;
-                        }
-                        zlog::trace!(logger => "Applying changes");
-                        buffer.handle.update(cx, |buffer, cx| {
-                            buffer.start_transaction();
                             buffer.edit(edits, None, cx);
-                            transaction_id_format =
+                        },
-                                transaction_id_format.or(buffer.end_transaction(cx));
+                    )?;
-                            if let Some(transaction_id) = transaction_id_format {
-                                buffer.group_until_transaction(transaction_id);
-                            }
-                        })?;
                 }
                 Formatter::CodeActions(code_actions) => {
                     let logger = zlog::scoped!(logger => "code-actions");
@@ -1554,7 +1487,7 @@ impl LocalLspStore {
 
                     let Some(buffer_path_abs) = buffer.abs_path.as_ref() else {
                         zlog::warn!(logger => "Cannot format buffer that is not backed by a file on disk using code actions. Skipping");
-                            continue 'formatters;
+                        continue;
                     };
                     let code_action_kinds = code_actions
                         .iter()
@@ -1564,13 +1497,13 @@ impl LocalLspStore {
                         .collect::<Vec<_>>();
                     if code_action_kinds.is_empty() {
                         zlog::trace!(logger => "No code action kinds enabled, skipping");
-                            continue 'formatters;
+                        continue;
                     }
+                    zlog::trace!(logger => "Attempting to resolve code actions {:?}", &code_action_kinds);
 
                     let mut actions_and_servers = Vec::new();
 
-                        for (index, (_, language_server)) in adapters_and_servers.iter().enumerate()
+                    for (index, (_, language_server)) in adapters_and_servers.iter().enumerate() {
-                        {
                         let actions_result = Self::get_server_code_actions_from_action_kinds(
                             &lsp_store,
                             language_server.server_id(),
@@ -1601,8 +1534,8 @@ impl LocalLspStore {
                     }
 
                     if actions_and_servers.is_empty() {
-                            zlog::trace!(logger => "No code actions were resolved, continuing");
+                        zlog::warn!(logger => "No code actions were resolved, continuing");
-                            continue 'formatters;
+                        continue;
                     }
 
                     'actions: for (mut action, server_index) in actions_and_servers {
@@ -1623,16 +1556,14 @@ impl LocalLspStore {
 
                         zlog::trace!(logger => "Executing {}", describe_code_action(&action));
 
-                            if let Err(err) =
+                        if let Err(err) = Self::try_resolve_code_action(server, &mut action).await {
-                                Self::try_resolve_code_action(server, &mut action).await
-                            {
                             zlog::error!(
                                 logger =>
                                 "Failed to resolve {}. Error: {}",
                                 describe_code_action(&action),
                                 err
                             );
-                                continue 'actions;
+                            continue;
                         }
 
                         if let Some(edit) = action.lsp_action.edit().cloned() {
@@ -1655,16 +1586,14 @@ impl LocalLspStore {
                                     "No changes for code action. Skipping {}",
                                     describe_code_action(&action),
                                 );
-                                    continue 'actions;
+                                continue;
                             }
 
                             let mut operations = Vec::new();
                             if let Some(document_changes) = edit.document_changes {
                                 match document_changes {
                                     lsp::DocumentChanges::Edits(edits) => operations.extend(
-                                            edits
+                                        edits.into_iter().map(lsp::DocumentChangeOperation::Edit),
-                                                .into_iter()
-                                                .map(lsp::DocumentChangeOperation::Edit),
                                     ),
                                     lsp::DocumentChanges::Operations(ops) => operations = ops,
                                 }
@@ -1689,7 +1618,7 @@ impl LocalLspStore {
                                     "No changes for code action. Skipping {}",
                                     describe_code_action(&action),
                                 );
-                                    continue 'actions;
+                                continue;
                             }
                             for operation in operations {
                                 let op = match operation {
@@ -1771,37 +1700,27 @@ impl LocalLspStore {
 
                             if edits.is_empty() {
                                 zlog::warn!(logger => "No edits resolved from LSP");
-                                    continue 'actions;
+                                continue;
                             }
 
-                                if let Some(err) = err_if_buffer_edited_since_start(
+                            extend_formatting_transaction(
                                 buffer,
-                                    transaction_id_format,
+                                formatting_transaction_id,
-                                    &cx,
+                                cx,
-                                ) {
+                                |buffer, cx| {
-                                    zlog::warn!(logger => "Buffer edited while formatting. Aborting");
-                                    result = Err(err);
-                                    break 'formatters;
-                                }
-                                zlog::info!(logger => "Applying changes");
-                                buffer.handle.update(cx, |buffer, cx| {
-                                    buffer.start_transaction();
                                     buffer.edit(edits, None, cx);
-                                    transaction_id_format =
+                                },
-                                        transaction_id_format.or(buffer.end_transaction(cx));
+                            )?;
-                                    if let Some(transaction_id) = transaction_id_format {
-                                        buffer.group_until_transaction(transaction_id);
-                                    }
-                                })?;
                         }
+
                         if let Some(command) = action.lsp_action.command() {
                             zlog::warn!(
                                 logger =>
                                 "Executing code action command '{}'. This may cause formatting to abort unnecessarily as well as splitting formatting into two entries in the undo history",
                                 &command.command,
                             );
-                                // bail early and command is invalid
+
-                                {
+                            // bail early if command is invalid
                             let server_capabilities = server.capabilities();
                             let available_commands = server_capabilities
                                 .execute_command_provider
@@ -1815,19 +1734,16 @@ impl LocalLspStore {
                                     command.command,
                                     server.name(),
                                 );
-                                        continue 'actions;
+                                continue;
-                                    }
                             }
 
-                                if let Some(err) = err_if_buffer_edited_since_start(
+                            // noop so we just ensure buffer hasn't been edited since resolving code actions
+                            extend_formatting_transaction(
                                 buffer,
-                                    transaction_id_format,
+                                formatting_transaction_id,
-                                    &cx,
+                                cx,
-                                ) {
+                                |_, _| {},
-                                    zlog::warn!(logger => "Buffer edited while formatting. Aborting");
+                            )?;
-                                    result = Err(err);
-                                    break 'formatters;
-                                }
                             zlog::info!(logger => "Executing command {}", &command.command);
 
                             lsp_store.update(cx, |this, _| {
@@ -1841,10 +1757,7 @@ impl LocalLspStore {
                                 .request::<lsp::request::ExecuteCommand>(
                                     lsp::ExecuteCommandParams {
                                         command: command.command.clone(),
-                                            arguments: command
+                                        arguments: command.arguments.clone().unwrap_or_default(),
-                                                .arguments
-                                                .clone()
-                                                .unwrap_or_default(),
                                         ..Default::default()
                                     },
                                 )
@@ -1878,7 +1791,6 @@ impl LocalLspStore {
                                     transaction.edit_ids.len(),
                                     &command.command,
                                 );
-                                    if let Some(transaction_id_format) = transaction_id_format {
                                 let transaction_id_project_transaction = transaction.id;
                                 buffer.handle.update(cx, |buffer, _| {
                                     // it may have been removed from history if push_to_history was
@@ -1887,18 +1799,15 @@ impl LocalLspStore {
                                     // and pop the combined transaction off the history stack
                                     // later if push_to_history is false
                                     if buffer.get_transaction(transaction.id).is_none() {
-                                                buffer
+                                        buffer.push_transaction(transaction, Instant::now());
-                                                    .push_transaction(transaction, Instant::now());
                                     }
                                     buffer.merge_transactions(
                                         transaction_id_project_transaction,
-                                                transaction_id_format,
+                                        formatting_transaction_id,
                                     );
                                 })?;
-                                    } else {
-                                        transaction_id_format = Some(transaction.id);
-                                    }
                             }
+
                             if !project_transaction_command.0.is_empty() {
                                 let extra_buffers = project_transaction_command
                                     .0
@@ -1917,54 +1826,9 @@ impl LocalLspStore {
                                     &command.command,
                                     extra_buffers,
                                 );
-                                    for (buffer_handle, transaction) in
+                                // NOTE: if this case is hit, the proper thing to do is to for each buffer, merge the extra transaction
-                                        project_transaction_command.0
+                                // into the existing transaction in project_transaction if there is one, and if there isn't one in project_transaction,
-                                    {
+                                // add it so it's included, and merge it into the format transaction when its created later
-                                        let entry = project_transaction.0.entry(buffer_handle);
-                                        use std::collections::hash_map::Entry;
-                                        match entry {
-                                            // if project_transaction already contains a transaction for this buffer, then
-                                            // we already formatted it, and we need to merge the new transaction into the one
-                                            // in project_transaction that we created while formatting
-                                            Entry::Occupied(mut occupied_entry) => {
-                                                let buffer_handle = occupied_entry.key();
-                                                buffer_handle.update(cx, |buffer, _| {
-                                                    // if push_to_history is true, then we need to make sure it is merged in the
-                                                    // buffer history as well
-                                                    if push_to_history {
-                                                        let transaction_id_project_transaction =
-                                                            transaction.id;
-                                                        // ensure transaction from command project transaction
-                                                        // is in history so we can merge
-                                                        if buffer
-                                                            .get_transaction(transaction.id)
-                                                            .is_none()
-                                                        {
-                                                            buffer.push_transaction(
-                                                                transaction.clone(),
-                                                                Instant::now(),
-                                                            );
-                                                        }
-                                                        let transaction_id_existing =
-                                                            occupied_entry.get().id;
-                                                        buffer.merge_transactions(
-                                                            transaction_id_project_transaction,
-                                                            transaction_id_existing,
-                                                        );
-                                                    }
-                                                })?;
-                                                let transaction_existing = occupied_entry.get_mut();
-                                                transaction_existing.merge_in(transaction);
-                                            }
-                                            // if there is no transaction in project_transaction, we either haven't formatted the buffer yet,
-                                            // or we won't format this buffer
-                                            // later iterations over the formattable_buffers list will use this as the starting transaction
-                                            // for the formatting on that buffer
-                                            Entry::Vacant(vacant_entry) => {
-                                                vacant_entry.insert(transaction);
-                                            }
-                                        }
-                                    }
                             }
                         }
                     }
@@ -1972,42 +1836,7 @@ impl LocalLspStore {
             }
         }
 
-            let buffer_handle = buffer.handle.clone();
+        Ok(())
-            buffer.handle.update(cx, |buffer, _| {
-                let Some(transaction_id) = transaction_id_format else {
-                    zlog::trace!(logger => "No formatting transaction id");
-                    return result;
-                };
-                let Some(transaction_id_last) =
-                    buffer.peek_undo_stack().map(|t| t.transaction_id())
-                else {
-                    // unwrapping should work here, how would we get a transaction id
-                    // with no transaction on the undo stack?
-                    // *but* it occasionally panics. Avoiding panics for now...
-                    zlog::warn!(logger => "No transaction present on undo stack, despite having a formatting transaction id?");
-                    return result;
-                };
-                if transaction_id_last != transaction_id {
-                    zlog::trace!(logger => "Last transaction on undo stack is not the formatting transaction, skipping finalization & update of project transaction");
-                    return result;
-                }
-                let transaction = buffer
-                    .finalize_last_transaction()
-                    .cloned()
-                    .expect("There is a transaction on the undo stack if we were able to peek it");
-                // debug_assert_eq!(transaction.id, transaction_id);
-                if !push_to_history {
-                    zlog::trace!(logger => "forgetting format transaction");
-                    buffer.forget_transaction(transaction.id);
-                }
-                project_transaction
-                    .0
-                    .insert(buffer_handle.clone(), transaction);
-                return result;
-            })??;
-        }
-
-        return Ok(project_transaction);
     }
 
     pub async fn format_ranges_via_lsp(
@@ -3088,19 +2917,7 @@ impl LocalLspStore {
                         .await?;
 
                     let transaction = buffer_to_edit.update(cx, |buffer, cx| {
-                        let is_formatting = this.read_with(cx, |lsp_store, _| {
-                            lsp_store
-                                .as_local()
-                                .map_or(false, |local| !local.buffers_being_formatted.is_empty())
-                        });
-                        // finalizing last transaction breaks workspace edits received due to
-                        // code actions that are run as part of formatting because formatting
-                        // groups transactions from format steps together to allow undoing all formatting with
-                        // a single undo, and to bail when the user modifies the buffer during formatting
-                        // finalizing the transaction prevents the necessary grouping from happening
-                        if !is_formatting {
                         buffer.finalize_last_transaction();
-                        }
                         buffer.start_transaction();
                         for (range, text) in edits {
                             buffer.edit([(range, text)], None, cx);
@@ -3108,10 +2925,7 @@ impl LocalLspStore {
 
                         let transaction = buffer.end_transaction(cx).and_then(|transaction_id| {
                             if push_to_history {
-                                if !is_formatting {
-                                    // see comment above about finalizing during formatting
                                 buffer.finalize_last_transaction();
-                                }
                                 buffer.get_transaction(transaction_id).cloned()
                             } else {
                                 buffer.forget_transaction(transaction_id)