perf(lsp): debounce didChange notifications to reduce processing#86
Conversation
Add debouncing to textDocument/didChange handler to avoid excessive processing during rapid typing. This significantly reduces network requests, CPU usage, and respects rate limits. Key changes: - Add ProcessingContext struct for cloneable async processing state - Implement per-URI debounce tracking with configurable delay - Add debounce_ms config option (default: 200ms) - Cancel pending debounce on document save (immediate processing) - Clean up debounce tasks on document close Performance improvement: ~84% reduction in network requests during rapid typing (e.g., typing "express" now triggers 1 request instead of 7). Closes #73
|
Warning Rate limit exceeded@mpiton has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughDebounce was added for per-URI didChange handling with a configurable delay (default 200ms). A private ProcessingContext centralizes async document parsing and dependency resolution. Backend state and registries/parsers are Arc-wrapped for safe sharing with spawned debounce and processing tasks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LSPHandler
participant DebounceMap
participant ProcessingContext
participant RegistryLayer
participant VulnerabilityService
Client->>LSPHandler: textDocument/didChange(uri, content)
LSPHandler->>DebounceMap: store pending_changes[uri]=content
LSPHandler->>DebounceMap: abort existing debounce_tasks[uri]
LSPHandler->>DebounceMap: spawn debounce task (delay = config.cache.debounce_ms)
DebounceMap->>DebounceMap: wait delay
DebounceMap->>LSPHandler: (on delay) validate latest pending content
LSPHandler->>ProcessingContext: process_document(uri, content)
ProcessingContext->>RegistryLayer: resolve dependencies (bounded concurrency)
RegistryLayer->>ProcessingContext: return resolved data
ProcessingContext->>VulnerabilityService: schedule/check vulnerabilities (background)
ProcessingContext->>LSPHandler: update document state & publish diagnostics/inlays
Note over Client,LSPHandler: textDocument/didSave bypasses debounce and calls ProcessingContext immediately
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @dependi-lsp/src/backend.rs:
- Around line 85-236: The PR has two duplicated implementations of the same
logic: ProcessingContext::process_document and DependiBackend::process_document;
remove the duplication by making DependiBackend::process_document delegate to
ProcessingContext::process_document (or vice‑versa) so there is a single
implementation. Modify DependiBackend::process_document to construct/obtain a
ProcessingContext (or pass &self into an existing context) and call
ProcessingContext::process_document(uri, content). Move any backend-specific
state access (e.g., self.crates_io, self.version_cache, self.client,
self.config, osv_client, vuln_cache) into ProcessingContext so the delegated
function has the required Arcs/clones, and update any callers to rely on the
single method; then delete the redundant block to eliminate the ~150-line
duplicate.
🧹 Nitpick comments (1)
dependi-lsp/src/backend.rs (1)
71-83: Consider extracting shared parsing logic to reduce duplication.The
parse_documentmethod duplicates the exact logic fromDependiBackend::parse_document(lines 351-363). Consider extracting this into a standalone function that both can call, or havingProcessingContext::parse_documentdelegate to a shared implementation.♻️ Potential refactor to eliminate duplication
One approach is to make the parsing logic a standalone function:
+fn parse_document_internal( + uri: &Url, + content: &str, + cargo_parser: &CargoParser, + npm_parser: &NpmParser, + python_parser: &PythonParser, + go_parser: &GoParser, + php_parser: &PhpParser, + dart_parser: &DartParser, + csharp_parser: &CsharpParser, + ruby_parser: &RubyParser, +) -> Vec<crate::parsers::Dependency> { + match FileType::detect(uri) { + Some(FileType::Cargo) => cargo_parser.parse(content), + Some(FileType::Npm) => npm_parser.parse(content), + Some(FileType::Python) => python_parser.parse(content), + Some(FileType::Go) => go_parser.parse(content), + Some(FileType::Php) => php_parser.parse(content), + Some(FileType::Dart) => dart_parser.parse(content), + Some(FileType::Csharp) => csharp_parser.parse(content), + Some(FileType::Ruby) => ruby_parser.parse(content), + None => vec![], + } +} + impl ProcessingContext { fn parse_document(&self, uri: &Url, content: &str) -> Vec<crate::parsers::Dependency> { - match FileType::detect(uri) { - Some(FileType::Cargo) => self.cargo_parser.parse(content), - Some(FileType::Npm) => self.npm_parser.parse(content), - Some(FileType::Python) => self.python_parser.parse(content), - Some(FileType::Go) => self.go_parser.parse(content), - Some(FileType::Php) => self.php_parser.parse(content), - Some(FileType::Dart) => self.dart_parser.parse(content), - Some(FileType::Csharp) => self.csharp_parser.parse(content), - Some(FileType::Ruby) => self.ruby_parser.parse(content), - None => vec![], - } + parse_document_internal( + uri, content, + &self.cargo_parser, &self.npm_parser, &self.python_parser, + &self.go_parser, &self.php_parser, &self.dart_parser, + &self.csharp_parser, &self.ruby_parser, + ) }Then apply similar changes to
DependiBackend::parse_document.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dependi-lsp/src/backend.rsdependi-lsp/src/config.rs
🧰 Additional context used
🧬 Code graph analysis (2)
dependi-lsp/src/config.rs (8)
dependi-lsp/src/cache/mod.rs (2)
default(116-118)default(245-247)dependi-lsp/src/registries/npm.rs (1)
default(113-115)dependi-lsp/src/registries/crates_io.rs (1)
default(144-146)dependi-lsp/src/registries/pub_dev.rs (1)
default(112-114)dependi-lsp/src/registries/nuget.rs (1)
default(117-119)dependi-lsp/src/vulnerabilities/cache.rs (1)
default(186-188)dependi-lsp/src/cache/sqlite.rs (1)
default(40-49)dependi-lsp/src/vulnerabilities/osv.rs (1)
default(80-82)
dependi-lsp/src/backend.rs (10)
dependi-lsp/src/file_types.rs (2)
detect(39-64)cache_key(86-97)dependi-lsp/src/parsers/cargo.rs (1)
new(12-14)dependi-lsp/src/parsers/csharp.rs (1)
new(10-12)dependi-lsp/src/parsers/ruby.rs (1)
new(15-17)dependi-lsp/src/parsers/dart.rs (1)
new(10-12)dependi-lsp/src/parsers/go.rs (1)
new(10-12)dependi-lsp/src/parsers/npm.rs (1)
new(10-12)dependi-lsp/src/parsers/php.rs (1)
new(10-12)dependi-lsp/src/parsers/python.rs (1)
new(10-12)dependi-lsp/src/providers/diagnostics.rs (1)
create_diagnostics(15-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Code coverage
- GitHub Check: security-audit
- GitHub Check: test
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
dependi-lsp/src/backend.rs (5)
42-68: LGTM! Well-designed context struct for debounce tasks.The
ProcessingContextstruct appropriately wraps all necessary state inArcfor safe sharing across async debounce tasks, and the documentation clearly explains its purpose.
238-271: LGTM! Appropriate Arc-wrapping for concurrent access.The Arc-wrapping of fields and addition of
debounce_tasksandpending_changesmaps correctly support per-URI debounce tracking with safe concurrent access.
823-877: Debounce implementation is sound with good race condition handling.The debouncing logic correctly:
- Cancels previous tasks before spawning new ones
- Verifies content hasn't changed before processing (line 859-862)
- Cleans up task handles after completion
One minor edge case: if
did_saveis called while a debounce task is already executingprocess_document(past the cancellation point), both will process the document. This is acceptable since processing is idempotent, but you may want to add a comment documenting this behavior.Optional: Consider adding a comment to document the idempotency requirement:
// Note: If did_save is called while a debounce task is already processing, // both will complete. This is acceptable as processing is idempotent.
879-895: LGTM! Correct implementation of save bypassing debounce.The logic properly cancels any pending debounce task, cleans up pending state, and processes immediately as expected for save operations.
897-911: LGTM! Proper cleanup prevents memory leaks.The close handler correctly cancels pending tasks and cleans up all associated state, preventing memory leaks from abandoned debounce tasks.
dependi-lsp/src/config.rs (1)
61-72: LGTM! Well-documented configuration field with sensible default.The 200ms default debounce delay strikes a good balance between responsiveness and reducing unnecessary processing. The field is properly documented and follows the existing configuration patterns.
Remove ~160 lines of duplicate code by having DependiBackend::process_document delegate to ProcessingContext::process_document. The ProcessingContext now contains the single canonical implementation for document processing. Changes: - Remove DependiBackend::parse_document (use ProcessingContext::parse_document) - Replace DependiBackend::process_document body with delegation to context - ProcessingContext remains the single source of truth for processing logic
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @dependi-lsp/src/backend.rs:
- Around line 694-715: The debounce task cleanup races because tasks remove the
entry unconditionally; fix by storing a generation id with each task handle
(change debounce_tasks to map URI -> (u64, JoinHandle<_>)), increment a
generation on each did_change spawn, capture that generation in the spawned
task, and when the task finishes only remove the entry if the stored generation
still equals the captured generation (i.e., compare the tuple's generation
before removing). Also update the abort/update path to destructure the tuple and
compare generations when aborting/replacing so you only abort the matching
generation; ensure all uses of debounce_tasks insert, remove and abort handle
the (gen, handle) tuple consistently and capture gen in the task closure.
🧹 Nitpick comments (1)
dependi-lsp/src/backend.rs (1)
160-162: Consider logging task failures.The error from
handle.awaitis silently ignored. While best-effort fetching is reasonable, logging join errors would help diagnose panics or task failures during development.📊 Optional: Add debug logging for task failures
for handle in handles { - let _ = handle.await; + if let Err(e) = handle.await { + tracing::debug!("Registry fetch task failed: {}", e); + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dependi-lsp/src/backend.rs
🧰 Additional context used
🧬 Code graph analysis (1)
dependi-lsp/src/backend.rs (9)
dependi-lsp/src/file_types.rs (2)
detect(39-64)cache_key(86-97)dependi-lsp/src/parsers/cargo.rs (1)
new(12-14)dependi-lsp/src/parsers/ruby.rs (1)
new(15-17)dependi-lsp/src/parsers/dart.rs (1)
new(10-12)dependi-lsp/src/parsers/csharp.rs (1)
new(10-12)dependi-lsp/src/parsers/go.rs (1)
new(10-12)dependi-lsp/src/parsers/npm.rs (1)
new(10-12)dependi-lsp/src/parsers/php.rs (1)
new(10-12)dependi-lsp/src/parsers/python.rs (1)
new(10-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
dependi-lsp/src/backend.rs (8)
42-68: Well-designed context struct for async processing.The ProcessingContext encapsulates all necessary state with Arc-wrapped references, enabling efficient cloning for debounced tasks. The design properly separates concerns and avoids expensive deep copies.
85-162: Solid concurrency control and processing pipeline.The implementation correctly:
- Limits concurrent registry fetches to 5 using a semaphore
- Updates document state and diagnostics immediately before background vulnerability checks
- Properly clones Arc references for async tasks
238-271: Appropriate Arc-wrapping for debounce support.The field updates correctly introduce Arc-wrapping for shared state and add the necessary debounce tracking structures. Documentation clearly explains the purpose of each change.
287-322: Constructor correctly initializes Arc-wrapped fields.All parsers, config, and documents are properly wrapped in Arc, and the new debounce tracking structures are initialized as empty DashMaps.
324-349: Clean helper for creating processing context.The method efficiently creates a ProcessingContext by cloning Arc references, which is a cheap operation (only reference count increments).
719-735: did_save correctly bypasses debounce for immediate processing.The implementation properly cancels any pending debounce task and processes the document immediately on save, which aligns with user expectations (save should be synchronous).
737-751: did_close correctly cancels debounce and cleans up state.The cleanup logic properly cancels pending debounce tasks, removes pending changes, clears document state, and publishes empty diagnostics.
490-494: Clean delegation eliminates duplication.The refactoring successfully centralizes document processing logic in ProcessingContext, removing the ~160 lines of duplication mentioned in the PR objectives.
Add generation IDs to debounce tasks to prevent racing cleanup. The issue: when task A is aborted and task B spawns for the same URI, task A's cleanup code could still run and remove task B's handle from the map, orphaning task B. Solution: - Store (generation, JoinHandle) tuples in debounce_tasks map - Increment global generation counter on each task spawn - Capture generation in spawned task closure - Only remove entry on cleanup if generation matches (remove_if) - Update did_save/did_close to destructure the new tuple format This ensures only the task that was actually spawned for an entry can clean it up.
1 participant
Summary
Add debouncing to
textDocument/didChangehandler to avoid excessive processing during rapid typing. This significantly reduces network requests, CPU usage, and respects rate limits.Key Changes
debounce_msconfig option inCacheConfig(default: 200ms)did_savecancels pending debounce and processes immediatelydid_closecancels pending tasks and cleans up statePerformance Improvement
Before (current behavior):
After (with debouncing):
Result: ~84% reduction in network requests and CPU usage during rapid typing.
Configuration
The debounce delay is configurable via LSP initialization options:
{ "cache": { "debounce_ms": 200 } }Test Plan
Closes #73
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.