Refactor symbol visibility management in DocumentSymbolProvider#8369
Refactor symbol visibility management in DocumentSymbolProvider#8369doofin wants to merge 8 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA new ChangesSymbols View Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala`:
- Around line 65-66: Add a UserConfigurationOption entry for the new field so it
is exposed in the generated config schema/docs: update the
UserConfiguration.options collection to include an option describing
symbolsViewShowAll (label "symbols-view-show-all", its Boolean type, default
false, and a descriptive message). Locate the UserConfiguration class/companion
where options is defined and append the new UserConfigurationOption referencing
the symbolsViewShowAll field so clients and docs can discover and surface it.
In
`@metals/src/main/scala/scala/meta/internal/parsing/DocumentSymbolProvider.scala`:
- Line 27: The code currently caches the user flag in the val symbolsViewShowAll
at construction, preventing runtime config changes from taking effect; change
this by reading userConfig().symbolsViewShowAll dynamically inside filterSymbols
(or convert symbolsViewShowAll to a def that delegates to userConfig()) so
filterSymbols uses the current setting each call; update references to
symbolsViewShowAll in DocumentSymbolProvider to use the dynamic def or the
direct userConfig() read to ensure outline behavior reflects runtime updates.
- Around line 104-107: The parameter symbols (Term.Param) are still being added
unconditionally; wrap or gate the branch that adds Term.Param with the existing
filterSymbols check so parameters are only added when symbolsViewShowAll is
true. Locate the code that emits parameter symbols (the Term.Param handling
block) and call filterSymbols { ... } (or pass the param-add closure into
filterSymbols) instead of unconditionally invoking the add, reusing the existing
filterSymbols helper to suppress parameters when symbolsViewShowAll is false.
- Around line 105-107: The boolean check in filterSymbols is inverted: change
the branch so that when symbolsViewShowAll is true it executes addPatF and when
false it does nothing. Locate the filterSymbols method and swap the branches (or
invert the condition) so addPatF runs under symbolsViewShowAll == true; keep the
lazy-by-name parameter addPatF usage intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d87231fa-9eb1-449a-8119-37e29c176bc5
📒 Files selected for processing (3)
metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scalametals/src/main/scala/scala/meta/internal/parsing/DocumentSymbolProvider.scala
| symbolsViewShowAll: Boolean = false, | ||
| ) { |
There was a problem hiding this comment.
Register symbols-view-show-all in UserConfiguration.options.
The new field is parsed/serialized, but without a UserConfigurationOption entry it won’t be discoverable in generated config docs/schema and many clients won’t surface it properly.
Suggested patch
@@
UserConfigurationOption(
"mcp-client",
"",
"claude",
"MCP Client Name",
@@
),
+ UserConfigurationOption(
+ "symbols-view-show-all",
+ "false",
+ "true",
+ "Show all symbols in outline view",
+ """|When enabled, include value/variable symbols (and other non-top-level entries)
+ |in the document symbols outline.
+ |""".stripMargin,
+ isBoolean = true,
+ ),
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala`
around lines 65 - 66, Add a UserConfigurationOption entry for the new field so
it is exposed in the generated config schema/docs: update the
UserConfiguration.options collection to include an option describing
symbolsViewShowAll (label "symbols-view-show-all", its Boolean type, default
false, and a descriptive message). Locate the UserConfiguration class/companion
where options is defined and append the new UserConfigurationOption referencing
the symbolsViewShowAll field so clients and docs can discover and surface it.
| userConfig: () => UserConfiguration, | ||
| ) { | ||
|
|
||
| val symbolsViewShowAll: Boolean = userConfig().symbolsViewShowAll |
There was a problem hiding this comment.
Don’t cache symbolsViewShowAll at construction time.
Line 27 captures the flag once, so runtime config updates won’t affect outline behavior until restart. Read it dynamically in filterSymbols (or make it a def).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@metals/src/main/scala/scala/meta/internal/parsing/DocumentSymbolProvider.scala`
at line 27, The code currently caches the user flag in the val
symbolsViewShowAll at construction, preventing runtime config changes from
taking effect; change this by reading userConfig().symbolsViewShowAll
dynamically inside filterSymbols (or convert symbolsViewShowAll to a def that
delegates to userConfig()) so filterSymbols uses the current setting each call;
update references to symbolsViewShowAll in DocumentSymbolProvider to use the
dynamic def or the direct userConfig() read to ensure outline behavior reflects
runtime updates.
| // don't show local variables, parameters, and private members in the outline view, since they are not visible outside of their enclosing class or method. This is especially important for parameters, since they can be very numerous and clutter the outline view. | ||
| def filterSymbols(addPatF: => Unit) = { | ||
| if (symbolsViewShowAll) {} else addPatF | ||
| } |
There was a problem hiding this comment.
Parameter symbols are still unfiltered.
The new behavior/comment mentions reducing parameter clutter, but Term.Param is still always added (Lines 232-239). Gate that branch with the same filter to keep behavior consistent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@metals/src/main/scala/scala/meta/internal/parsing/DocumentSymbolProvider.scala`
around lines 104 - 107, The parameter symbols (Term.Param) are still being added
unconditionally; wrap or gate the branch that adds Term.Param with the existing
filterSymbols check so parameters are only added when symbolsViewShowAll is
true. Locate the code that emits parameter symbols (the Term.Param handling
block) and call filterSymbols { ... } (or pass the param-add closure into
filterSymbols) instead of unconditionally invoking the add, reusing the existing
filterSymbols helper to suppress parameters when symbolsViewShowAll is false.
| def filterSymbols(addPatF: => Unit) = { | ||
| if (symbolsViewShowAll) {} else addPatF | ||
| } |
There was a problem hiding this comment.
symbolsViewShowAll condition is reversed.
Line 106 currently hides symbols when symbolsViewShowAll is true and shows them when false, which is the opposite of the flag name and expected behavior.
Suggested patch
- def filterSymbols(addPatF: => Unit) = {
- if (symbolsViewShowAll) {} else addPatF
- }
+ def filterSymbols(addPatF: => Unit): Unit = {
+ if (userConfig().symbolsViewShowAll) addPatF else ()
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def filterSymbols(addPatF: => Unit) = { | |
| if (symbolsViewShowAll) {} else addPatF | |
| } | |
| def filterSymbols(addPatF: => Unit) = { | |
| if (symbolsViewShowAll) addPatF else () | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@metals/src/main/scala/scala/meta/internal/parsing/DocumentSymbolProvider.scala`
around lines 105 - 107, The boolean check in filterSymbols is inverted: change
the branch so that when symbolsViewShowAll is true it executes addPatF and when
false it does nothing. Locate the filterSymbols method and swap the branches (or
invert the condition) so addPatF runs under symbolsViewShowAll == true; keep the
lazy-by-name parameter addPatF usage intact.
…er initialization
…ll in filterSymbols
1 participant
This pull request improves the outline view in the Metals language server by reducing clutter from local variables, parameters, and private members. The main change is introducing a filter to control which symbols are shown, ensuring only top-level symbols are displayed in the outline.
Summary by CodeRabbit