refactor(language_server): move gitignore_glob to ServerLinter#10762
Conversation
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 #10762 will not alter performanceComparing Summary
|
96c2cbb to
c6e2d45
Compare
1a95dc4 to
66aea30
Compare
c6e2d45 to
794f6a2
Compare
66aea30 to
54592e0
Compare
794f6a2 to
5087608
Compare
66aea30 to
3d794f6
Compare
5087608 to
8c611a6
Compare
WalkthroughThe changes centralize ignore pattern handling within the In contrast, the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (13)
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
crates/oxc_language_server/src/worker.rs (1)
106-119:⚠️ Potential issueStale diagnostics remain for newly-ignored files
When
run_singlereturnsNone(e.g. the file has just become ignored via a new.gitignorerule),
we exit early without purging the previous entry indiagnostics_report_map.
As a result, the client will keep showing old diagnostics for files that should now be ignored.- if let Some(diagnostics) = diagnostics { - self.diagnostics_report_map - .read() - .await - .pin() - .insert(uri.to_string(), diagnostics.clone()); - - return Some(diagnostics); - } - - None + match diagnostics { + Some(reports) => { + self.diagnostics_report_map + .read() + .await + .pin() + .insert(uri.to_string(), reports.clone()); + Some(reports) + } + None => { + // Clear stale diagnostics for ignored / deleted files + self.diagnostics_report_map + .read() + .await + .pin() + .remove(&uri.to_string()); + None + } + }Ensuring the cache is cleaned keeps the client view consistent with the server’s ignore logic.
🧹 Nitpick comments (4)
crates/oxc_language_server/src/worker.rs (2)
70-80: Minimise lock-holding while rebuilding the linter
refresh_server_linterkeeps a read lock onnested_configsfor the whole duration ofServerLinter::create_server_linter(..).
That helper clones the entire map (pin().iter() … collect) and also performs filesystem I/O when rebuilding configs, which can be slow.
Holding the read-lock that long prevents writers (e.g.did_change_watched_files) from mutatingnested_configs, so a burst of rebuilds could starve writers.A lighter pattern:
-let nested_configs = self.nested_configs.read().await; -let server_linter = ServerLinter::create_server_linter( - self.root_uri.get().unwrap(), - &options, - &nested_configs, -); +let nested_snapshot = { + // clone while we *briefly* hold the lock … + self.nested_configs.read().await.clone() +}; +let server_linter = ServerLinter::create_server_linter( + self.root_uri.get().unwrap(), + &options, + &nested_snapshot, +);This confines the critical section to an inexpensive clone and removes any contention between config updates and linter rebuilds.
252-253: Avoid unnecessary full linter rebuilds
refresh_server_linter()is called unconditionally after every watched-file event when nested configs are enabled, even when the change was not to anoxlintconfig file (earlier guards may bail-out, but the call still happens).
Rebuilding the linter is expensive (re-parsing every config, re-walking ignore files).Consider invoking the refresh only when:
file_event.typimplies creation / update / deletion and- the file matched
OXC_CONFIG_FILE.This yields the same behaviour with far less CPU.
crates/oxc_language_server/src/linter/server_linter.rs (2)
70-116:create_ignore_globcan become O(N·files) & block the serverThe function recursively walks the entire workspace on every linter rebuild to locate
.gitignore/.eslintignorefiles.
In large mono-repos this can take seconds and is performed on the async-runtime thread, blocking other requests.Suggestions:
- Cache the resulting
Vec<Gitignore>inWorkspaceWorkerand only refresh it when a watched ignore file changes.- Restrict the walk to directories actually referenced by the current
uribeing linted (lazily load).- Off-load the walk to a blocking task:
tokio::task::spawn_blocking.These changes would keep latency predictable even for huge repositories.
200-206: Run-time check now relies on ignore logic – add unit tests
run_singleshort-circuits onis_ignored, which is great, but there are no
tests exercising workspace ignore rules (only nested-config tests exist).Adding fixtures such as:
fixtures/linter/ignore_patterns ├─ .gitignore (# ignore *.ts) └─ src/ignored.tsand asserting that
run_singlereturnsNoneforignored.tswould lock this
behaviour in place and prevent future regressions.
📜 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(5 hunks)crates/oxc_language_server/src/worker.rs(6 hunks)
8c611a6 to
88072e4
Compare
camc314
left a comment
There was a problem hiding this comment.
looks good to me, but looks like there's conflicts
88072e4 to
fb5a464
Compare
Merge activity
|
fb5a464 to
9ec13f6
Compare

No description provided.