-
Notifications
You must be signed in to change notification settings - Fork 53
🐛 Fix nodejs provider counting files twice when Location and workspaceFolders overlap #1036
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 Fix nodejs provider counting files twice when Location and workspaceFolders overlap #1036
Conversation
…eFolders overlap The nodejs provider's GetDocumentUris() method was counting files twice during provider preparation when both InitConfig.Location and workspaceFolders were set to the same directory. This caused incorrect progress reporting and performance degradation. Added a deduplication check to skip workspace folders that duplicate the primaryPath, ensuring each directory is only scanned once by FileSearcher. Fixes konveyor#1035 Signed-off-by: tsanders <[email protected]>
WalkthroughFixed a duplicate file counting issue in the nodejs provider's Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.go (1)
44-47: Deduplication guard correctly fixes double-counting; consider path normalization for robustnessThe new check cleanly prevents adding a workspace folder that is exactly equal to
primaryPath, which addresses the reported double-counting whenLocationandworkspaceFolderspoint to the same directory while preserving the existing behavior whenprimaryPathstarts empty.If you want this to be a bit more robust against minor path spelling differences (e.g.,
"/repo"vs"/repo/"), you could normalize both sides before comparison:- // Skip workspace folders that duplicate primaryPath to avoid counting files twice - if path == primaryPath { + // Skip workspace folders that duplicate primaryPath (after normalization) to avoid counting files twice + if filepath.Clean(path) == filepath.Clean(primaryPath) { continue }This keeps the core behavior but avoids surprises from equivalent paths with different string forms, while still treating
additionalPathsas already‑vetted workspaces without changing exclude patterns on them. Based on learnings, this remains consistent with only applying exclusion patterns toprimaryPath.
📜 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 (2)
📓 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.
📚 Learning: 2025-11-21T14:12:52.228Z
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.
Applied to files:
external-providers/generic-external-provider/pkg/server_configurations/nodejs/symbol_cache_helper.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). (12)
- GitHub Check: build-all-providers (c-sharp, c-sharp-provider, Dockerfile, konveyor/c-sharp-analyzer-provider, f...
- GitHub Check: build-all-providers (java, java-provider, external-providers/java-external-provider/Dockerfile, J...
- GitHub Check: build-all-providers (yq, yq-provider, external-providers/yq-external-provider/Dockerfile, true)
- GitHub Check: build-all-providers (generic, generic-provider, external-providers/generic-external-provider/Dock...
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
- GitHub Check: benchmark (ubuntu-latest, linux)
- GitHub Check: test (macos-latest, darwin, arm64)
- GitHub Check: test (windows-latest, windows, amd64)
- GitHub Check: test (ubuntu-latest, linux, amd64)
- GitHub Check: test (ubuntu-latest, linux, arm64)
- GitHub Check: test (macos-latest, darwin, amd64)
Removes the nodejs provider workaround that set Location to empty string to avoid duplicate file counting. This workaround is no longer needed as the root cause has been fixed in analyzer-lsp#1036. Changes: - Update analyzer-lsp to version with deduplication fix (2a753870572f) - Remove nodejs-specific Location="" workaround - Simplify provider configuration logic - Update comments to reference merged fix Fixes: konveyor/analyzer-lsp#1035 Related: konveyor/analyzer-lsp#1036 Signed-off-by: tsanders <[email protected]>
Removes the nodejs provider workaround that set Location to empty string to avoid duplicate file counting. This workaround is no longer needed as the root cause has been fixed in analyzer-lsp#1036. Changes: - Remove nodejs-specific Location="" workaround - Simplify provider configuration logic - Update comments to reference merged fix Fixes: konveyor/analyzer-lsp#1035 Related: konveyor/analyzer-lsp#1036 Signed-off-by: tsanders <[email protected]>
Summary
Fixes the nodejs provider's
GetDocumentUris()method counting files twice during provider preparation when bothInitConfig.LocationandProviderSpecificConfig["workspaceFolders"]are set to the same directory.Changes
symbol_cache_helper.goto skip workspace folders that duplicate the primaryPathImpact
Testing
Fixes #1035
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.