crates/lsp/src/utils.rs (1)

76-99: Align CodeAction.kind with advertised kinds.
You advertise quickfix.ast-grep in capabilities but return kind=quickfix here. Consider returning the same sub-kind to avoid client-side filtering mismatches.

Apply this diff:

-      kind: Some(CodeActionKind::QUICKFIX),
+      // Align with advertised kind to survive client-side 'only' filtering
+      kind: Some(CodeActionKind::new("quickfix.ast-grep")),

crates/lsp/src/lib.rs (1)

331-345: Cache builder is correct but can skip non-fixable diagnostics.
Avoid inserting entries with empty fixers to reduce memory and lookups.

Apply this diff:

   if let (Some(data), Some(NumberOrString::String(id))) = (diagnostic.data.as_ref(), diagnostic.code.as_ref()) {
-    if let Some(rewrite_data) = RewriteData::from_value(data.clone()) {
-      fixes.insert((diagnostic.range, id.clone()), rewrite_data);
-    }
+    if let Some(rewrite_data) = RewriteData::from_value(data.clone()) {
+      if !rewrite_data.fixers.is_empty() {
+        fixes.insert((diagnostic.range, id.clone()), rewrite_data);
+      }
+    }
   }

crates/lsp/tests/basic.rs (2)

388-417: Robustness: handle additional headers in wait_for_diagnostics path.
Some servers include extra headers (e.g., Content-Type). Your custom codec currently assumes only Content-Length. Optional to extend decoder; fine for tests.

---

565-613: LspCodec: tolerate multiple headers or interleaved frames (optional).
Current decode searches a single "Content-Length: " and assumes no extra headers. Consider parsing header lines until CRLFCRLF to be more future-proof; fine as-is for internal tests.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1497
File: crates/cli/src/utils/mod.rs:174-175
Timestamp: 2024-09-29T21:59:50.675Z
Learning: ast-grep requires Rust 1.65 or newer. It's acceptable to use syntax and features introduced in Rust 1.65 or newer in ast-grep code.

crates/lsp/src/lib.rs (3)

30-31: De-duplicate Fixes alias; import from utils instead.

Maintain a single source of truth to avoid drift.

Apply this diff:

-use utils::{convert_match_to_diagnostic, diagnostic_to_code_action, RewriteData};
+use utils::{convert_match_to_diagnostic, diagnostic_to_code_action, RewriteData, Fixes};
@@
-type Fixes = HashMap<(Range, String), RewriteData>;

And in crates/lsp/src/utils.rs, make Fixes public (see separate comment there).

---

439-459: Prefer non-overlapping, larger edits first; avoid extra iterator churn.

Sorting by end ascending keeps the shortest when ranges overlap. Prefer longest-first at the same start to reduce partial edits. Also consume the vector directly.

Apply this diff:

-    let mut fixes = Vec::from_iter(fixes.iter());
-    fixes.sort_by_key(|((range, _id), _rewrite_data)| (range.start, range.end));
+    let mut entries: Vec<_> = fixes.iter().collect();
+    // start ASC, end DESC to prefer larger ranges when overlapping
+    entries.sort_by(|((ra, _), _), ((rb, _), _)| {
+      ra.start.cmp(&rb.start).then(rb.end.cmp(&ra.end))
+    });
@@
-    let edits: Vec<TextEdit> = fixes
-      .iter()
+    let edits: Vec<TextEdit> = entries
+      .into_iter()
       .filter_map(|((range, _id), rewrite_data)| {
         if range.start < last {
           return None;
         }
         let fixed = rewrite_data.fixers.first()?.fixed.to_string();
         let edit = TextEdit::new(*range, fixed);
         last = range.end;
         Some(edit)
       })
       .collect();

---

396-397: Guard against empty content changes to avoid index panic.

LSP can legally send an empty array.

Apply this diff:

-    let text = &params.content_changes[0].text;
+    let change = params.content_changes.first()?;
+    let text = &change.text;

crates/lsp/src/utils.rs (3)

14-15: Export Fixes so lib.rs can reuse it.

Prevents duplicated aliases and keeps the key contract centralized.

Apply this diff:

-type Fixes = HashMap<(Range, String), RewriteData>;
+pub type Fixes = HashMap<(Range, String), RewriteData>;

---

28-53: Skip caching empty fixers.

Avoid storing RewriteData with no actionable edits.

Apply this diff:

   pub fn from_node_match<L: LanguageExt>(
@@
-    let fixers = rule
+    let fixers: Vec<OneFix> = rule
@@
-      .collect();
-    Some(Self { fixers })
+      .collect();
+    if fixers.is_empty() {
+      None
+    } else {
+      Some(Self { fixers })
+    }

---

67-85: Mark only the first quick fix as preferred.

Setting all as preferred is ambiguous for clients.

Apply this diff:

-  let actions = rewrite_data.fixers.clone().into_iter().map(|fixer| {
+  let actions = rewrite_data
+    .fixers
+    .clone()
+    .into_iter()
+    .enumerate()
+    .map(|(i, fixer)| {
@@
-    CodeAction {
+    CodeAction {
       title,
       command: None,
       diagnostics: None,
       edit: Some(edit),
       disabled: None,
       kind: Some(CodeActionKind::QUICKFIX),
-      is_preferred: Some(true),
+      is_preferred: Some(i == 0),
       data: None,
     }
   });

crates/lsp/tests/basic.rs (3)

555-557: Remove fixed sleep; rely on wait_for_diagnostics for robustness.

Avoids flaky timing.

Apply this diff:

-  tokio::time::sleep(tokio::time::Duration::from_secs(2)).await;

---

687-714: Decoder should return Ok(None) on partial/incomplete UTF-8, not error.

Prevents spurious decode errors on chunk boundaries.

Apply this diff:

-    let src_str =
-      std::str::from_utf8(&src[..]).map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?;
+    let src_str = match std::str::from_utf8(&src[..]) {
+      Ok(s) => s,
+      Err(_) => return Ok(None), // wait for more bytes
+    };

---

419-446: Consider unique request IDs to avoid collisions across requests.

Using a constant id can collide with prior in-flight requests; increment or randomize.

Example:

-  let code_action_request = serde_json::json!({
-    "jsonrpc": "2.0",
-    "id": 1,
+  static ID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1);
+  let id = ID.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
+  let code_action_request = serde_json::json!({
+    "jsonrpc": "2.0",
+    "id": id,
@@
-    match tokio::time::timeout(std::time::Duration::from_secs(2), sender.next()).await {
+    match tokio::time::timeout(std::time::Duration::from_secs(2), sender.next()).await {
       Ok(Some(Ok(msg))) => {
-        if msg.get("id") == Some(&serde_json::json!(1)) {
+        if msg.get("id") == Some(&serde_json::json!(id)) {
           return Some(msg);
         }
       }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: reductor
PR: ast-grep/ast-grep#2209
File: crates/lsp/src/lib.rs:355-357
Timestamp: 2025-08-31T10:52:39.398Z
Learning: In the ast-grep LSP server, diagnostic.data is kept intact even when clients don't advertise codeAction data support. This defensive programming approach ensures compatibility with clients that might work despite not properly advertising their capabilities, prioritizing robustness over strict LSP spec compliance.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1497
File: crates/cli/src/utils/mod.rs:174-175
Timestamp: 2024-09-29T21:59:50.675Z
Learning: ast-grep requires Rust 1.65 or newer. It's acceptable to use syntax and features introduced in Rust 1.65 or newer in ast-grep code.

Learnt from: reductor
PR: ast-grep/ast-grep#2209
File: crates/lsp/src/lib.rs:355-357
Timestamp: 2025-08-31T10:52:39.398Z
Learning: In the ast-grep LSP server, diagnostic.data is kept intact even when clients don't advertise codeAction data support. This defensive programming approach ensures compatibility with clients that might work despite not properly advertising their capabilities, prioritizing robustness over strict LSP spec compliance.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1546
File: crates/cli/src/utils/rule_overwrite.rs:76-91
Timestamp: 2024-10-22T14:04:15.734Z
Learning: In the `ast-grep-cli` application code, avoid unnecessary abstractions; functions don't need to accept iterators instead of `Vec` if not required.

crates/lsp/src/utils.rs (1)

27-56: from_node_match: exact-range filter is safe but may drop valid fixers.

You currently skip fixers whose edit doesn’t exactly match the node range. That prevents misapplies, but also drops legit multi-node/multi-line edits. Consider supporting expanded ranges later.

crates/lsp/tests/basic.rs (1)

341-379: Minor: expose request_code_action for reuse in tests.

It’s handy for reuse across scenarios; consider pub(crate) if needed by other test modules.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: reductor
PR: ast-grep/ast-grep#2209
File: crates/lsp/src/lib.rs:355-357
Timestamp: 2025-08-31T10:52:39.435Z
Learning: In the ast-grep LSP server, diagnostic.data is kept intact even when clients don't advertise codeAction data support. This defensive programming approach ensures compatibility with clients that might work despite not properly advertising their capabilities, prioritizing robustness over strict LSP spec compliance.

Learnt from: reductor
PR: ast-grep/ast-grep#2209
File: crates/lsp/src/lib.rs:355-357
Timestamp: 2025-08-31T10:52:39.435Z
Learning: In the ast-grep LSP server, diagnostic.data is kept intact even when clients don't advertise codeAction data support. This defensive programming approach ensures compatibility with clients that might work despite not properly advertising their capabilities, prioritizing robustness over strict LSP spec compliance.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1546
File: crates/cli/src/utils/rule_overwrite.rs:76-91
Timestamp: 2024-10-22T14:04:15.734Z
Learning: In the `ast-grep-cli` application code, avoid unnecessary abstractions; functions don't need to accept iterators instead of `Vec` if not required.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1497
File: crates/cli/src/utils/mod.rs:174-175
Timestamp: 2024-09-29T21:59:50.675Z
Learning: ast-grep requires Rust 1.65 or newer. It's acceptable to use syntax and features introduced in Rust 1.65 or newer in ast-grep code.

crates/lsp/tests/basic.rs (1)

378-436: Fix: apply edits across multiple lines and preserve trailing newline.

Current implementation drops the final newline and ignores multi-line ranges, which can make “fix all” assertions inaccurate.

Apply:

 fn apply_all_code_actions(text: &str, actions: &Vec<Value>) -> String {
-  // As offsets are based on the original text, we need to track changes
-  let mut lines: Vec<String> = text.lines().map(|s| s.to_string()).collect();
+  // Work line-wise but preserve trailing newline and handle multi-line edits
+  let mut lines: Vec<String> = text.lines().map(|s| s.to_string()).collect();
+  let had_trailing_newline = text.ends_with('\n');
   let mut all_edits = Vec::new();
@@
-  // Apply edits
-  for (start_line, start_char, _end_line, end_char, new_text) in all_edits {
-    let line = &lines[start_line];
-    let prefix = &line[..start_char];
-    let suffix = &line[end_char..];
-    lines[start_line] = format!("{}{}{}", prefix, new_text, suffix);
-  }
+  // Apply edits (same-line and cross-line)
+  for (start_line, start_char, end_line, end_char, new_text) in all_edits {
+    if start_line == end_line {
+      let line = &lines[start_line];
+      let prefix = &line[..start_char];
+      let suffix = &line[end_char..];
+      lines[start_line] = format!("{}{}{}", prefix, new_text, suffix);
+    } else {
+      let first = &lines[start_line];
+      let last = &lines[end_line];
+      let prefix = &first[..start_char];
+      let suffix = &last[end_char..];
+      let merged = format!("{}{}{}", prefix, new_text, suffix);
+      lines.splice(start_line..=end_line, std::iter::once(merged));
+    }
+  }
@@
-  // Join lines back into a single string
-  lines.join("\n")
+  // Join lines back, preserving original trailing newline
+  let mut s = lines.join("\n");
+  if had_trailing_newline {
+    s.push('\n');
+  }
+  s
 }

crates/lsp/tests/basic.rs (2)

310-338: Minor: shorten timeout loop to speed up tests.

Consider fewer iterations or a shorter timeout (e.g., 10 x 1s) to reduce test latency on failures.

---

538-590: Update expectation to keep trailing newline; prefer assert_eq!.

After fixing apply_all_code_actions, preserve the final '\n' and use assert_eq! for clarity.

-  assert!(actions.len() == 1, "Expected 1 code action for fix all");
+  assert_eq!(actions.len(), 1, "Expected 1 code action for fix all");
@@
-  // TODO: This fix ends up with \n being trimmed at the end, need to investigate
-  assert_eq!(fixed_text, "alert('Hello, world!')\nalert('Another log')");
+  assert_eq!(fixed_text, "alert('Hello, world!')\nalert('Another log')\n");

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: reductor
PR: ast-grep/ast-grep#2209
File: crates/lsp/src/lib.rs:355-357
Timestamp: 2025-08-31T10:52:39.435Z
Learning: In the ast-grep LSP server, diagnostic.data is kept intact even when clients don't advertise codeAction data support. This defensive programming approach ensures compatibility with clients that might work despite not properly advertising their capabilities, prioritizing robustness over strict LSP spec compliance.

Learnt from: HerringtonDarkholme
PR: ast-grep/ast-grep#1497
File: crates/cli/src/utils/mod.rs:174-175
Timestamp: 2024-09-29T21:59:50.675Z
Learning: ast-grep requires Rust 1.65 or newer. It's acceptable to use syntax and features introduced in Rust 1.65 or newer in ast-grep code.