✨ Add Prepare() function to providers to run pre-evaluation warmup #975
Conversation
…s are correctly configured Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
…Changes() to grpc server Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
… that in node provider Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
WalkthroughThe PR adds a provider preparation phase: rule loading now extracts per-provider conditions, the analyzer accumulates them, and invokes a new Prepare(ctx, conditions) method on each provider before engine initialization. Supporting changes include symbol caching, a Node.js-specific symbol search helper, file-exclusion-aware directory walking, and proto/GRPC updates for Prepare/NotifyFileChanges. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Analyzer Main
participant Parser as Rule Parser
participant ProvMgr as Provider Manager
participant Provider as Provider (each)
participant Engine as Rule Engine
Main->>Parser: LoadRules(filepath)
activate Parser
Parser-->>Main: (ruleSets, clients, provConditions, err)
deactivate Parser
Main->>ProvMgr: ProviderInit(clients)
ProvMgr-->>Main: initialized clients
rect rgb(230,245,255)
Note over Main,Provider: NEW — Provider Prepare Phase
Main->>Main: aggregate provConditions per provider
loop for each provider
Main->>Provider: Prepare(ctx, conditions)
alt prepare success
Provider-->>Main: nil
else prepare error
Provider-->>Main: error (logged, continue)
end
end
end
Main->>Engine: Initialize(ruleSets)
Engine-->>Main: ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas to focus review on:
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]>
…nces, query document symbols Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
pranavgaikwad
force-pushed
the
feature/preEvaluationWarmup
branch
from
b67a85a to
60c19c7
Compare
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
| variables: | ||
| file: file:///examples/nodejs/test_b.ts | ||
| effort: 1 | ||
| node-sample-rule-003: |
There was a problem hiding this comment.
This is correctly matching now for query greeter matching on greeter.hello()
| lineNumber: 1 | ||
| variables: | ||
| file: file:///examples/nodejs/Component.tsx | ||
| - uri: file:///examples/nodejs/LegacyComponent.jsx |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
parser/rule_parser.go (1)
615-784: getConditions: providerConditions is collected but never returned or mergedIn
getConditionsyou:
- Introduce
providerConditions := map[string][]provider.ConditionsByCap{}(Line 618).- Call
mergeProviderConditionswhen encountering a provider condition (Lines 750-753).- Still return only
([]engine.ConditionEntry, map[string]provider.InternalProviderClient, error)and ignoreproviderConditionsin the finalreturn conditions, providers, nil(Line 783).As a result:
- Work done in
mergeProviderConditionshere is effectively dead; only logs are observable.- Nested
and/orconditions never contribute to theproviderConditionsmap returned byLoadRule, so providers miss those conditions during Prepare.Consider one of the following:
Propagate nested providerConditions (strongly recommended):
- Change the signature to include the map:
func (r *RuleParser) getConditions( conditionsInterface []interface{}, ) ([]engine.ConditionEntry, map[string]provider.InternalProviderClient, map[string][]provider.ConditionsByCap, error)- Return
providerConditionsfromgetConditions.- In
LoadRule’sand/orbranches, capture and merge:conds, provs, nestedProvConds, err := r.getConditions(iConditions) // merge nestedProvConds into this function’s providerConditions mapThis gives providers a complete picture of conditions, regardless of nesting.
Or, if nested conditions are intentionally ignored for Prepare:
- Remove
providerConditionsfromgetConditionsentirely (the local map andmergeProviderConditionscalls), to avoid confusion about unused state.Right now it’s in an in‑between state that’s easy to misinterpret and doesn’t fully support the new Prepare behavior.
🧹 Nitpick comments (13)
provider/lib.go (2)
57-63: Exclude precedence wiring looks correct but split uses raw pathsUsing rule-level excludes when present and otherwise falling back to provider-level is consistent with the existing priority model used later in the function for filtering. The only caveat is that
splitPathsAndPatternshere operates on the raw constraint strings (without joining toBasePath), so relative defaults like"node_modules"will only be classified as directories if they exist relative to the process working directory, not the workspace base. That just affects how much the walker can prune early; final exclusion filtering still uses the full patterns and remains correct.If you want directory pruning to work reliably for relative excludes, consider normalizing them against
f.BasePathbefore/insidesplitPathsAndPatternsin this call site.
285-335: Normalize absolute excludedDirs and precompile exclude patterns for WalkDirThe new
cachedWalkDirimplementation is generally sound, but there are a couple of edge cases and perf considerations worth tightening:
excludedDirsvsrelPath:
- Inside the walker you compute
relPath := filepath.Rel(basePath, path)and compare it directly to eachexcludedDir.GetExcludedDirsFromConfig(lines 439-483) constructs absolute paths for user-specified excludes, so when those end up inExcludePathsOrPatterns→excludedDirs, therelPath == excludedDir || strings.HasPrefix(relPath, excludedDir)check will never fire for those entries. Those directories will still be excluded later byfilterFilesByPathsOrPatterns, but they won’t be pruned during traversal.- Normalizing
excludedDirsrelative tobasePathbefore the walk would make both default relative entries (e.g."node_modules") and absolute config entries work consistently at traversal time.Regex compilation inside the callback:
regexp.Compile(excludedPattern)is executed on every directory visit for every pattern; this is unnecessarily expensive on large trees.- Precompiling the subset of patterns you want to treat as regex once per
basePathkeeps the callback lightweight.Cache key ignores excludes:
- Today
newCachedWalkDiris only used insideSearch, andexcludedDirs/excludedPatternsare constant for all calls within a singleSearchinvocation, so caching solely onbasePathis safe.- If this helper gets re-used with varying exclude sets for the same
basePath, the cache will return results computed with the first exclude set. It’s worth documenting this assumption or including the exclude slices in the cache key if you expect broader reuse.A possible refactor for the walker normalization and regex precompilation (keeps behavior but makes excludes effective for absolute dirs and more efficient):
type cachedWalkDir func(path string, excludedDirs []string, excludedPatterns []string) ([]string, error) func newCachedWalkDir() cachedWalkDir { cache := make(map[string]struct { files []string err error }) - return func(basePath string, excludedDirs []string, excludedPatterns []string) ([]string, error) { + return func(basePath string, excludedDirs []string, excludedPatterns []string) ([]string, error) { val, ok := cache[basePath] if !ok { files := []string{} - err := filepath.WalkDir(basePath, func(path string, d fs.DirEntry, err error) error { + // Normalize excludedDirs relative to basePath so absolute config entries are effective. + relExcludedDirs := make([]string, 0, len(excludedDirs)) + for _, ed := range excludedDirs { + if filepath.IsAbs(ed) { + if rel, err := filepath.Rel(basePath, ed); err == nil { + relExcludedDirs = append(relExcludedDirs, rel) + continue + } + } + relExcludedDirs = append(relExcludedDirs, ed) + } + + // Precompile excludedPatterns we want to treat as regex. + var excludedRegexps []*regexp.Regexp + for _, p := range excludedPatterns { + if !strings.Contains(p, "*") { + continue + } + if rx, err := regexp.Compile(p); err == nil { + excludedRegexps = append(excludedRegexps, rx) + } + } + + err := filepath.WalkDir(basePath, func(path string, d fs.DirEntry, err error) error { if err != nil { return err } if !d.IsDir() { files = append(files, path) return nil } - // do not recurse into excluded dirs - for _, excludedDir := range excludedDirs { - relPath, err := filepath.Rel(basePath, path) - if err == nil && (relPath == excludedDir || strings.HasPrefix(relPath, excludedDir)) { - return fs.SkipDir - } - } - for _, excludedPattern := range excludedPatterns { - if !strings.Contains(excludedPattern, "*") { - continue - } - regex, err := regexp.Compile(excludedPattern) - if err != nil { - continue - } - if regex.MatchString(path) || regex.MatchString(filepath.Base(path)) { - return fs.SkipDir - } - } + // do not recurse into excluded dirs + relPath, err := filepath.Rel(basePath, path) + if err != nil { + return err + } + for _, ed := range relExcludedDirs { + if relPath == ed || strings.HasPrefix(relPath, ed) { + return fs.SkipDir + } + } + for _, rx := range excludedRegexps { + if rx.MatchString(path) || rx.MatchString(filepath.Base(path)) { + return fs.SkipDir + } + } return nil }) cache[basePath] = struct { files []string err error }{ files: files, err: err, } return files, err } return val.files, val.err } }This keeps the public surface the same while making configuration-driven excludes more effective at the traversal stage and reducing per-entry overhead inside the walker.
Also applies to: 439-483
lsp/base_service_client/symbol_cache.go (1)
225-265: Minor readability issue: reusedconditionidentifier in nested loopsIn both
MatchFileContentByConditionsandMatchSymbolByConditions, the inner loop reuses theconditionidentifier from the outer loop:for _, condition := range conditionsByCap { if condition.Cap == "referenced" { for _, condition := range condition.Conditions { // ... } } }This shadowing doesn’t break correctness but makes the code harder to read and reason about. Renaming the inner variable (e.g., to
rawCondordata) would improve clarity with no behavior change.provider/grpc/service_client.go (1)
201-225: Prepare RPC mapping looks correct; consider minor allocation tweaksThe mapping from
[]provider.ConditionsByCapto[]*pb.ConditionsByCapabilityand the error handling aroundPrepareare consistent with the existingEvaluatepath and look correct.You could shave a couple of allocations by pre-allocating the slices, e.g.:
func (g *grpcServiceClient) Prepare(ctx context.Context, conditionsByCap []provider.ConditionsByCap) error { - conditionsByCapability := []*pb.ConditionsByCapability{} - for _, condition := range conditionsByCap { - conditions := []string{} + conditionsByCapability := make([]*pb.ConditionsByCapability, 0, len(conditionsByCap)) + for _, condition := range conditionsByCap { + conditions := make([]string, 0, len(condition.Conditions)) for _, conditionInfo := range condition.Conditions { conditions = append(conditions, string(conditionInfo)) }Not required for correctness, just a small optimization if this runs on many rules.
parser/rule_parser_test.go (1)
23-26: Test provider now satisfiesPrepare; consider exercising provider-conditions in testsThe no-op
PrepareontestProvidercorrectly satisfies the updatedInternalProviderClientinterface, and updating theLoadRulescall toruleSets, clients, _, err := ...keeps existing assertions working while ignoring the new return value.To get coverage of the new provider-conditions / preparation path, you might later add a focused test that:
- Captures the third return from
LoadRules, and- Uses a
testProviderthat recordsconditionsByCappassed toPrepare, so you can assert that the parser builds and wires conditions as expected.Not required for this PR, but would lock in the new behavior.
Also applies to: 828-828
provider/server.go (1)
532-579: Prepare/NotifyFileChanges RPC wiring looks correct; consider guarding missing client IDsThe conversions from gRPC types to
ConditionsByCapandFileChangeand delegation toclient.client.Prepare/NotifyFileChangesare consistent with the rest of the server and look fine.One potential improvement:
client := s.clients[in.Id]silently returns the zero value if the ID is unknown, which will panic onclient.client.Prepare/NotifyFileChanges. For robustness (and nicer errors if the client was already stopped), consider checking theokfrom the map lookup and returning a gRPC error when the client is not found.cmd/analyzer/main.go (1)
293-350: Provider conditions aggregation and Prepare orchestration look good; clarify error handling semanticsThe new
providerConditionsmap populated fromprovConditionsand the subsequent loop:
- Aggregates per-provider
[]provider.ConditionsByCapcorrectly by appendingv....- Only calls
Prepare(ctx, conditions)for providers actually present inneedProviders, which aligns with rules-driven usage.- Invokes
Prepareafter allProviderInitcalls (including builtin), matching the intended lifecycle.Two minor points to consider:
Duplicate conditions: If the same rule file is loaded multiple times or multiple rules contribute identical conditions for a provider/capability, you will send duplicates to
Prepare. If this is undesirable, you might want to deduplicate per(provider, cap, condition)before callingPrepare.Non-fatal Prepare errors: Right now,
Preparefailures are only logged. If some providers depend heavily on warmup (e.g., Node.js symbol cache), you may want to make failures for those providers fatal (or at least expose them via progress reporting) so callers understand why later evaluations might be incomplete.external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go (1)
29-73: GetDocumentUris is functionally correct; consider leveraging conditions and normalizing base path
GetDocumentUriscorrectly:
- Normalizes
config.Locationby strippingfile://when present.- Collects additional roots from
workspaceFoldersinProviderSpecificConfig.- Uses
FileSearcherwith JS/TS includes and sensible default excludes (git, dist, vscode, husky, node_modules).- Returns
file://URIs viauri.File(file).Two minor improvements you might consider:
- Use
primaryPathfor excludes as wellIf
InitConfig.Locationis ever afile://URI,filepath.Join(h.config.Location, ".git")etc. will not match real filesystem paths. Using the normalizedprimaryPathwould be more robust:- ExcludePathsOrPatterns: []string{ - filepath.Join(h.config.Location, ".git"), - filepath.Join(h.config.Location, "dist"), - filepath.Join(h.config.Location, ".vscode"), - filepath.Join(h.config.Location, ".husky"), - ".*node_modules.*", - }, + ExcludePathsOrPatterns: []string{ + filepath.Join(primaryPath, ".git"), + filepath.Join(primaryPath, "dist"), + filepath.Join(primaryPath, ".vscode"), + filepath.Join(primaryPath, ".husky"), + ".*node_modules.*", + },
- Optionally narrow the search using rule conditions
The
conditionsByCap ...provider.ConditionsByCapparameter is currently unused. If performance becomes an issue on very large workspaces, you could parse thereferencedpatterns here (viaparseQuery) to selectively add search roots (e.g.,node_modules/@pkg/scope) instead of always scanning all source directories.lsp/base_service_client/base_service_client.go (2)
120-158: Symbol cache integration and SymbolSearchHelper wiring look solid; be mindful of WaitGroup semanticsThe additions to
LSPServiceClientBase:
symbolCache,symbolSearchHelper,symbolCacheUpdateChan,symbolCacheUpdateWaitGroup, andallConditions.SymbolSearchHelperinterface and its injection viaNewLSPServiceClientBase.- Initialization of default helper when none is passed, plus background
symbolCacheUpdateHandler.are all consistent and keep the base client generic while enabling Node.js (and other languages) to customize symbol discovery and matching.
A couple of points to keep in mind:
- WaitGroup used across Prepare/NotifyFileChanges and GetAllDeclarations
PrepareandNotifyFileChangesboth callsymbolCacheUpdateWaitGroup.Add(...), whileGetAllDeclarationscallssymbolCacheUpdateWaitGroup.Wait()before reading from the cache. This guarantees that any currently queued updates complete beforeGetAllDeclarationsfalls back to the cache, but:
- New updates scheduled after
Wait()begins will not be waited on, so a caller cannot rely onGetAllDeclarationsto always see a fully up‑to‑date cache.AddandWaitcan run concurrently, which is discouraged in thesync.WaitGroupdocs and can make reasoning about ordering harder.If you need strong guarantees that
GetAllDeclarationsonly sees a fully prepared cache (for the lastPrepare), consider either:
- Using a separate
WaitGroupperPrepareinvocation and waiting inside the orchestration layer (e.g., in the analyzer main) instead of insideGetAllDeclarations, or- Guarding
AddandWaitwith a dedicated mutex so they never run concurrently, keeping updates and reads well ordered.
- SymbolSearchHelper lifetime
Because
NewLSPServiceClientBasealways assigns asymbolSearchHelper(either injected or default) before starting the cache goroutine, all internal uses (GetLanguageID,MatchFileContentByConditions, etc.) are safe. Just ensure any future call sites outside this constructor also follow the “helper or default” pattern.Overall, the wiring and caching strategy look good; the main thing is being explicit about the ordering guarantees you need from the WaitGroup and adjusting if stronger guarantees are required.
Also applies to: 142-152, 160-186, 187-196, 257-266, 314-348
314-333: Document symbol cache population pipeline is coherent; small robustness nitsThe new document-symbol pipeline (
NotifyFileChanges,populateDocumentSymbolCache,symbolCacheUpdateHandler,queryDocumentSymbol,searchContentForWorkspaceSymbols,getDefinitionForPosition, and helpers) is well thought out:
- File changes invalidate per‑file cache entries and enqueue URIs for refresh.
populateDocumentSymbolCache:
- Reads file content.
- Uses
searchContentForWorkspaceSymbolsto find rule matches.- For each match, resolves definitions via
textDocument/definitionand buildsWorkspaceSymbolDefinitionsPairvalues keyed by location.queryDocumentSymbol:
- Properly opens/closes documents via
didOpen/didCloseand cachesDocumentSymbols by URI.searchContentForWorkspaceSymbols:
- Uses per‑line scanning and
MatchFileContentByConditionsto locate matches, then finds the nearestDocumentSymbolor synthesizes a fallback symbol.A few minor robustness suggestions:
- Content vs on‑disk state
Both
populateDocumentSymbolCacheandgetDefinitionForPositionread files from disk (os.ReadFile(uri.Filename())). IfNotifyFileChangesis ever called with unsaved content (i.e., editor buffer not yet flushed), the cache will lag slightly behind the in‑memory state. If that matters for your use cases, consider optionally usingprovider.FileChange.ContentwhenSaved == falseinstead of re‑reading from disk inNotifyFileChanges/populateDocumentSymbolCache.
- Scanner line limits
bufio.Scannerhas a default token limit (~64K). If Node.js projects include extremely long lines,searchContentForWorkspaceSymbolsmay silently drop content. You can bump the buffer size withscanner.Buffer(make([]byte, 0, 64*1024), maxTokenSize)if this proves to be an issue.
- Graceful degredation on symbolSearchHelper errors
You already log and continue when
didOpen/textDocument/documentSymbol/didClosefail. That’s good. Just ensure thatsymbolSearchHelper.GetLanguageIDimplementations never panic on unknown URIs; if needed, they should fall back to a default language ID.These are all non-blocking; the overall design for symbol cache population and use is sound.
Also applies to: 479-577, 590-620, 622-677, 679-746, 748-773, 775-846
parser/rule_parser.go (3)
91-192: LoadRules: providerConditions aggregation looks correct but merge blocks are duplicatedThe new
providerConditionsreturn and its aggregation across recursiveLoadRulescalls is consistent with howclientMapis handled and should behave correctly for both single-file and directory trees. The twofor k, v := range provConditionsblocks (directory vs. regular file cases) are effectively identical, though; consider extracting a tiny helper likemergeProvConditions(dst, src)to avoid drift if this logic changes again.
194-492: LoadRule: providerConditions only capture top-level provider conditionsThe
providerConditionsmap here is only updated in the default branch (direct{provider}.{capability}in the top-levelwhen), so any provider references that live insideand/orclauses are not reflected in the returnedproviderConditions. Given this PR’s goal (pre‑evaluation warmup per provider), that means Prepare will miss some conditions and only see a subset of the rules’ provider usage.If this is intentional (e.g., you only care about simple top‑level conditions for warmup), it would be good to document that assumption; otherwise, you’ll likely want to also aggregate conditions coming from nested
and/orstructures viagetConditions(see next comment), so providers get a complete view of their usage.
871-892: mergeProviderConditions: behavior is fine, consider coalescing by capabilityThe helper correctly:
- Serializes
{capability: value}to YAML.- Appends a
provider.ConditionsByCapentry underproviderConditions[providerKey].Two minor points to consider:
- Multiple conditions for the same
providerKey+capabilitywill produce multipleConditionsByCapentries with identicalCapvalues, each wrapping a single[][]byte. If providers would find it simpler to process one entry per capability, you could instead detect an existingConditionsByCapfor thatcapand append to itsConditionsslice.- YAML marshal failures are logged and swallowed, which is acceptable, but if missing condition info in Prepare would be problematic, you may want to surface that as an error rather than only logging it.
These are non‑blocking and mostly about shaping the data in a provider‑friendly way.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
examples/nodejs/package-lock.jsonis excluded by!**/package-lock.jsonexternal-providers/generic-external-provider/go.sumis excluded by!**/*.sumprovider/internal/grpc/library.pb.gois excluded by!**/*.pb.goprovider/internal/grpc/library_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (31)
cmd/analyzer/main.go(2 hunks)demo-local.Dockerfile(1 hunks)demo-output.yaml(3 hunks)examples/nodejs/package.json(1 hunks)external-providers/dotnet-external-provider/pkg/dotnet/provider.go(1 hunks)external-providers/dotnet-external-provider/pkg/dotnet/service_client.go(1 hunks)external-providers/generic-external-provider/go.mod(2 hunks)external-providers/generic-external-provider/pkg/server_configurations/generic/service_client.go(1 hunks)external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go(5 hunks)external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go(1 hunks)external-providers/generic-external-provider/pkg/server_configurations/pylsp/service_client.go(1 hunks)external-providers/generic-external-provider/pkg/server_configurations/yaml_language_server/service_client.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/provider.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(1 hunks)external-providers/yq-external-provider/pkg/yq_provider/provider.go(1 hunks)external-providers/yq-external-provider/pkg/yq_provider/service_client.go(1 hunks)jsonrpc2_v2/conn.go(2 hunks)lsp/base_service_client/base_capabilities.go(2 hunks)lsp/base_service_client/base_service_client.go(5 hunks)lsp/base_service_client/symbol_cache.go(1 hunks)parser/rule_parser.go(27 hunks)parser/rule_parser_test.go(2 hunks)provider/grpc/provider.go(1 hunks)provider/grpc/service_client.go(1 hunks)provider/internal/builtin/provider.go(1 hunks)provider/internal/builtin/service_client.go(1 hunks)provider/internal/grpc/library.proto(2 hunks)provider/lib.go(8 hunks)provider/provider.go(2 hunks)provider/provider_test.go(1 hunks)provider/server.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-21T16:27:59.216Z
Learnt from: shawn-hurley
Repo: konveyor/analyzer-lsp PR: 860
File: provider/server.go:251-259
Timestamp: 2025-10-21T16:27:59.216Z
Learning: In provider/server.go, the Init method uses context.Background() for creating the RPC client via GetProviderRPCClient because this RPC client is stored in the InitConfig and used across multiple subsequent requests (Evaluate, GetDependencies, etc.). The RPC client's lifecycle is managed separately through explicit Stop calls, not tied to the Init request's context.
Applied to files:
external-providers/generic-external-provider/pkg/server_configurations/pylsp/service_client.goprovider/grpc/provider.goprovider/grpc/service_client.goexternal-providers/generic-external-provider/pkg/server_configurations/generic/service_client.goexternal-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
📚 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:
external-providers/generic-external-provider/pkg/server_configurations/pylsp/service_client.goexternal-providers/generic-external-provider/pkg/server_configurations/generic/service_client.goexternal-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.golsp/base_service_client/base_service_client.go
📚 Learning: 2025-07-30T12:11:45.673Z
Learnt from: pranavgaikwad
Repo: konveyor/analyzer-lsp PR: 859
File: external-providers/java-external-provider/pkg/java_external_provider/dependency.go:694-694
Timestamp: 2025-07-30T12:11:45.673Z
Learning: In the Java external provider dependency walker (external-providers/java-external-provider/pkg/java_external_provider/dependency.go), errors from toDependency function calls should be ignored as they are not considered important by the maintainers.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/provider.go
🧬 Code graph analysis (20)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
provider/provider.go (1)
ConditionsByCap(472-475)
external-providers/dotnet-external-provider/pkg/dotnet/provider.go (1)
provider/provider.go (1)
ConditionsByCap(472-475)
external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (1)
provider/provider.go (1)
ConditionsByCap(472-475)
provider/internal/builtin/service_client.go (1)
provider/provider.go (1)
ConditionsByCap(472-475)
external-providers/yq-external-provider/pkg/yq_provider/service_client.go (1)
provider/provider.go (1)
ConditionsByCap(472-475)
provider/grpc/provider.go (1)
provider/provider.go (2)
ConditionsByCap(472-475)FullPrepareResponse(433-441)
provider/provider_test.go (1)
provider/provider.go (1)
ConditionsByCap(472-475)
provider/grpc/service_client.go (2)
provider/provider.go (1)
ConditionsByCap(472-475)provider/internal/grpc/library.pb.go (6)
ConditionsByCapability(1642-1648)ConditionsByCapability(1661-1661)ConditionsByCapability(1676-1678)PrepareRequest(1694-1700)PrepareRequest(1713-1713)PrepareRequest(1728-1730)
provider/internal/builtin/provider.go (1)
provider/provider.go (1)
ConditionsByCap(472-475)
parser/rule_parser_test.go (1)
provider/provider.go (1)
ConditionsByCap(472-475)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (3)
provider/provider.go (5)
Config(88-98)ProviderContext(349-354)Location(319-322)IncidentContext(309-317)Position(324-342)external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go (1)
NewNodejsSymbolCacheHelper(173-179)lsp/base_service_client/base_service_client.go (2)
LSPServiceClientBase(120-158)NewLspServiceClientEvaluator(77-94)
lsp/base_service_client/base_capabilities.go (1)
provider/provider.go (1)
ProviderContext(349-354)
external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)
provider/provider.go (1)
ConditionsByCap(472-475)
cmd/analyzer/main.go (1)
provider/provider.go (1)
ConditionsByCap(472-475)
external-providers/yq-external-provider/pkg/yq_provider/provider.go (1)
provider/provider.go (1)
ConditionsByCap(472-475)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go (5)
lsp/base_service_client/base_service_client.go (1)
SymbolSearchHelper(165-185)provider/provider.go (2)
InitConfig(123-159)ConditionsByCap(472-475)provider/lib.go (3)
FileSearcher(19-28)IncludeExcludeConstraints(38-41)SearchCriteria(33-36)lsp/protocol/tsprotocol.go (2)
Pattern(3424-3424)WorkspaceSymbol(5052-5063)lsp/base_service_client/symbol_cache.go (2)
WorkspaceSymbolDefinitionsPair(27-30)NewDefaultSymbolCacheHelper(267-272)
lsp/base_service_client/base_service_client.go (3)
lsp/base_service_client/symbol_cache.go (4)
SymbolCache(20-25)WorkspaceSymbolDefinitionsPair(27-30)NewDocumentSymbolCache(32-37)NewDefaultSymbolCacheHelper(267-272)lsp/protocol/tsprotocol.go (20)
URI(4673-4673)Log(5479-5479)WorkspaceSymbol(5052-5063)WorkspaceSymbolParams(5095-5101)Location(2389-2392)Range(3559-3564)Info(5477-5477)OrPLocation_workspace_symbol(2764-2766)DocumentURI(1460-1460)BaseSymbolInformation(51-65)DidOpenTextDocumentParams(1100-1103)TextDocumentItem(4485-4495)Text(5341-5341)DidCloseTextDocumentParams(1083-1086)TextDocumentIdentifier(4478-4481)DocumentSymbol(1388-1413)DocumentSymbolParams(1448-1453)File(5582-5582)Position(3452-3466)TextDocumentPositionParams(4499-4504)provider/provider.go (5)
ConditionsByCap(472-475)InitConfig(123-159)FileChange(466-470)Location(319-322)Position(324-342)
provider/server.go (2)
provider/internal/grpc/library.pb.go (15)
PrepareRequest(1694-1700)PrepareRequest(1713-1713)PrepareRequest(1728-1730)PrepareResponse(1746-1751)PrepareResponse(1764-1764)PrepareResponse(1779-1781)NotifyFileChangesRequest(1546-1552)NotifyFileChangesRequest(1565-1565)NotifyFileChangesRequest(1580-1582)NotifyFileChangesResponse(1598-1603)NotifyFileChangesResponse(1616-1616)NotifyFileChangesResponse(1631-1633)FileChange(1486-1493)FileChange(1506-1506)FileChange(1521-1523)provider/provider.go (2)
ConditionsByCap(472-475)FileChange(466-470)
parser/rule_parser.go (2)
engine/conditions.go (2)
RuleSet(93-99)Rule(101-107)provider/provider.go (3)
InternalProviderClient(450-453)ConditionsByCap(472-475)Capability(82-86)
lsp/base_service_client/symbol_cache.go (5)
lsp/protocol/tsprotocol.go (5)
DocumentSymbol(1388-1413)WorkspaceSymbol(5052-5063)Range(3559-3564)SelectionRange(3778-3783)Pattern(3424-3424)provider/provider.go (3)
InitConfig(123-159)ConditionsByCap(472-475)ProviderContext(349-354)lsp/base_service_client/base_capabilities.go (1)
ReferencedCondition(30-35)provider/lib.go (3)
FileSearcher(19-28)IncludeExcludeConstraints(38-41)SearchCriteria(33-36)lsp/base_service_client/base_service_client.go (1)
SymbolSearchHelper(165-185)
⏰ 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). (5)
- GitHub Check: test
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
🔇 Additional comments (24)
examples/nodejs/package.json (1)
1-7: Minimal, well-structured Node.js example workspace.The dependency versions are sound and follow current best practices. React 19.2 combined with @types/react@^19.2.6 provides type safety for the latest stable version. The caret versioning strategy is appropriate for an example workspace.
demo-local.Dockerfile (1)
13-13: Appropriate Docker build step for Node.js dependencies.Placement and structure follow the existing pattern for environment setup. The
npm installwill correctly resolve dependencies from the package.json added in the same PR.demo-output.yaml (3)
603-615: Node.js provider symbol caching working correctly.The line number adjustments for
node-sample-rule-001andnode-sample-rule-002reflect accurate symbol matching after the Prepare() phase. These are expected outcomes as the Node.js provider now caches document symbols before evaluation.Also applies to: 626-641
642-652: New test rule validates module filtering behavior.
node-sample-rule-003tests that module files are correctly excluded from matching. This addition expands test coverage and aligns with the PR's goal of improving symbol-based rule evaluation through the Prepare() function.
802-807: JSX support expansion confirmed.The addition of
LegacyComponent.jsxundertest-tsx-support-00010demonstrates that the Node.js provider now correctly identifies React references across both TypeScript (.tsx) and JavaScript (.jsx) files. This is a positive outcome of the symbol caching improvements.provider/lib.go (1)
75-85: All walkDir call sites now correctly propagate exclude informationPassing
excludedDirsandexcludedPatternsinto everywalkDirFuncinvocation keeps traversal and final filtering consistent (search-criteria scan, includedDirs, additionalPaths, and basePath-only scan all respect the same exclude set). No functional issues spotted here.Also applies to: 102-113, 140-150, 160-169, 173-181
external-providers/generic-external-provider/pkg/server_configurations/yaml_language_server/service_client.go (1)
103-112: New LSP base client argument wiring looks correctPassing
nilas the new final argument tobase.NewLSPServiceClientBaseis consistent with the updated constructor and keeps YAML provider behavior unchanged (no custom symbol search helper).external-providers/generic-external-provider/pkg/server_configurations/generic/service_client.go (1)
93-99: Generic service client base initialization remains consistentThe added
nilargument tobase.NewLSPServiceClientBasealigns with the new signature and keeps the generic provider behavior intact (no symbol search helper).external-providers/generic-external-provider/go.mod (1)
5-13: Dependency updates look aligned with new features; verify module hygieneUpgrading
github.com/konveyor/analyzer-lspand addinggolang.org/x/syncas indirect both match the new symbol cache / preparation work. Please ensurego mod tidyhas been run so there are no unintended extra changes in this module.Also applies to: 15-39
provider/provider.go (2)
433-441: FullPrepareResponse mirrors existing aggregation patterns correctlyThe new
FullPrepareResponsemirrorsFullNotifyFileChangesResponseand correctly aggregates errors witherrors.Join, while still calling each client even if previous ones fail.
472-482: ConditionsByCap and Prepare API shape look appropriate
ConditionsByCapcleanly groups raw, serialized conditions per capability, and theServiceClient.Preparehook provides a clear pre‑evaluation extension point without changing existingEvaluatesemantics.external-providers/dotnet-external-provider/pkg/dotnet/service_client.go (1)
41-43: No-op Prepare correctly satisfies new ServiceClient contractThe added
Preparemethod safely returnsnil, keeping current behavior unchanged while satisfying the extendedprovider.ServiceClientinterface and leaving room for future warmup logic.provider/internal/builtin/service_client.go (1)
54-56: Builtin Prepare hook is a safe no-opAdding
Prepareas a no-op keeps the builtin provider compatible with the new lifecycle without altering existing evaluation behavior.lsp/base_service_client/symbol_cache.go (1)
274-283: compileSymbolQueryRegex behavior is sound for simple and regex queriesThe
compileSymbolQueryRegexhelper correctly treats queries without regex metacharacters as exact, case-insensitive matches (^...$withQuoteMeta), while allowing power users to pass full regexes when needed. This is a good balance between usability and flexibility.external-providers/generic-external-provider/pkg/server_configurations/pylsp/service_client.go (1)
72-77: LGTM!The addition of
nilas the symbol-search helper parameter maintains existing behavior while aligning with the updated base client constructor signature.external-providers/yq-external-provider/pkg/yq_provider/provider.go (1)
129-131: LGTM!The no-op
Prepareimplementation correctly implements the new provider interface method, establishing the hook for future pre-evaluation warmup logic.provider/internal/builtin/provider.go (1)
149-152: LGTM!The no-op
Prepareimplementation with a clear TODO comment establishes the infrastructure hook while documenting the intended future use for pre-processing file content searches.external-providers/dotnet-external-provider/pkg/dotnet/provider.go (1)
203-205: LGTM!The no-op
Prepareimplementation correctly satisfies the provider interface, establishing the preparation hook for future enhancement.external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)
520-522: LGTM!The no-op
Prepareimplementation correctly implements the provider interface method, maintaining consistency with other providers in the preparation phase infrastructure.external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
111-113: LGTM!The no-op
Prepareimplementation on the service client correctly implements the interface, aligning with the provider-level preparation infrastructure.provider/grpc/provider.go (1)
245-247: LGTM!The
Prepareimplementation correctly orchestrates the preparation phase across all service clients usingprovider.FullPrepareResponse, maintaining consistency with the patterns used inEvaluate,GetDependencies, and other orchestration methods.external-providers/yq-external-provider/pkg/yq_provider/service_client.go (1)
59-61: LGTM!The no-op
Prepareimplementation correctly satisfies the service client interface, establishing the preparation hook while maintaining consistency with other providers.provider/provider_test.go (1)
22-24: No-opPreparecorrectly satisfies theClientinterface in testsThis stub is sufficient for tests that don't care about preparation behavior and keeps the fake in sync with the production interface.
lsp/base_service_client/base_capabilities.go (1)
34-35: EmbeddingProviderContextand updatedGetAllDeclarationsusage look consistentInlining
provider.ProviderContextintoReferencedConditioncleanly exposes the shared provider fields to YAML without changing existing behavior, and switching tosc.GetAllDeclarations(ctx, query)matches the simplified API while still performing workspace/dependency filtering viaBaseConfig. No issues from this change alone.Also applies to: 57-57
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go
Show resolved
Hide resolved
...-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go
Show resolved
Hide resolved
Signed-off-by: Pranav Gaikwad <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
lsp/base_service_client/symbol_cache.go (1)
128-198: Add exclusions for additional workspace folders.The exclude patterns in
GetDocumentUrisare currently built only fromprimaryPath(lines 173-181), butadditionalPathsfrom workspace folders also need corresponding exclusions. Without this, heavy directories likenode_modules,vendor, etc., in additional workspace folders won't be excluded from the search.Apply this approach to add exclusions for each additional path:
+ allExcludes := []string{} + for _, basePath := range append([]string{primaryPath}, additionalPaths...) { + allExcludes = append(allExcludes, + filepath.Join(basePath, "node_modules"), + filepath.Join(basePath, "vendor"), + filepath.Join(basePath, ".git"), + filepath.Join(basePath, "dist"), + filepath.Join(basePath, "build"), + filepath.Join(basePath, "target"), + filepath.Join(basePath, ".venv"), + filepath.Join(basePath, "venv"), + ) + } searcher := provider.FileSearcher{ BasePath: primaryPath, AdditionalPaths: additionalPaths, ProviderConfigConstraints: provider.IncludeExcludeConstraints{ - ExcludePathsOrPatterns: []string{ - filepath.Join(primaryPath, "node_modules"), - filepath.Join(primaryPath, "vendor"), - filepath.Join(primaryPath, ".git"), - filepath.Join(primaryPath, "dist"), - filepath.Join(primaryPath, "build"), - filepath.Join(primaryPath, "target"), - filepath.Join(primaryPath, ".venv"), - filepath.Join(primaryPath, "venv"), - }, + ExcludePathsOrPatterns: allExcludes, },Based on learnings
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (1)
196-212: Fix CodeLocation to use zero-based line numbers and preserve multi-line spans.According to the
Positionstruct documentation (provider/provider.go lines 323-341),Lineshould be zero-based. Currently:
CodeLocation.StartPosition.LineandEndPosition.Lineboth uselineNumber(which is 1-based)- Multi-line symbols are flattened because start and end use the same line number
Apply this diff to use zero-based positions and preserve spans:
lineNumber := int(baseLocation.Range.Start.Line) + 1 incident := provider.IncidentContext{ FileURI: u, LineNumber: &lineNumber, Variables: map[string]interface{}{ "file": baseLocation.URI, }, CodeLocation: &provider.Location{ StartPosition: provider.Position{ - Line: float64(lineNumber), + Line: float64(baseLocation.Range.Start.Line), Character: float64(baseLocation.Range.Start.Character), }, EndPosition: provider.Position{ - Line: float64(lineNumber), + Line: float64(baseLocation.Range.End.Line), Character: float64(baseLocation.Range.End.Character), }, }, }Based on learnings
📜 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(5 hunks)external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go(1 hunks)lsp/base_service_client/symbol_cache.go(1 hunks)provider/grpc/service_client.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T16:27:59.216Z
Learnt from: shawn-hurley
Repo: konveyor/analyzer-lsp PR: 860
File: provider/server.go:251-259
Timestamp: 2025-10-21T16:27:59.216Z
Learning: In provider/server.go, the Init method uses context.Background() for creating the RPC client via GetProviderRPCClient because this RPC client is stored in the InitConfig and used across multiple subsequent requests (Evaluate, GetDependencies, etc.). The RPC client's lifecycle is managed separately through explicit Stop calls, not tied to the Init request's context.
Applied to files:
provider/grpc/service_client.go
🧬 Code graph analysis (4)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go (5)
lsp/base_service_client/base_service_client.go (1)
SymbolSearchHelper(165-185)provider/provider.go (2)
InitConfig(123-159)ConditionsByCap(472-475)provider/lib.go (3)
FileSearcher(19-28)IncludeExcludeConstraints(38-41)SearchCriteria(33-36)lsp/protocol/tsprotocol.go (2)
Pattern(3424-3424)WorkspaceSymbol(5052-5063)lsp/base_service_client/symbol_cache.go (2)
WorkspaceSymbolDefinitionsPair(27-30)NewDefaultSymbolCacheHelper(270-275)
provider/grpc/service_client.go (2)
provider/internal/grpc/library.pb.go (9)
NotifyFileChangesRequest(1546-1552)NotifyFileChangesRequest(1565-1565)NotifyFileChangesRequest(1580-1582)ConditionsByCapability(1642-1648)ConditionsByCapability(1661-1661)ConditionsByCapability(1676-1678)PrepareRequest(1694-1700)PrepareRequest(1713-1713)PrepareRequest(1728-1730)provider/provider.go (1)
ConditionsByCap(472-475)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (3)
provider/provider.go (5)
Config(88-98)ProviderContext(349-354)Location(319-322)IncidentContext(309-317)Position(324-342)external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go (1)
NewNodejsSymbolCacheHelper(178-184)lsp/base_service_client/base_service_client.go (2)
LSPServiceClientBase(120-158)NewLspServiceClientEvaluator(77-94)
lsp/base_service_client/symbol_cache.go (5)
lsp/protocol/tsprotocol.go (5)
DocumentSymbol(1388-1413)WorkspaceSymbol(5052-5063)Range(3559-3564)SelectionRange(3778-3783)Pattern(3424-3424)provider/provider.go (3)
InitConfig(123-159)ConditionsByCap(472-475)ProviderContext(349-354)lsp/base_service_client/base_capabilities.go (1)
ReferencedCondition(30-35)provider/lib.go (3)
FileSearcher(19-28)IncludeExcludeConstraints(38-41)SearchCriteria(33-36)lsp/base_service_client/base_service_client.go (1)
SymbolSearchHelper(165-185)
⏰ 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 (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: test (macos-latest, darwin, arm64)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (macos-latest, mac)
🔇 Additional comments (5)
provider/grpc/service_client.go (1)
202-226: LGTM! Prepare method implementation looks solid.The new
Preparemethod correctly:
- Converts
provider.ConditionsByCapto protobuf format- Includes the provider ID in the request
- Handles both RPC errors and application-level errors from the response
The implementation follows the same pattern as other methods in this file (e.g.,
Evaluate,GetDependencies).lsp/base_service_client/symbol_cache.go (1)
74-81: LGTM! InvalidateAll now correctly clears both caches.The fix addresses the previous review concern by clearing both workspace symbols and document symbols under their respective mutexes.
external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go (2)
52-56: Good defensive programming.Initializing
ProviderSpecificConfigwhen nil prevents potential panics when settingworkspaceFolders.
173-190: LGTM! Workspace folder and dependency filtering are now correct.The previous review concerns have been addressed:
- Line 173 now guards against empty
WorkspaceFoldersto prevent panics- Lines 177-190 correctly use a
skipflag to filter out symbols from dependency foldersexternal-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go (1)
106-157: LGTM! Scoped query matching now uses platform-independent URI format.Lines 127 and 137 correctly use
fmt.Sprintf("%s/%s", *query.Package, *query.Scope)instead offilepath.Join, which ensures URI matching works correctly across all platforms (avoiding Windows backslash issues).
...-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go
Show resolved
Hide resolved
Signed-off-by: Pranav Gaikwad <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go (1)
187-224: Consider defensive checks for empty package or scope components.The
parseQueryfunction can assign empty strings toPackageorScopein certain edge cases:
- Pattern
"package/"yieldsPackage="package",Scope=""- Pattern
"package/#query"yieldsPackage="package",Scope=""While these patterns may not occur in practice, adding checks before assigning could prevent subtle bugs:
pkg := prefix[:slashIdx] scope := prefix[slashIdx+1:] - if pkg != "" { + if strings.TrimSpace(pkg) != "" { result.Package = &pkg } - if scope != "" { + if strings.TrimSpace(scope) != "" { result.Scope = &scope }This ensures only non-empty, non-whitespace values are assigned.
📜 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/symbol_cache_helper.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T14:12:52.206Z
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.206Z
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.
Applied to files:
external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go
🧬 Code graph analysis (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go (5)
lsp/base_service_client/base_service_client.go (1)
SymbolSearchHelper(165-185)provider/provider.go (2)
InitConfig(123-159)ConditionsByCap(472-475)provider/lib.go (3)
FileSearcher(19-28)IncludeExcludeConstraints(38-41)SearchCriteria(33-36)lsp/protocol/tsprotocol.go (2)
Pattern(3424-3424)WorkspaceSymbol(5052-5063)lsp/base_service_client/symbol_cache.go (2)
WorkspaceSymbolDefinitionsPair(27-30)NewDefaultSymbolCacheHelper(270-275)
⏰ 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, amd64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
🔇 Additional comments (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go (1)
75-99: No issues found withreferencedConditiontype usage.The
referencedConditiontype is properly defined in the same package atexternal-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go(lines 122-127) and correctly matches the unmarshal pattern used inMatchFileContentByConditions. The code in lines 75-99 properly accesses theReferenced.Patternfield as expected from the type definition.
...-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go
Show resolved
Hide resolved
Signed-off-by: Pranav Gaikwad <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go (1)
106-157: This concern was previously flagged.Previous reviews identified that lines 127 and 137 should prepend
@when matching npm scoped packages (e.g.,@types/node). The current implementation requires users to explicitly include the@prefix in their patterns, which was noted as unintuitive. Consider the previous suggestion to usefmt.Sprintf("@%s/%s", *query.Package, *query.Scope).
🧹 Nitpick comments (1)
lsp/base_service_client/base_service_client.go (1)
142-151: Consider buffering strategy for symbolCacheUpdateChan.The channel is buffered to 10 items (line 264), which may cause blocking if many file updates arrive simultaneously in large codebases. Consider whether this capacity is sufficient or if a larger buffer or different queuing strategy might be needed for responsive symbol cache updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go(1 hunks)lsp/base_service_client/base_service_client.go(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-21T14:12:52.206Z
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.206Z
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.
Applied to files:
external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go
📚 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
🧬 Code graph analysis (2)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go (5)
lsp/base_service_client/base_service_client.go (1)
SymbolSearchHelper(165-185)provider/provider.go (2)
InitConfig(123-159)ConditionsByCap(472-475)provider/lib.go (3)
FileSearcher(19-28)IncludeExcludeConstraints(38-41)SearchCriteria(33-36)lsp/protocol/tsprotocol.go (2)
Pattern(3424-3424)WorkspaceSymbol(5052-5063)lsp/base_service_client/symbol_cache.go (2)
WorkspaceSymbolDefinitionsPair(27-30)NewDefaultSymbolCacheHelper(270-275)
lsp/base_service_client/base_service_client.go (3)
lsp/base_service_client/symbol_cache.go (4)
SymbolCache(20-25)WorkspaceSymbolDefinitionsPair(27-30)NewDocumentSymbolCache(32-37)NewDefaultSymbolCacheHelper(270-275)lsp/protocol/tsprotocol.go (15)
URI(4673-4673)Log(5479-5479)WorkspaceSymbol(5052-5063)Location(2389-2392)Range(3559-3564)Info(5477-5477)OrPLocation_workspace_symbol(2764-2766)DocumentURI(1460-1460)BaseSymbolInformation(51-65)Text(5341-5341)TextDocumentIdentifier(4478-4481)DocumentSymbol(1388-1413)DocumentSymbolParams(1448-1453)File(5582-5582)Position(3452-3466)provider/provider.go (5)
ConditionsByCap(472-475)InitConfig(123-159)FileChange(466-470)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 (windows-latest, windows, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (macos-latest, darwin, amd64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
🔇 Additional comments (18)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go (7)
1-24: LGTM!The package structure, imports, and type definition are well-organized. Embedding
base.SymbolSearchHelperallows extending the default behavior while reusing base functionality.
26-73: LGTM!The document URI discovery logic correctly normalizes paths, uses appropriate file filters for TypeScript/JavaScript files, and excludes common non-source directories.
Based on learnings
75-104: LGTM!The content matching logic appropriately distinguishes between literal and regex patterns, uses whole-word matching for literals, and handles errors gracefully by logging and continuing.
159-176: LGTM!The condition matching logic correctly delegates to
MatchSymbolByPatternsafter unmarshaling the YAML condition.
178-184: LGTM!The constructor correctly initializes the Node.js symbol search helper by embedding the default cache helper.
186-229: LGTM!The query parsing logic correctly handles all pattern variations: plain queries without
#, patterns with package/scope prefixes, and gracefully handles edge cases.
231-285: LGTM!The helper functions correctly implement whole-word matching with proper token boundary detection. The identifier character set appropriately includes JavaScript/TypeScript-specific characters like
$.lsp/base_service_client/base_service_client.go (11)
160-185: LGTM!The
SymbolSearchHelperinterface provides a clean abstraction for language-specific symbol search behavior. The documentation clearly distinguishes between Prepare-phase and Evaluate-phase operations.
187-296: LGTM!The constructor correctly initializes the symbol cache infrastructure, provides sensible defaults, and properly starts the background update handler before the client is ready for use.
315-333: LGTM!The file change notification correctly invalidates affected cache entries and queues updates with proper synchronization. The wait group is incremented before sending to the channel, ensuring safe coordination.
336-348: LGTM!The
Preparemethod correctly initiates symbol cache population by calling the helper'sGetDocumentUrisand queuing discovered files for processing. Wait group usage properly tracks all async operations.
423-456: LGTM!The refactored
GetAllDeclarationsappropriately prefers the LSP's nativeworkspace/symbolrequest and falls back to cached symbol search when needed. The wait for pending updates ensures cache consistency.
479-573: LGTM!The symbol cache population orchestrates multiple LSP operations correctly: text search, definition lookup, document symbol queries, and symbol matching. Error handling appropriately logs and continues, allowing batch processing to complete despite individual file errors.
575-620: LGTM!The symbol cache update handler correctly implements the worker pattern with graceful shutdown. The drain logic prevents wait group deadlock by decrementing without processing when shutting down.
622-694: LGTM!The document symbol query helpers correctly manage the LSP lifecycle (didOpen/didClose) around symbol requests and cache results to avoid redundant calls. The
toURIutility properly handles both URI-formatted and plain file paths.
696-759: LGTM!The content search efficiently scans line-by-line, queries document symbols only once, and handles cases where symbol information is unavailable by creating a fallback workspace symbol with the matched text.
761-786: LGTM!The definition lookup correctly wraps the LSP call with didOpen/didClose and handles errors gracefully. Note that line 768 uses
location.Range.Endas the position, which may be intentional to resolve definitions at the end of matched text.
788-859: LGTM!The symbol finding utilities correctly implement tree traversal with best-match selection based on smallest overlapping range. The range comparison and length calculation logic is sound. Note that line 796 calls
preferredRange, which should be defined elsewhere in the codebase.
2 participants
This implements Prepare() function for nodejs provider too.
Prepare() function will be called prior to running Evaluate(). The providers will be passed all conditions in the Prepare() function where the providers may choose to perform custom logic after Init() but before Evaluate(). The generic provider uses the Prepare() call to query document symbols and cache them in-memory. Evaluate() call simply returns the cached results.
Summary by CodeRabbit
New Features
Updates
✏️ Tip: You can customize this high-level summary in your review settings.