refactor(language_server): move functions related to ServerLinter to ServerLinter#10761
Conversation
github-actions
bot
added
A-editor
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Instrumentation Performance ReportMerging #10761 will not alter performanceComparing Summary
|
WalkthroughThe changes restructure the configuration and initialization logic for the linter within the language server. The 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/oxc_language_server/src/linter/server_linter.rs (1)
110-128: Gracefully fall back when a custom config path is invalidSeveral chained
expect/unwrapcalls (e.g.root_path.join(relative_config_path.unwrap()),config.try_exists().expect(...)) can still panic if IO errors occur (e.g. permission denied, symlink loops). Consider replacing them withmatch+warn!so that the server linter continues with defaults instead of aborting.crates/oxc_language_server/src/worker.rs (1)
295-307: Minor optimisation: avoid locking per-file for ignore checks
is_ignoredacquires aMutexfor every file linted. Becausegitignore_globis mutated only when configuration changes (an infrequent, serialised path), switching it to anRwLock<Vec<Gitignore>>lets the hot path use a non-blocking read lock:- gitignore_glob: Mutex<Vec<Gitignore>>, + gitignore_glob: RwLock<Vec<Gitignore>>, ... - let gitignore_globs = &(*self.gitignore_glob.lock().await); + let gitignore_globs = &(*self.gitignore_glob.read().await);This reduces contention when multiple files are linted concurrently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/oxc_language_server/src/linter/server_linter.rs(2 hunks)crates/oxc_language_server/src/worker.rs(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/oxc_language_server/src/worker.rs (1)
crates/oxc_language_server/src/linter/server_linter.rs (4)
nested_configs(149-152)create_nested_configs(29-58)create_server_linter(103-165)create_ignore_glob(60-101)
Sysix
force-pushed
the
05-02-refactor_language_server_move_functions_related_to_serverlinter_to_serverlinter_
branch
from
1a95dc4 to
66aea30
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/oxc_language_server/src/linter/server_linter.rs (1)
50-62: Proper error handling implemented for config filesThis code now correctly handles errors from config file parsing instead of using
expect()calls, which prevents the language server from crashing on invalid config files.
🧹 Nitpick comments (4)
crates/oxc_language_server/src/linter/server_linter.rs (4)
147-147: Potential for error propagationThis line unwraps the result of
ConfigStoreBuilder::from_oxlintrcand falls back to default, but doesn't log any warning if the creation fails.- let config_builder = - ConfigStoreBuilder::from_oxlintrc(false, oxlintrc.clone()).unwrap_or_default(); + let config_builder = match ConfigStoreBuilder::from_oxlintrc(false, oxlintrc.clone()) { + Ok(builder) => builder, + Err(err) => { + warn!("Failed to create config store builder: {}", err); + ConfigStoreBuilder::default() + } + };
163-168: Consider avoiding the needless copy of nested configsThis code copies the entire nested configs collection to create a new FxHashMap, which could be inefficient for large projects with many config files.
Consider refactoring the
Linter::new_with_nested_configsAPI to accept a reference to the configs, or create a method that works directly with the ConcurrentHashMap to avoid the extra copy.
219-234: Good test for nested config discoveryThis test validates the correct behavior of finding and loading multiple nested configs. However, it would be even better to include a test for error handling.
Consider adding a test case that includes an invalid config file to verify the error handling code works properly.
149-149: TODO comment needs addressingThere's a TODO comment about refactoring shared functionality. Consider creating an issue to track this technical debt or address it in a follow-up PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/oxc_language_server/src/linter/server_linter.rs(2 hunks)crates/oxc_language_server/src/worker.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/oxc_language_server/src/worker.rs
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Clippy
- GitHub Check: Benchmark (formatter)
- GitHub Check: Benchmark (codegen)
- GitHub Check: Benchmark (minifier)
- GitHub Check: Benchmark (semantic)
- GitHub Check: Benchmark (isolated_declarations)
- GitHub Check: Test wasm32-wasip1-threads
- GitHub Check: Benchmark (transformer)
- GitHub Check: Benchmark (parser)
- GitHub Check: Test (macos-latest)
- GitHub Check: Benchmark (lexer)
- GitHub Check: Test
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Build Linter Benchmark
🔇 Additional comments (5)
crates/oxc_language_server/src/linter/server_linter.rs (5)
1-18: Import organization looks good for the new functionalityThe imports are well organized and cover all the needed functionality for the new methods. The code imports standard library components, external crates, and internal modules in a logical manner.
29-68: Good implementation ofcreate_nested_configswith proper error handlingThis implementation properly handles potential errors during config loading, addressing previous concerns about panicking on malformed configs. The error handling logic now gracefully skips invalid configurations with appropriate warning logs.
105-108: Good fix for potential panic when handling oxlintrc pathsThis properly handles the case where
oxlintrc.pathhas no parent directory, addressing a previous review comment about potential panics with the default config.
182-186: Good change to make constructor privateMaking
new_with_linterprivate is appropriate since it's now only called internally by the public static methodcreate_server_linter. This provides a more focused API for consumers.
207-217: Good test case for disabled nested configsThis test correctly verifies that when nested configs are disabled, an empty collection is returned. This helps ensure the feature flag works properly.
|
This should only be a move Fn A to B. The AI wants me to improve it all in one go 😆 |
Merge activity
|
Boshen
force-pushed
the
05-02-refactor_language_server_move_functions_related_to_serverlinter_to_serverlinter_
branch
from
66aea30 to
54592e0
Compare
…o `ServerLinter` (#10761)
graphite-app
bot
force-pushed
the
05-02-refactor_language_server_move_functions_related_to_serverlinter_to_serverlinter_
branch
from
54592e0 to
623bc45
Compare
…o `ServerLinter` (#10761)
graphite-app
bot
force-pushed
the
05-02-refactor_language_server_move_functions_related_to_serverlinter_to_serverlinter_
branch
from
eacb96b to
3d794f6
Compare
graphite-app
bot
deleted the
05-02-refactor_language_server_move_functions_related_to_serverlinter_to_serverlinter_
branch
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
crates/oxc_language_server/src/linter/server_linter.rs (4)
112-113:⚠️ Potential issueConvert
expectto error handling in add_lineThere's a potential crash point in the code where
builder.add_line(None, entry)usesexpect. If adding an ignore line fails, this will crash the entire language server process.- builder.add_line(None, entry).expect("Failed to add ignore line"); + if let Err(err) = builder.add_line(None, entry) { + warn!("Failed to add ignore line: {}: {}", entry, err); + continue; + }
123-124:⚠️ Potential issueUnwrap could panic when converting URI to file path
The
to_file_path().unwrap()call can panic if the URI cannot be converted to a file path, crashing the language server.- let root_path = root_uri.to_file_path().unwrap(); + let root_path = match root_uri.to_file_path() { + Ok(path) => path, + Err(_) => { + warn!("Failed to convert URI to file path: {}", root_uri); + return (ServerLinter::new_with_linter( + Linter::default(), + IsolatedLintHandlerOptions { + use_cross_module: false, + root_path: PathBuf::new(), + }, + ), Oxlintrc::default()); + } + };
158-159:⚠️ Potential issueHandle config store build errors gracefully
Using
expectwhen building the config store can crash the language server if there's an issue with the configuration.- let config_store = config_builder.build().expect("Failed to build config store"); + let config_store = match config_builder.build() { + Ok(store) => store, + Err(err) => { + warn!("Failed to build config store: {}", err); + ConfigStoreBuilder::default().build().unwrap_or_default() + } + };
127-134:⚠️ Potential issueReplace expect with proper error handling for file existence check
Using
expectfor checking if a file exists can crash the language server if there's an issue with file system access.- if config.try_exists().expect("Could not get fs metadata for config") { + if let Ok(exists) = config.try_exists() { + if exists { + if let Ok(oxlintrc) = Oxlintrc::from_file(&config) { + oxlintrc + } else { + warn!("Failed to initialize oxlintrc config: {}", config.to_string_lossy()); + Oxlintrc::default() + } + } else { + warn!( + "Config file not found: {}, fallback to default config", + config.to_string_lossy() + ); + Oxlintrc::default() + } + } else { + warn!("Could not get fs metadata for config: {}", config.to_string_lossy()); + Oxlintrc::default() + }
🧹 Nitpick comments (1)
crates/oxc_language_server/src/linter/server_linter.rs (1)
147-147: Handle unwrap_or_default potential errorWhile
unwrap_or_default()won't panic, it silently falls back to a default. Consider logging a warning when this happens for better debuggability.- let config_builder = - ConfigStoreBuilder::from_oxlintrc(false, oxlintrc.clone()).unwrap_or_default(); + let config_builder = match ConfigStoreBuilder::from_oxlintrc(false, oxlintrc.clone()) { + Ok(builder) => builder, + Err(err) => { + warn!("Failed to create config builder from oxlintrc: {}", err); + ConfigStoreBuilder::default() + } + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/oxc_language_server/src/linter/server_linter.rs(2 hunks)crates/oxc_language_server/src/worker.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/oxc_language_server/src/worker.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/oxc_language_server/src/linter/server_linter.rs (7)
crates/oxc_linter/src/lib.rs (3)
options(106-108)new(80-82)new_with_nested_configs(85-91)crates/oxc_linter/src/config/config_builder.rs (3)
default(31-33)new(430-432)from_oxlintrc(87-187)crates/oxc_language_server/src/linter/config_walker.rs (2)
paths(83-89)new(66-81)crates/oxc_language_server/src/worker.rs (1)
new(32-47)crates/oxc_language_server/src/tester.rs (2)
new(95-97)get_file_uri(13-17)crates/oxc_language_server/src/linter/isolated_lint_handler.rs (1)
new(37-39)crates/oxc_linter/src/config/oxlintrc.rs (1)
from_file(111-143)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Benchmark (formatter)
- GitHub Check: Benchmark (codegen)
- GitHub Check: Benchmark (minifier)
- GitHub Check: Test wasm32-wasip1-threads
- GitHub Check: Benchmark (semantic)
- GitHub Check: Benchmark (isolated_declarations)
- GitHub Check: Clippy
- GitHub Check: Benchmark (transformer)
- GitHub Check: Test Linux
- GitHub Check: Benchmark (parser)
- GitHub Check: Benchmark (lexer)
- GitHub Check: Build Linter Benchmark
- GitHub Check: Test
🔇 Additional comments (5)
crates/oxc_language_server/src/linter/server_linter.rs (5)
29-68: LGTM! Improved resilience compared to previous implementationThe new
create_nested_configsmethod includes proper error handling for config file parsing and building, appropriately logging warnings and continuing with execution rather than crashing the language server. This is a significant improvement over the previous implementation, which usedexpectcalls that could potentially terminate the process.
105-108: LGTM! Improved handling of parent pathThis code now properly handles the case where
oxlintrc.pathhas no parent, which addresses a previous issue where the language server could crash when using the default config. This is a good improvement.
182-186: LGTM! Appropriate encapsulationMaking the
new_with_lintermethod private is a good design choice, as it encourages using the more comprehensivecreate_server_linterstatic method, which handles all the necessary setup logic.
206-217: LGTM! Good test coverage for disabled nested configsThis test properly verifies that when nested configs are disabled, the
create_nested_configsmethod returns an empty map without attempting to search for config files.
219-234: LGTM! Good test coverage for nested configsThis test verifies that the
create_nested_configsmethod correctly finds and processes config files in a nested directory structure. The sorting of results ensures consistent test results.
| gitignore_globs.push(builder.build().unwrap()); | ||
| gitignore_globs |
There was a problem hiding this comment.
Handle unwrap in gitignore build
The builder.build().unwrap() call can panic if building the gitignore fails.
- gitignore_globs.push(builder.build().unwrap());
+ if let Ok(gitignore) = builder.build() {
+ gitignore_globs.push(gitignore);
+ } else {
+ warn!("Failed to build gitignore from oxlintrc patterns");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gitignore_globs.push(builder.build().unwrap()); | |
| gitignore_globs | |
| if let Ok(gitignore) = builder.build() { | |
| gitignore_globs.push(gitignore); | |
| } else { | |
| warn!("Failed to build gitignore from oxlintrc patterns"); | |
| } | |
| gitignore_globs |
🤖 Prompt for AI Agents (early access)
In crates/oxc_language_server/src/linter/server_linter.rs around lines 114 to 115, the call to builder.build().unwrap() can cause a panic if building the gitignore fails. Replace unwrap() with proper error handling by matching on the Result returned by builder.build(), and handle the error case gracefully, such as returning an error or logging it, to prevent the program from panicking.
| let walk = ignore::WalkBuilder::new(root_uri.to_file_path().unwrap()) | ||
| .ignore(true) |
There was a problem hiding this comment.
Handle URI to file path conversion error
The code uses unwrap() to convert a URI to file path, which could crash the language server if the URI is invalid.
- let walk = ignore::WalkBuilder::new(root_uri.to_file_path().unwrap())
+ let root_path = match root_uri.to_file_path() {
+ Ok(path) => path,
+ Err(_) => {
+ warn!("Failed to convert URI to file path in create_ignore_glob: {}", root_uri);
+ return vec![];
+ }
+ };
+ let walk = ignore::WalkBuilder::new(root_path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let walk = ignore::WalkBuilder::new(root_uri.to_file_path().unwrap()) | |
| .ignore(true) | |
| // Safely convert the root URI to a file path, avoiding a panic on invalid URIs | |
| let root_path = match root_uri.to_file_path() { | |
| Ok(path) => path, | |
| Err(_) => { | |
| warn!( | |
| "Failed to convert URI to file path in create_ignore_glob: {}", | |
| root_uri | |
| ); | |
| return vec![]; | |
| } | |
| }; | |
| let walk = ignore::WalkBuilder::new(root_path) | |
| .ignore(true) |
🤖 Prompt for AI Agents (early access)
In crates/oxc_language_server/src/linter/server_linter.rs around lines 78 to 79, the code uses unwrap() to convert a URI to a file path, which can cause a crash if the URI is invalid. Replace unwrap() with proper error handling by checking if the conversion returns Some(path) or None, and handle the None case gracefully, such as returning an error or logging it, to prevent the language server from crashing.
| builder.add(Glob::new("**/.eslintignore").unwrap()); | ||
| builder.add(Glob::new("**/.gitignore").unwrap()); | ||
|
|
||
| let ignore_file_glob_set = builder.build().unwrap(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential unwrap panics in glob creation
The code contains multiple unwrap() calls that could panic if there are issues with glob pattern creation.
- builder.add(Glob::new("**/.eslintignore").unwrap());
- builder.add(Glob::new("**/.gitignore").unwrap());
-
- let ignore_file_glob_set = builder.build().unwrap();
+ if let Ok(eslintignore_glob) = Glob::new("**/.eslintignore") {
+ builder.add(eslintignore_glob);
+ } else {
+ warn!("Failed to create glob pattern for .eslintignore");
+ }
+
+ if let Ok(gitignore_glob) = Glob::new("**/.gitignore") {
+ builder.add(gitignore_glob);
+ } else {
+ warn!("Failed to create glob pattern for .gitignore");
+ }
+
+ let ignore_file_glob_set = match builder.build() {
+ Ok(set) => set,
+ Err(err) => {
+ warn!("Failed to build glob set: {}", err);
+ return vec![];
+ }
+ };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In crates/oxc_language_server/src/linter/server_linter.rs around lines 73 to 77, the use of unwrap() on Glob::new and builder.build() can cause panics if glob patterns are invalid or building fails. Replace unwrap() calls with proper error handling by matching on the Result returned from Glob::new and builder.build(), and handle errors gracefully, such as by returning a Result from the function or logging the error and exiting cleanly.
2 participants
No description provided.