add ignore current file to code actions #178

nkxxll · 2025-12-12T18:58:29Z

added the ignore current file code action
the action adds the current file as relative file path to the codebook.toml
for that I have to resolve the relative (to the workspace_dir) path of the file
as the workspace_dir is not canonicalised I'll have to do that every time (but I don't like doing the canonicalisation every time somebody hits show code action maybe one can add the canonicalised path to the Backend structure but I'll leave that up to you)

- another thing I though of is that the path is computed every time you want to get the code actions. It might be more performant to send the current uri as a whole and compute the relative path when the user actually invokes the command...Let me know if that is something you would want to have changed

PROBLEM: relative paths like shown in the screenshot above are not correctly ignored. Did I mess up the path or is this a bug?
I'll need some feedback from you on that but I'll also look into it when I have the time to.

Update: I looked into it a bit and the paths are correctly shown as ignored from the config somehow the LSP does not respect that. When trying the cli codebook it works just as expected. I think it is something specific to the LSP, but I cant quite grasp what it is.

Apart from that the code action works like a charm. Let me know if you have some feedback 😄.

Copilot

Pull request overview

This PR adds a new code action that allows users to ignore the current file by adding its relative path to the codebook.toml configuration file. The implementation includes command handling, path resolution with workspace directory canonicalization, and configuration updates.

Key changes:

Adds "Ignore Current File" code action to LSP code action menu
Implements relative path computation by canonicalizing the workspace directory
Adds new command codebook.ignoreFile with execution handler

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-12T19:03:36Z

crates/codebook-lsp/src/lsp.rs

+        let relative_path = self.get_relative_path(&params.text_document.uri);
+        actions.push(CodeActionOrCommand::CodeAction(CodeAction {
+            title: format!("Add current file to ignore list"),
+            kind: Some(CodeActionKind::QUICKFIX),
+            diagnostics: None,
+            edit: None,
+            command: Some(Command {
+                title: format!("Add current file to ignore list"),
+                command: CodebookCommand::IgnoreFile.into(),
+                arguments: Some(vec![relative_path.into()]),
+            }),
+            is_preferred: None,
+            disabled: None,
+            data: None,
+        }));


The ignore file code action is being added unconditionally, even when there are no diagnostics in the file. This means the action will appear in files that are already being ignored or have no spelling issues. Consider only showing this action when there are actual diagnostics present, similar to how the word-related actions are only shown within the diagnostic loop.

Copilot · 2025-12-12T19:03:36Z

crates/codebook-lsp/src/lsp.rs

                data: None,
            }));
        }
+        let relative_path = self.get_relative_path(&params.text_document.uri);


The get_relative_path method is called on every code action request, which triggers filesystem canonicalization each time. This is inefficient, especially since code actions are requested frequently (on cursor movement, file changes, etc.). Consider caching the canonicalized workspace directory in the Backend struct or computing the relative path only when the command is actually executed.

removed and placed in the add_ignore_file function

Copilot · 2025-12-12T19:03:37Z

crates/codebook-lsp/src/lsp.rs

+            Ok(dir) => match file_path.strip_prefix(dir) {
+                Ok(relative) => relative.to_string_lossy().to_string(),
+                Err(_) => file_path.to_string_lossy().to_string(),
+            },


The file path obtained from the URI might not be canonicalized, which could cause strip_prefix to fail even when the file is within the workspace. For example, if the file path contains symbolic links or . or .. components while the workspace directory is canonicalized, the prefix stripping will fail. Consider canonicalizing the file path as well before comparing, or handle this case more explicitly to ensure consistent path resolution.

Suggested change

Ok(dir) => match file_path.strip_prefix(dir) {

Ok(relative) => relative.to_string_lossy().to_string(),

Err(_) => file_path.to_string_lossy().to_string(),

},

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(),

}

}

Copilot · 2025-12-12T19:03:37Z

crates/codebook-lsp/src/lsp.rs

        }
        should_save
    }



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 · 2025-12-12T19:03:37Z

crates/codebook-lsp/src/lsp.rs

+            }
+        }
+    }
+


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 · 2025-12-12T19:03:38Z

crates/codebook-lsp/src/lsp.rs

+                let file_uri = params
+                    .arguments
+                    .first()
+                    .expect("CodebookCommand::IgnoreFile: There has to be a file URI here!");
+                let updated = self.add_ignore_file(
+                    config.as_ref(),
+                    file_uri.as_str().expect(
+                        "CodebookCommand::IgnoreFile: Argument should be convertable to a String!",
+                    ),


Using expect() here will cause a panic if arguments are missing. This could crash the LSP server if there's a protocol error or malformed request. Consider using pattern matching with proper error handling instead, similar to how other command handlers extract arguments.

Suggested change

let file_uri = params

.arguments

.first()

.expect("CodebookCommand::IgnoreFile: There has to be a file URI here!");

let updated = self.add_ignore_file(

config.as_ref(),

file_uri.as_str().expect(

"CodebookCommand::IgnoreFile: Argument should be convertable to a String!",

),

let file_uri = match params.arguments.first() {

Some(arg) => match arg.as_str() {

Some(s) => s,

None => {

return Err(

RpcError::invalid_params(

"CodebookCommand::IgnoreFile: Argument should be convertible to a String!",

)

);

}

},

None => {

return Err(

RpcError::invalid_params(

"CodebookCommand::IgnoreFile: There has to be a file URI here!",

)

);

}

};

let updated = self.add_ignore_file(

config.as_ref(),

file_uri,

Copilot · 2025-12-12T19:03:38Z

crates/codebook-lsp/src/lsp.rs

+                let file_uri = params
+                    .arguments
+                    .first()
+                    .expect("CodebookCommand::IgnoreFile: There has to be a file URI here!");
+                let updated = self.add_ignore_file(
+                    config.as_ref(),
+                    file_uri.as_str().expect(
+                        "CodebookCommand::IgnoreFile: Argument should be convertable to a String!",
+                    ),


Using expect() here will cause a panic if the argument is not a string. This could crash the LSP server if there's a protocol error. Consider using pattern matching or returning an error instead.

Suggested change

let file_uri = params

.arguments

.first()

.expect("CodebookCommand::IgnoreFile: There has to be a file URI here!");

let updated = self.add_ignore_file(

config.as_ref(),

file_uri.as_str().expect(

"CodebookCommand::IgnoreFile: Argument should be convertable to a String!",

),

let file_uri = match params.arguments.first() {

Some(uri) => uri,

None => {

error!("CodebookCommand::IgnoreFile: There has to be a file URI here!");

return Err(jsonrpc_v2::Error::invalid_params(

"Missing file URI argument for IgnoreFile command",

));

}

};

let file_uri_str = match file_uri.as_str() {

Some(s) => s,

None => {

error!("CodebookCommand::IgnoreFile: Argument should be convertible to a String!");

return Err(jsonrpc_v2::Error::invalid_params(

"File URI argument for IgnoreFile command is not a string",

));

}

};

let updated = self.add_ignore_file(

config.as_ref(),

file_uri_str,

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-14T22:10:17Z

crates/codebook-lsp/src/lsp.rs

+                    Ok(relative) => relative.to_string_lossy().to_string(),
+                    Err(_) => file_path.to_string_lossy().to_string(),
+                },
+                Err(_) => file_path.to_string_lossy().to_string(),


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(),

}

}

that makes literally no sense at all

Copilot · 2025-12-14T22:10:18Z

crates/codebook-lsp/src/lsp.rs

+        match config.add_ignore(relative_path) {
+            Ok(true) => true,
+            Ok(false) => {
+                info!("File {file_uri} already exists in the ignored files.");


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 · 2025-12-14T22:10:18Z

crates/codebook-lsp/src/lsp.rs

+            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);


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.

crates/codebook-lsp/src/lsp.rs

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-14T22:39:34Z

crates/codebook-lsp/src/lsp.rs

+        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(),


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 · 2025-12-14T22:39:34Z

crates/codebook-lsp/src/lsp.rs

+        let uri = Url::parse(uri)
+            .expect("This is a correctly formatted URL because it comes from the LSP protocol.");
+        let file_path = uri.to_file_path().unwrap_or_default();
+        let absolute_workspace_dir = &self.workspace_dir.canonicalize();


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 · 2025-12-14T22:39:35Z

crates/codebook-lsp/src/lsp.rs

+        let uri = Url::parse(uri)
+            .expect("This is a correctly formatted URL because it comes from the LSP protocol.");


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();

}

};

add ignore current file to code actions

fcead67

Copilot AI review requested due to automatic review settings December 12, 2025 18:58

Copilot started reviewing on behalf of nkxxll December 12, 2025 18:58 View session

Copilot AI reviewed Dec 12, 2025

View reviewed changes

nkxxll added 2 commits December 14, 2025 22:59

compute relative path only if the action is called

b1bcd8f

canonicalize both paths to ensure match if there is one

739673c

Copilot AI review requested due to automatic review settings December 14, 2025 22:06

Copilot started reviewing on behalf of nkxxll December 14, 2025 22:07 View session

Copilot AI reviewed Dec 14, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings December 14, 2025 22:36

Copilot started reviewing on behalf of nkxxll December 14, 2025 22:36 View session

use .to_string() instead of format!(...)

7a04410

nkxxll force-pushed the ignore-current-file-code-action branch from f1fcfc5 to 7a04410 Compare December 14, 2025 22:38

Copilot AI reviewed Dec 14, 2025

View reviewed changes

+    /// 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.

+    /// 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.

-                let file_uri = params
-                    .arguments
-                    .first()
-                    .expect("CodebookCommand::IgnoreFile: There has to be a file URI here!");
-                let updated = self.add_ignore_file(
-                    config.as_ref(),
-                    file_uri.as_str().expect(
-                        "CodebookCommand::IgnoreFile: Argument should be convertable to a String!",
-                    ),
+                let file_uri = match params.arguments.first() {
+                    Some(arg) => match arg.as_str() {
+                        Some(s) => s,
+                        None => {
+                            return Err(
+                                RpcError::invalid_params(
+                                    "CodebookCommand::IgnoreFile: Argument should be convertible to a String!",
+                                )
+                            );
+                        }
+                    },
+                    None => {
+                        return Err(
+                            RpcError::invalid_params(
+                                "CodebookCommand::IgnoreFile: There has to be a file URI here!",
+                            )
+                        );
+                    }
+                };
+                let updated = self.add_ignore_file(
+                    config.as_ref(),
+                    file_uri,

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

		let uri = Url::parse(uri)
		.expect("This is a correctly formatted URL because it comes from the LSP protocol.");

-        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();
+            }
+        };

add ignore current file to code actions #178

Are you sure you want to change the base?

add ignore current file to code actions #178

Uh oh!

Conversation

nkxxll commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

nkxxll Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

nkxxll Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

nkxxll Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

nkxxll commented Dec 12, 2025 •

edited

Loading