🐛 fix flaky nodejs queries #995
Conversation
WalkthroughNormalizes Node.js workspace URIs, expands InitializeParams capabilities and WorkspaceFolders; updates LSP base client to track opened files, add mutexes, change symbol helper signatures to use Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BaseClient as LSPServiceClientBase
participant Cache as SymbolCache
participant Workspace as WorkspaceSymbolService
rect rgb(245,250,245)
Note over Client,BaseClient: Initialization + WorkspaceFolders normalization
Client->>BaseClient: Initialize(WorkspaceFolders, Capabilities, InitializationOptions)
BaseClient-->>Client: Initialized
end
rect rgb(250,245,245)
Note over Client,BaseClient: File lifecycle & cache population
Client->>BaseClient: NotifyFileChanges(files)
activate BaseClient
BaseClient->>BaseClient: Lock allConditionsMutex
BaseClient->>BaseClient: didClose(changed URIs) (update openedFiles)
BaseClient->>BaseClient: populateDocumentSymbolCache (parallel tasks)
BaseClient->>BaseClient: Unlock allConditionsMutex
deactivate BaseClient
end
rect rgb(245,245,250)
Note over Client,BaseClient: Declaration lookup path (flag selects)
Client->>BaseClient: GetAllDeclarations(ctx, query, useWorkspaceSymbol)
alt useWorkspaceSymbol == true
BaseClient->>Workspace: workspace/symbols RPC
Workspace-->>BaseClient: []WorkspaceSymbol
else
BaseClient->>Cache: search cached DocumentSymbols
Cache-->>BaseClient: []WorkspaceSymbol
end
BaseClient-->>Client: []WorkspaceSymbol
end
rect rgb(250,250,240)
Note over Client,BaseClient: documentSymbol retrieval with retries
Client->>BaseClient: queryDocumentSymbol(uri)
BaseClient->>BaseClient: didOpen(uri) (track openedFiles)
loop retries
BaseClient->>BaseClient: request documentSymbol RPC
end
BaseClient-->>Client: Normalized DocumentSymbols
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Signed-off-by: Pranav Gaikwad <[email protected]>
pranavgaikwad
force-pushed
the
fix/nodejsCapabilities
branch
from
f7a9983 to
a4ca7e8
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lsp/base_service_client/base_service_client.go (1)
661-690: Add mutex protection foropenedFilesaccess.The
openedFilesmap is read/written here without synchronization. This can cause data races when multiple goroutines calldidOpen/didCloseconcurrently (e.g., during parallel symbol cache population).func (sc *LSPServiceClientBase) didOpen(ctx context.Context, uri uri.URI, text []byte) error { + sc.openedFilesMutex.Lock() if _, exists := sc.openedFiles[uri]; exists { + sc.openedFilesMutex.Unlock() return nil } sc.openedFiles[uri] = true + sc.openedFilesMutex.Unlock() params := protocol.DidOpenTextDocumentParams{ // ... } return sc.Conn.Notify(ctx, "textDocument/didOpen", params) } func (sc *LSPServiceClientBase) didClose(ctx context.Context, uri uri.URI) error { + sc.openedFilesMutex.Lock() if _, exists := sc.openedFiles[uri]; !exists { + sc.openedFilesMutex.Unlock() return nil } delete(sc.openedFiles, uri) + sc.openedFilesMutex.Unlock() params := protocol.DidCloseTextDocumentParams{ // ... } return sc.Conn.Notify(ctx, "textDocument/didClose", params) }
🧹 Nitpick comments (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
68-80: Consider using URI parsing instead of string replacement.Line 77 uses
strings.ReplaceAll(f, "file://", "")which could be fragile. If the path somehow containsfile://elsewhere (unlikely but possible), it would cause issues.seen[f] = true + parsed, err := uri.Parse(f) + var name string + if err == nil { + name = filepath.Base(parsed.Filename()) + } else { + name = filepath.Base(strings.TrimPrefix(f, "file://")) + } workspaceFolders = append(workspaceFolders, protocol.WorkspaceFolder{ URI: f, - Name: filepath.Base(strings.ReplaceAll(f, "file://", "")), + Name: name, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go(4 hunks)lsp/base_service_client/base_capabilities.go(1 hunks)lsp/base_service_client/base_service_client.go(16 hunks)lsp/base_service_client/symbol_cache.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T16:25:40.903Z
Learnt from: shawn-hurley
Repo: konveyor/analyzer-lsp PR: 860
File: lsp/base_service_client/base_service_client.go:212-214
Timestamp: 2025-10-21T16:25:40.903Z
Learning: In `lsp/base_service_client/base_service_client.go`, when an RPC connection is provided via `c.RPC` (not nil), it is treated as already initialized and no handler setup is performed. This is known technical debt that needs to be addressed in the future to ensure LSP notifications like `textDocument/publishDiagnostics` are properly processed.
Applied to files:
lsp/base_service_client/base_service_client.go
⏰ 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). (7)
- GitHub Check: test
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
🔇 Additional comments (14)
lsp/base_service_client/base_capabilities.go (1)
57-57: LGTM!The addition of
trueforuseWorkspaceSymbolcorrectly enables workspace symbol queries for the genericEvaluateReferencedimplementation, while allowing provider-specific overrides (like Node.js) to disable it.lsp/base_service_client/symbol_cache.go (1)
200-215: LGTM!The updated signature using
uri.URIis more type-safe. The implementation correctly validates the file scheme before extracting the extension, returning an empty string for non-file URIs.external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (3)
49-55: LGTM!The location normalization ensures consistent URI formatting. Setting the location as the sole workspace folder aligns with the PR objective.
82-97: LGTM!The ClientCapabilities are correctly configured to:
- Enable workspace folder support
- Enable diagnostic refresh
- Enable definition link support (required for node_modules lookups)
- Enable hierarchical document symbols
These align with the PR objectives for proper Node.js LSP integration.
169-169: LGTM!Passing
falseforuseWorkspaceSymbolcorrectly disables the unreliableworkspace/symbolqueries for Node.js, per the PR objectives.lsp/base_service_client/base_service_client.go (9)
341-343: LGTM!Calling
didClosebefore invalidating the cache ensures the LSP server is properly notified of file changes, preventing stale state.
353-355: LGTM!The mutex properly protects the
allConditionsassignment from concurrent access during Prepare.
450-481: LGTM!The
useWorkspaceSymbolparameter cleanly controls whether to attemptworkspace/symbolqueries, falling back to the symbol cache. This enables provider-specific behavior (e.g., disabling for Node.js).
567-579: LGTM!Creating a
wsForDefinitionwith the matched symbol name ensures proper fallback whenfindDocumentSymbolAtLocationdoesn't find a better match, preserving the definition location information.
706-741: LGTM!The retry logic with
maxAttempts=2and 100ms delay addresses flaky document symbol queries. The normalization handling for workspace symbol types returned by typescript-language-server is a good defensive measure.
769-772: LGTM!Proper use of
RLock/RUnlockto safely readallConditionswhile allowing concurrent reads.
828-854: LGTM!The
unmarshalLocationshelper robustly handles the various response formats fromtextDocument/definition:
[]LocationLink(withTargetURI/TargetRange)- Single
Location[]LocationThis defensive parsing prevents failures with different LSP server implementations.
870-894: LGTM!The retry logic for
textDocument/definitionmirrors the pattern used inqueryDocumentSymbol, providing consistent retry behavior for flaky LSP operations.
899-944: LGTM!The refactored
findDocumentSymbolAtLocationnow:
- Constructs a proper
WorkspaceSymbolfor comparison- Uses
MatchSymbolByPatternsfor flexible matching- Finds the smallest overlapping symbol (most specific match)
This provides more accurate symbol resolution.
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lsp/base_service_client/base_service_client.go (3)
278-280: Mutex unnecessary during initialization.The mutex protection during map initialization is defensive but not required, as the struct has not yet been shared across goroutines at this point in
NewLSPServiceClientBase.Consider simplifying:
- sc.openedFilesMutex.Lock() sc.openedFiles = make(map[uri.URI]bool) - sc.openedFilesMutex.Unlock()
552-581: Add context cancellation check in parallel goroutines.The parallelization with semaphore and WaitGroup is well-structured, but the goroutines should check
ctx.Err()before performing expensive operations to respect cancellation.Apply this diff to add context cancellation checks:
wg.Add(1) go func(ms protocol.WorkspaceSymbol) { defer wg.Done() sem <- struct{}{} defer func() { <-sem }() + if ctx.Err() != nil { + return + } + location, ok := ms.Location.Value.(protocol.Location) if !ok { sc.Log.V(7).Info("unable to get location from workspace symbol", "workspace symbol", ms) return } defs := sc.getDefinitionForPosition(ctx, fileURI, location) if len(defs) > 0 { results <- defResult{matchedSymbol: ms, definitions: defs} } }(matchedSymbol)
955-979: Distinguish between RPC errors and empty responses.The current implementation logs an error for both RPC failures and empty responses. Empty responses (no definitions found) are not necessarily errors and could reduce log noise.
Consider separating empty response handling:
for attempt := 1; attempt <= maxAttempts; attempt++ { var tmp json.RawMessage err := sc.Conn.Call(ctx, "textDocument/definition", position).Await(ctx, &tmp) if err != nil { lastErr = err } else if len(tmp) == 0 { - lastErr = fmt.Errorf("textDocument/definition returned zero locations") + // Empty response is not an error, just no definitions found + return nil } else { locations, ok := unmarshalLocations(tmp) if ok && len(locations) > 0 { return locations } + lastErr = fmt.Errorf("unable to unmarshal definition response") } if ctx.Err() != nil { return nil } if attempt < maxAttempts { time.Sleep(100 * time.Millisecond) } } if lastErr != nil { sc.Log.Error(lastErr, "textDocument/definition request failed", "uri", uri) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
demo-output.yaml(1 hunks)lsp/base_service_client/base_service_client.go(19 hunks)lsp/base_service_client/symbol_cache.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lsp/base_service_client/symbol_cache.go
🧰 Additional context used
🧬 Code graph analysis (1)
lsp/base_service_client/base_service_client.go (1)
provider/provider.go (3)
ConditionsByCap(472-475)Location(319-322)Position(324-342)
⏰ 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). (9)
- GitHub Check: test
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (windows-latest, windows)
🔇 Additional comments (10)
demo-output.yaml (1)
806-811: New nodejs provider incident for tsx React reference aligns with PR fixes.This addition shows the nodejs provider now detecting a React reference at line 3 (React.FC type annotation) in Component.tsx, complementing the existing detection at line 1 (React import). This reflects the improved nodejs provider capabilities for TypeScript/React files enabled by the PR changes (workspace setup, client capabilities, and query retries).
The incident structure and formatting are consistent with the existing patterns in the file.
lsp/base_service_client/base_service_client.go (9)
150-154: Mutexes added correctly to protect concurrent access.The addition of
openedFilesMutex(regular mutex) andallConditionsMutex(RW mutex) properly addresses the concurrency concerns flagged in the previous review. The choice of mutex types is appropriate:openedFilesMutexfor simple map operations andallConditionsMutexfor read-heavy access patterns in parallel goroutines.
344-346: Proper lifecycle handling for file changes.Calling
didClosewhen files change ensures proper LSP lifecycle management and signals the server to release resources for the old version before re-querying.
356-358: Correct mutex usage for concurrent write protection.The
allConditionsMutexproperly protects the write toallConditions, preventing data races with concurrent readers in parallel goroutines.
699-734: Proper tracking of opened files to prevent duplicate notifications.The implementation correctly uses
openedFilesMutexto track opened files and avoid duplicatedidOpen/didClosenotifications. The pattern of checking state before RPC calls is appropriate.
764-772: Verify mixed response type handling.The normalization assumes all symbols in the response have the same shape (either all with
Locationor all without). If the server returns mixed types, symbols withoutLocationwill have unsetRangefields, which could cause issues downstream.Consider adding validation:
} else { documentSymbols := make([]protocol.DocumentSymbol, 0, len(symbols)) // typescript-language-server seems to return workspaceSymbol types even for document symbols // we need to normalize them back into document symbol types by copying the range for _, symbol := range symbols { if symbol.Location != nil { symbol.DocumentSymbol.Range = symbol.Location.Range + } else if symbol.DocumentSymbol.Range.Start.Line == 0 && symbol.DocumentSymbol.Range.End.Line == 0 { + sc.Log.V(7).Info("symbol missing both Location and Range", "symbol", symbol.Name) + continue } documentSymbols = append(documentSymbols, symbol.DocumentSymbol) }
984-1029: Enhanced symbol matching with name pattern validation.The updated logic now combines range overlap checking with name pattern matching (via
MatchSymbolByPatterns), providing more robust symbol identification. This reduces false positives when multiple symbols overlap the same location.
453-486: Unable to verify call sites due to repository access issues.The repository clone is failing, preventing verification of all
GetAllDeclarationscall sites. While the breaking signature change is clear (addition ofuseWorkspaceSymbol boolparameter), I cannot confirm that all callers have been updated to provide this new parameter.
504-506: Since I'm unable to access the repository to verify the callers, I'll provide a rewritten review comment based on the concern raised:Verify nil return value is properly handled by all callers of
GetAllReferences.This function now returns
nilon RPC failure (line 505) instead of an empty slice. Callers must check fornilto prevent nil pointer dereference. Confirm all call sites handle this return value appropriately.
174-174: Verify allGetLanguageIDimplementations have been updated to useuri.URIparameter.The
GetLanguageIDmethod signature changed from acceptingstringtouri.URI. This is a breaking interface change for all implementations of theSymbolSearchHelperinterface. Confirm that all implementations throughout the codebase have been updated to use the newuri.URIparameter type.
Signed-off-by: Pranav Gaikwad <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
lsp/base_service_client/base_service_client.go (6)
276-281: Initialization ofopenedFilesunder mutex is fine but mutex isn’t strictly needed hereYou correctly allocate
openedFilesbefore any goroutines are started; the extra lock/unlock is harmless but not necessary at construction time. Optional micro-cleanup only.
355-367: All-conditions guarded by RWMutex; WaitGroup usage looks mostly soundLocking
allConditionsduringPrepareand reading it underRLockin the workers avoids races. ThesymbolCacheUpdateWaitGrouppattern (Add before enqueue; Done inprocessSymbolCacheUpdateand in the URI producer goroutine) appears balanced.One subtlety:
GetAllDeclarationscan callWaitwhile other goroutines callAdd(e.g., viaNotifyFileChanges). If callers ever fireNotifyFileChangesafter they start querying,Waitmight return before those late additions complete. If that ordering matters for your use case, you may want a higher-level contract or a separate “prepare done” barrier.
736-786:queryDocumentSymbol: retries and normalization are good; silent nil result may hide persistent failuresPositives:
didOpenbefore requesting symbols helps temperamental servers.- Two attempts with a small delay should reduce flakiness.
- Normalizing TS servers that return workspace-symbol-shaped payloads into pure
DocumentSymboland caching them is pragmatic.Concern:
- On repeated failures or zero-symbol responses, the function returns
(nil, nil)and only logs at V(7). Callers can’t distinguish “no symbols” from “RPC or decode failed,” which might make debugging harder and changes prior error semantics.Consider returning the last error when all attempts fail:
- sc.Log.V(7).Info("textDocument/documentSymbol failed", "uri", uri, "error", lastErr) - return nil, nil + sc.Log.V(7).Info("textDocument/documentSymbol failed", "uri", uri, "error", lastErr) + return nil, lastErrand adjusting callers that intentionally want to ignore the error (e.g., treat it as “no symbols”) to do so explicitly.
805-911: Parallel line scanning and guarded access to shared state look safeThe new implementation of
searchContentForWorkspaceSymbols:
- Scans lines in parallel with a bounded semaphore (20) and uses
allConditionsMutex.RLockto safely read shared conditions.- Calls
queryDocumentSymbolat most once per file (per search) and guards the cachedsymbolsslice withsymbolsMutex.- Fixes the earlier early-return bug on
filepath.Absby logging and continuing, so a single bad path won’t drop all matches.Overall the function is more scalable and robust. If you ever want to be stricter about cancellation, you could also check
ctx.Err()inside the per-line goroutines before doing heavier work, but that’s optional.
913-983:getDefinitionForPosition: robust response-shape handling and retries; same comment on silent nilThe
unmarshalLocationshelper correctly covers the LSP variants for definition responses (LocationLink[],Location,Location[]) and normalizes them into a[]protocol.Location. Combined with:
didOpen+ file read before the request,- two attempts with a small delay,
- and logging on final failure,
this should improve reliability against both Node/TS and other servers.
As with
queryDocumentSymbol, though, the function returnsnilwithout an error in all failure modes except capability-missing, which blurs “no definition” vs “request failed repeatedly.” If callers ever need to differentiate those, consider returninglastErralongside a nil slice.
985-1030:findDocumentSymbolAtLocationmatches definitions to symbols using overlap + pattern and prefers the smallest rangeThis traversal combines:
- Range overlap against the definition location,
- Language-specific
MatchSymbolByPatternson the symbol name/container,- And a “shortest range wins” heuristic,
which is a sensible approach to pick the innermost matching symbol. Type assertions on
defSymbol.Location.Valueare safe given current call sites, and usingdocURIfor the workspace symbol location keeps URIs consistent.Just document the “shortest enclosing range” behavior somewhere if external consumers depend on which symbol is chosen when multiple overlap.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
-
lsp/base_service_client/base_service_client.go(19 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
🧬 Code graph analysis (1)
lsp/base_service_client/base_service_client.go (3)
lsp/protocol/tsprotocol.go (15)
URI(4673-4673)WorkspaceSymbol(5052-5063)Info(5477-5477)Location(2389-2392)Range(3559-3564)OrPLocation_workspace_symbol(2764-2766)BaseSymbolInformation(51-65)DidOpenTextDocumentParams(1100-1103)TextDocumentItem(4485-4495)DidCloseTextDocumentParams(1083-1086)TextDocumentIdentifier(4478-4481)Text(5341-5341)File(5582-5582)Position(3452-3466)DocumentURI(1460-1460)provider/provider.go (3)
ConditionsByCap(472-475)Location(319-322)Position(324-342)lsp/base_service_client/symbol_cache.go (1)
WorkspaceSymbolDefinitionsPair(27-30)
⏰ 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). (9)
- GitHub Check: test
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
🔇 Additional comments (7)
lsp/base_service_client/base_service_client.go (7)
150-155: Good: shared state for opened files and conditions now mutex-protectedAdding
openedFilesMutexandallConditionsMutexaround shared maps/slices addresses prior race concerns and makes concurrent access from cache-population goroutines safe.
330-350: Closing documents on file change is reasonable; relies on consistenturimappingCalling
didClosebefore invalidating and re-populating the symbol cache should help TS/Node servers reset state for modified files. This assumes every URI passed here matches what was used indidOpen(viatoURI), otherwise the close will be skipped silently by theopenedFilesmap. That’s probably fine, but worth keeping in mind if you later introduce alternative URI construction paths.
523-528: Symbol cache population: concurrency, keying, and fallbacks look correctThe new
workspaceSymbolKeyand parallel definition lookups (bounded by a semaphore of 10) are a solid improvement:
- Type assertions on
WorkspaceSymbol.Location.Valueare only applied to symbols constructed in this function, so they’re safe as long as future callers keep that invariant.- The
resultschannel +wgpattern avoids goroutine leaks and processes only symbols with actual definitions.- When
findDocumentSymbolAtLocationcan’t improve a definition, you correctly fall back to the synthetic workspace symbol.All of this should make cache population faster and more resilient without obviously changing semantics.
Also applies to: 535-551, 553-582, 583-642
168-188: I'll help you verify that all implementations ofSymbolSearchHelper.GetLanguageIDwere updated to accept theuri.URIparameter. Let me search the codebase for all implementations.
<function_calls>
#!/bin/bashSearch for all GetLanguageID method implementations in Go files
rg -n "GetLanguageID(" --type go -A 2 -B 2
</function_calls>
#!/bin/bashAlso search for types that might implement SymbolSearchHelper
rg -n "type.*SymbolSearchHelper" --type go -A 5 -B 2
</function_calls><function_calls>
#!/bin/bashFind all receiver types that have GetLanguageID method
ast-grep --pattern $'func ($_
$_ ) GetLanguageID( $ $$ ) {
$$$
}'
</function_calls>Let me also find the SymbolSearchHelper interface definition and check the broader context:
<function_calls>
#!/bin/bashFind the SymbolSearchHelper interface definition
rg -n "type SymbolSearchHelper" --type go -A 30
</function_calls>
699-734: I'm unable to verify the review comment at this time due to persistent repository access issues. However, let me provide the rewritten review comment based on the limitation:
didOpen/didClosewithopenedFilesdedupe align better with LSP expectations, butsymbolSearchHelpernil-safety requires verificationTracking opened URIs and short-circuiting duplicate opens/closes should reduce noisy LSP notifications and avoid TS "No project" issues. Using
uri.URIeverywhere for the key keeps it consistent withtoURI.
⚠️ Verification needed: The code callssc.symbolSearchHelper.GetLanguageID(uri)indidOpenwithout nil checks. Confirm thatsymbolSearchHelperis always initialized inNewLSPServiceClientBaseand never set to nil after initialization, otherwise this will panic at runtime.
453-487: Yes, please search for both:
- LSPServiceClientBase.GetAllDeclarations - to find all call sites and see what boolean values are being passed for the
useWorkspaceSymbolparameter- useWorkspaceSymbol - to understand the usage patterns and see if there are any inconsistencies
This will help verify whether callers are correctly passing the flag as the review comment suggests.
489-508: Based on my research, I can now provide a complete verification of the review comment.Key findings:
LSP Protocol Contract: The LSP specification explicitly allows
Location[] | nullfor thetextDocument/referencesresponse, meaning returningnull(which nil marshals to) is protocol-compliant.Go Idiom: According to Go best practices, when a function encounters an error, returning
nilfor a slice (the zero value) is idiomatic and standard. This follows the pattern: "on error, return nil, err."JSON Encoding: In Go's
encoding/json:
nilslice → JSONnull[]T{}→ JSON[]Both are valid per the LSP spec.
Caller Impact: The concern about callers checking
== nilvslen(...) == 0is valid but minor:
rangeandlen()work identically for both nil and empty slices- Only explicit
== nilchecks or JSON marshalling behavior would differ- This is a real behavior change if the previous code returned
[]protocol.Location{}Conclusion: The reviewer's concern is valid but nuanced. The change follows Go idioms and LSP protocol specs. However, without verifying the previous implementation, we cannot definitively say if this is an oversight or intentional refactoring. The suggested fix (returning empty slice) would also be acceptable and would preserve prior semantics if callers were checking for nil.
Behavior change: on error,
GetAllReferencesnow returnsnilinstead of an empty sliceLogging the failure is good. Returning
nilaligns with Go idioms (nil being the zero value for error cases) and is valid per the LSP specification, which allowsLocation[] | null. However, this is a behavioral change: callers explicitly checking== nilwill see different results than before. If prior behavior returned[]protocol.Location{}, consider preserving semantics for compatibility:- sc.Log.Error(err, "textDocument/references request failed", "uri", location.URI) - return nil + sc.Log.Error(err, "textDocument/references request failed", "uri", location.URI) + return []protocol.Location{}
shawn-hurley
left a comment
shawn-hurley
left a comment
There was a problem hiding this comment.
Nits can be follow one if this helps get ci stable
| } | ||
| params.WorkspaceFolders = workspaceFolders | ||
|
|
||
| params.Capabilities = protocol.ClientCapabilities{ |
There was a problem hiding this comment.
Either here or a follow up can note what each of these does and why we need them?
| ".go": "go", | ||
| } | ||
| return languageIDMap[filepath.Ext(filepath.Base(uri))] | ||
| if strings.HasPrefix(string(u), fmt.Sprintf("%s://", uri.FileScheme)) { |
There was a problem hiding this comment.
Could use the same constant above?
Signed-off-by: Pranav Gaikwad <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (3)
49-55: Location → workspace folder normalization looks correct; consider more robust URI construction.Treating
c.Locationas the single workspace folder and ensuring afile://prefix aligns with the PR goal. The manual prefixing assumesc.Locationis a filesystem path; for cross‑platform robustness you might instead resolve to an absolute path and construct the URI viauri.File(and optionally handle errors):if c.Location != "" { if !strings.HasPrefix(c.Location, "file://") { abs, err := filepath.Abs(c.Location) if err != nil { return nil, fmt.Errorf("resolving location %q: %w", c.Location, err) } c.Location = uri.File(abs).String() } sc.Config.WorkspaceFolders = []string{c.Location} }Also worth double‑checking that overwriting any YAML‑provided
WorkspaceFolderswhenLocationis set is the intended behavior for Node.
68-81: WorkspaceFolders construction/dedup is solid; name derivation could lean on URI helpers.The deduplication map and conversion into
protocol.WorkspaceFolderlook good and should avoid redundant folders. For theNamefield, instead of manually stripping"file://"and then callingfilepath.Base, you could derive the path via the URI helper for clarity and fewer string assumptions, e.g.:u, err := uri.Parse(f) if err == nil { name := filepath.Base(u.Filename()) workspaceFolders = append(workspaceFolders, protocol.WorkspaceFolder{ URI: f, Name: name, }) } else { // fallback to current behavior or log }This keeps the logic aligned with how the rest of the code parses URIs and is friendlier to any future non‑file schemes.
176-178: NewGetAllDeclarationsflag: confirm semantics and consider making intent self‑documenting.Passing
falsefor the newGetAllDeclarationsboolean aligns with the PR intent to avoid flaky workspace‑wide symbol queries for Node, but the meaning of the flag isn’t obvious at the call site. Two follow‑ups to consider:
- Confirm that
falseindeed selects the “no workspace/symbol (or no references)” path you want for Node.- To improve readability, consider a named constant or options struct so the call reads more like
GetAllDeclarations(ctx, query, useWorkspaceSymbol /* false */)or similar, instead of a bare boolean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 975
File: external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go:29-78
Timestamp: 2025-11-21T14:12:52.228Z
Learning: In the analyzer-lsp codebase, additional workspace folders (additionalPaths) used in FileSearcher are already vetted and scoped, so they do not require additional exclusion patterns for directories like node_modules, .git, dist, etc. Only the primary path needs these exclusions.
⏰ 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). (9)
- GitHub Check: test
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (ubuntu-latest, linux)
🔇 Additional comments (2)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (2)
7-7: Import ofpath/filepathis appropriate and scoped.The new
filepathimport is used only for deriving workspace folder names and is a reasonable choice; no issues here.
82-105: Capabilities block matches stated needs; verify behavior against the Node language server.Enabling workspace folders, diagnostics refresh, definition
LinkSupport, and hierarchical document symbols is consistent with the PR description and should give you richer symbol info (andLocationLinks) needed for reliable Node/TS analysis.Since this is tightly coupled to LSP server behavior, please verify via integration/E2E that:
- The Node language server returns
LocationLink[]fortextDocument/definitiongivenLinkSupport: true.textDocument/documentSymbolreturns hierarchical symbols as expected.- You are not accidentally re‑enabling
workspace/symbolfor Node (i.e., noWorkspaceSymbolcapability advertised here or elsewhere for this client).
2 participants
Summary by CodeRabbit
Refactor
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.