Skip to content

add ignore current file to code actions #178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nkxxll wants to merge 4 commits into
base: main
Choose a base branch
from
Open

add ignore current file to code actions #178

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fcead67
add ignore current file to code actions
nkxxll Dec 12, 2025
b1bcd8f
compute relative path only if the action is called
nkxxll Dec 14, 2025
739673c
canonicalize both paths to ensure match if there is one
nkxxll Dec 14, 2025
7a04410
use `.to_string()` instead of `format!(...)`
nkxxll Dec 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions crates/codebook-lsp/src/lsp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ pub struct Backend {
enum CodebookCommand {
AddWord,
AddWordGlobal,
IgnoreFile,
Unknown,
}

Expand All @@ -46,6 +47,7 @@ impl From<&str> for CodebookCommand {
match command {
"codebook.addWord" => CodebookCommand::AddWord,
"codebook.addWordGlobal" => CodebookCommand::AddWordGlobal,
"codebook.ignoreFile" => CodebookCommand::IgnoreFile,
_ => CodebookCommand::Unknown,
}
}
Expand All @@ -56,6 +58,7 @@ impl From<CodebookCommand> for String {
match command {
CodebookCommand::AddWord => "codebook.addWord".to_string(),
CodebookCommand::AddWordGlobal => "codebook.addWordGlobal".to_string(),
CodebookCommand::IgnoreFile => "codebook.ignoreFile".to_string(),
CodebookCommand::Unknown => "codebook.unknown".to_string(),
}
}
Expand Down Expand Up @@ -93,6 +96,7 @@ impl LanguageServer for Backend {
commands: vec![
CodebookCommand::AddWord.into(),
CodebookCommand::AddWordGlobal.into(),
CodebookCommand::IgnoreFile.into(),
],
work_done_progress_options: Default::default(),
}),
Expand Down Expand Up @@ -256,6 +260,20 @@ impl LanguageServer for Backend {
data: None,
}));
}
actions.push(CodeActionOrCommand::CodeAction(CodeAction {
title: "Add current file to ignore list".to_string(),
kind: Some(CodeActionKind::QUICKFIX),
diagnostics: None,
edit: None,
command: Some(Command {
title: "Add current file to ignore list".to_string(),
Comment on lines +263 to +269
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The title string is duplicated in the title and command.title fields. This is inconsistent with the other code actions above (lines 234-247, 248-261) which use different strings for these fields. The title field should be a user-facing description while command.title can be the same. Consider making these consistent or using a variable to avoid duplication.

Suggested change
actions.push(CodeActionOrCommand::CodeAction(CodeAction {
title: "Add current file to ignore list".to_string(),
kind: Some(CodeActionKind::QUICKFIX),
diagnostics: None,
edit: None,
command: Some(Command {
title: "Add current file to ignore list".to_string(),
let ignore_file_title = "Add current file to ignore list".to_string();
actions.push(CodeActionOrCommand::CodeAction(CodeAction {
title: ignore_file_title.clone(),
kind: Some(CodeActionKind::QUICKFIX),
diagnostics: None,
edit: None,
command: Some(Command {
title: ignore_file_title,

Copilot uses AI. Check for mistakes.
command: CodebookCommand::IgnoreFile.into(),
arguments: Some(vec![params.text_document.uri.to_string().into()]),
}),
is_preferred: None,
disabled: None,
data: None,
}));
match actions.is_empty() {
true => Ok(None),
false => Ok(Some(actions)),
Expand Down Expand Up @@ -294,6 +312,23 @@ impl LanguageServer for Backend {
}
Ok(None)
}
CodebookCommand::IgnoreFile => {
let config = self.config_handle();
let file_uri = params
.arguments
.first()
.expect("CodebookCommand::IgnoreFile: There has to be a file URI here!")
.as_str()
.expect(
"CodebookCommand::IgnoreFile: Argument should be convertible to a String.",
);
let updated = self.add_ignore_file(config.as_ref(), file_uri);
Comment on lines +315 to +325
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with the AddWord command handler (lines 291-293), consider adding an info log when the ignore file command is executed, such as "Adding file to ignore list: {relative_path}". This would make debugging and monitoring LSP operations easier.

Copilot uses AI. Check for mistakes.
if updated {
let _ = config.save();
self.recheck_all().await;
}
Ok(None)
}
CodebookCommand::Unknown => Ok(None),
}
}
Expand Down Expand Up @@ -382,6 +417,7 @@ impl Backend {
}
should_save
}

fn add_words_global(
&self,
config: &CodebookConfigFile,
Expand All @@ -404,6 +440,42 @@ impl Backend {
should_save
}

Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get_relative_path method lacks documentation explaining its behavior, especially the fallback cases when canonicalization fails or when the file is outside the workspace. Consider adding a doc comment that describes the return value in different scenarios.

Suggested change
/// Returns the path of the file referred to by `uri` relative to the workspace directory.
///
/// - If the workspace directory can be canonicalized and the file is inside it,
/// returns the relative path from the workspace root.
/// - If the file is outside the workspace directory, returns the absolute file path as a string.
/// - If canonicalization of the workspace directory fails, returns the absolute file path as a string.

Copilot uses AI. Check for mistakes.
fn get_relative_path(&self, uri: &str) -> String {
let uri = Url::parse(uri)
.expect("This is a correctly formatted URL because it comes from the LSP protocol.");
Comment on lines +444 to +445
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using expect here can cause a panic if the URL is malformed. While the comment suggests this comes from the LSP protocol, it's safer to handle this error gracefully. Consider returning an error or using the original uri string as a fallback.

Suggested change
let uri = Url::parse(uri)
.expect("This is a correctly formatted URL because it comes from the LSP protocol.");
let uri = match Url::parse(uri) {
Ok(url) => url,
Err(e) => {
error!("Failed to parse URI '{uri}': {e}");
return uri.to_string();
}
};

Copilot uses AI. Check for mistakes.
let file_path = uri.to_file_path().unwrap_or_default();
let absolute_workspace_dir = &self.workspace_dir.canonicalize();
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The get_relative_path method performs canonicalization every time it's called, which can be expensive I/O operation. As noted in the PR description, this happens every time code actions are shown. Consider caching the canonicalized workspace directory in the Backend struct to improve performance, or defer path computation to when the command is actually executed.

Copilot uses AI. Check for mistakes.

match absolute_workspace_dir {
Ok(dir) => match file_path.canonicalize() {
Ok(canon_file_path) => match canon_file_path.strip_prefix(dir) {
Ok(relative) => relative.to_string_lossy().to_string(),
Err(_) => file_path.to_string_lossy().to_string(),
},
Err(_) => file_path.to_string_lossy().to_string(),
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback logic has a potential issue when canonicalize() fails on the file path. When the file cannot be canonicalized (e.g., file doesn't exist or was deleted), the function falls back to the non-canonicalized file_path, but this is then implicitly compared against a canonicalized workspace directory in the outer match. This mismatch can cause strip_prefix to fail even for files legitimately within the workspace, potentially causing relative paths not to be computed correctly. Consider either: (1) using non-canonicalized paths for both workspace_dir and file_path as a fallback, or (2) handling the canonicalization failure of the file more explicitly to avoid the mismatch.

Suggested change
Err(_) => file_path.to_string_lossy().to_string(),
Err(_) => {
// Fallback: try to strip the non-canonicalized workspace dir from the non-canonicalized file path
match file_path.strip_prefix(&self.workspace_dir) {
Ok(relative) => relative.to_string_lossy().to_string(),
Err(_) => file_path.to_string_lossy().to_string(),
}
}

Copilot uses AI. Check for mistakes.
Copy link
Author

@nkxxll nkxxll Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes literally no sense at all

},
Err(err) => {
info!("Could not get absolute path from workspace directory. Error: {err}.");
file_path.to_string_lossy().to_string()
}
}
}

Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The add_ignore_file method lacks documentation explaining its purpose and return value. Consider adding a doc comment that describes what this method does and when it returns true vs false.

Suggested change
/// Adds the specified file URI to the list of ignored files in the given configuration.
///
/// Returns `true` if the file was successfully added to the ignore list.
/// Returns `false` if the file was already present in the ignore list or if an error occurred.
///
/// # Arguments
///
/// * `config` - The configuration file to update.
/// * `file_uri` - The URI of the file to ignore.

Copilot uses AI. Check for mistakes.
fn add_ignore_file(&self, config: &CodebookConfigFile, file_uri: &str) -> bool {
let relative_path = &self.get_relative_path(file_uri);
match config.add_ignore(relative_path) {
Ok(true) => true,
Ok(false) => {
info!("File {file_uri} already exists in the ignored files.");
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log message uses file_uri (the full URI) but should use relative_path to accurately reflect what was actually added to the ignore list. This inconsistency could make debugging more difficult.

Suggested change
info!("File {file_uri} already exists in the ignored files.");
info!("File {relative_path} already exists in the ignored files.");

Copilot uses AI. Check for mistakes.
false
}
Err(e) => {
error!("Failed to add ignore file: {e}");
false
}
}
}

fn make_suggestion(&self, suggestion: &str, range: &Range, uri: &Url) -> CodeAction {
let title = format!("Replace with '{suggestion}'");
let mut map = HashMap::new();
Expand Down