chore: dead code removal, extract helpers, deduplicate logic - #98
Merged
Conversation
Dead code removal: - Remove unused closeStaleClassLoaders/closeClassLoader in DederProjectState - Remove unused expectedCredentialType in CredentialsResolver - Remove unused @tailrec import in PluginLoader - Remove TODO about plugin task namespacing (rely on prefix convention) Refactors: - Extract withCliSpan helper in CliClientMessageHandler (13+ OTEL blocks → 1) - Extract ifHelpThenShow helper for --help flag checks (8 repeated blocks) - Extract scalafixTask private method in CoreTasks (fix/fixCheck 90% deduped) - Move shellSplit from TabCompleter to ShellUtils.scala - Deduplicate META-INF/services entries in JarUtils fat JAR assembly Fix: - shellSplit: handle cursor in middle of last token (was missing currentWordIndex) Tests: - Add ShellUtilsSuite with 11 tests for shellSplit
📝 WalkthroughWalkthroughFive independent refactoring changes: ChangesMulti-area Refactoring and Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
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: 2
🧹 Nitpick comments (2)
server/src/ba/sake/deder/cli/CliClientMessageHandler.scala (1)
584-589: ⚡ Quick winTrace the help fast path before exiting.
ifHelpThenShowemitsOutput+Exit(0)before callers enterwithCliSpan, so requests likemodules --help,tasks -h, etc. produce no CLI span withclientId/request.id. Pass the command name into this helper and wrap the help branch withwithCliSpan(...), or call it from inside each command span. As per coding guidelines,**/*.scala: “Usetraced()/javaFuture()wrappers fromOTEL.scalafor tracing spans in BSP/CLI requests with OpenTelemetry.”Suggested direction
- private def ifHelpThenShow(args: Seq[String], helpText: => String): Boolean = + private def ifHelpThenShow(command: String, args: Seq[String], helpText: => String): Boolean = if args == Seq("--help") || args == Seq("-h") then { - serverMessages.put(CliServerMessage.Output(helpText)) - serverMessages.put(CliServerMessage.Exit(0)) + withCliSpan(command) { _ => + serverMessages.put(CliServerMessage.Output(helpText)) + serverMessages.put(CliServerMessage.Exit(0)) + } true } else falseCallers would then use e.g.
ifHelpThenShow("modules", m.args, ...).🤖 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 `@server/src/ba/sake/deder/cli/CliClientMessageHandler.scala` around lines 584 - 589, The `ifHelpThenShow` method emits messages and exits before callers can establish an OpenTelemetry span, so help requests lack proper tracing context. Update the method signature to accept a command name parameter, then wrap the help branch (where Output and Exit messages are put) with `withCliSpan(...)` using that command name. Update all callers of `ifHelpThenShow` to pass the command name as the first argument (e.g., "modules", "tasks", etc.).Source: Coding guidelines
server/test/src/ba/sake/deder/cli/ShellUtilsSuite.scala (1)
59-70: ⚡ Quick winAdd a regression test for cursor-before-first-word with leading spaces.
The suite currently misses the documented
cursorTokenIndex = -1case when cursor is before the first token (e.g.," deder", cursor0).Suggested test
+ test("cursor before first word after leading spaces") { + val (tokens, cursorIdx) = ShellUtils.shellSplit(" deder", 0) + assertEquals(tokens, Seq("deder")) + assertEquals(cursorIdx, -1) + }🤖 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 `@server/test/src/ba/sake/deder/cli/ShellUtilsSuite.scala` around lines 59 - 70, The test suite is missing a regression test for the documented case where the cursor is positioned before the first word with leading spaces (e.g., cursor at position 0 in " deder"), which should return cursorTokenIndex = -1. Add a new test case in the ShellUtilsSuite that calls ShellUtils.shellSplit with a string containing leading whitespace followed by a word, with the cursor positioned at 0, and verify using assertEquals that tokens contains the word(s) and cursorIdx equals -1.
🤖 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 `@server/src/ba/sake/deder/cli/ShellUtils.scala`:
- Around line 25-27: The condition at line 25 that sets isCurrentWord = true
based on the cursor position does not properly account for leading whitespace
before the first word. When the cursor is in leading whitespace (e.g., at
position 0 in " deder"), the code incorrectly identifies it as the current
word and then maps it to the wrong token index at lines 60-63, returning 0
instead of the documented -1. Fix this by ensuring isCurrentWord is only set to
true when the cursor position is actually within a word token, not in leading
whitespace before any words. Adjust the condition at line 25 to check that the
cursor position falls within a valid token's boundaries before setting
isCurrentWord to true.
In `@server/src/ba/sake/deder/jvm/JarUtils.scala`:
- Around line 198-206: The deduplicateServiceLines method does not trim
whitespace from lines before partitioning and filtering, which violates the Java
ServiceLoader specification and prevents proper deduplication. Modify the method
to trim each line immediately after splitting the raw string, ensuring that
entries with leading or trailing whitespace are normalized before being
partitioned into comments and entries, and that whitespace-only lines are
properly filtered out by the .nonEmpty check. Apply the trim operation to the
lines sequence right after calling linesIterator.toSeq.
---
Nitpick comments:
In `@server/src/ba/sake/deder/cli/CliClientMessageHandler.scala`:
- Around line 584-589: The `ifHelpThenShow` method emits messages and exits
before callers can establish an OpenTelemetry span, so help requests lack proper
tracing context. Update the method signature to accept a command name parameter,
then wrap the help branch (where Output and Exit messages are put) with
`withCliSpan(...)` using that command name. Update all callers of
`ifHelpThenShow` to pass the command name as the first argument (e.g.,
"modules", "tasks", etc.).
In `@server/test/src/ba/sake/deder/cli/ShellUtilsSuite.scala`:
- Around line 59-70: The test suite is missing a regression test for the
documented case where the cursor is positioned before the first word with
leading spaces (e.g., cursor at position 0 in " deder"), which should return
cursorTokenIndex = -1. Add a new test case in the ShellUtilsSuite that calls
ShellUtils.shellSplit with a string containing leading whitespace followed by a
word, with the cursor positioned at 0, and verify using assertEquals that tokens
contains the word(s) and cursorIdx equals -1.
🪄 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 Plus
Run ID: 7a767b1e-61dd-4184-8a6c-cb2d7e62baff
📒 Files selected for processing (9)
server/src/ba/sake/deder/CoreTasks.scalaserver/src/ba/sake/deder/DederProjectState.scalaserver/src/ba/sake/deder/cli/CliClientMessageHandler.scalaserver/src/ba/sake/deder/cli/ShellUtils.scalaserver/src/ba/sake/deder/cli/TabCompleter.scalaserver/src/ba/sake/deder/jvm/JarUtils.scalaserver/src/ba/sake/deder/plugin/PluginLoader.scalaserver/src/ba/sake/deder/publish/CredentialsResolver.scalaserver/test/src/ba/sake/deder/cli/ShellUtilsSuite.scala
💤 Files with no reviewable changes (3)
- server/src/ba/sake/deder/plugin/PluginLoader.scala
- server/src/ba/sake/deder/publish/CredentialsResolver.scala
- server/src/ba/sake/deder/DederProjectState.scala
Comment on lines
+25
to
+27
| if i == cursorPos then { | ||
| isCurrentWord = true | ||
| } |
There was a problem hiding this comment.
Cursor-before-first-word case can return the wrong token index.
Line 25 sets isCurrentWord even when the cursor is still in leading whitespace, and Line 60-63 can then map that to the last token. For " deder" at cursor 0, this returns 0 instead of documented -1.
Suggested fix
- } else if isCurrentWord && tokens.nonEmpty then {
+ } else if isCurrentWord && tokens.nonEmpty
+ && commandLine.indexWhere(ch => !ch.isWhitespace) <= cursorPos then {
// cursor is in the middle of the last token (no trailing whitespace)
currentWordIndex = tokens.length - 1
}Also applies to: 60-63
🤖 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 `@server/src/ba/sake/deder/cli/ShellUtils.scala` around lines 25 - 27, The
condition at line 25 that sets isCurrentWord = true based on the cursor position
does not properly account for leading whitespace before the first word. When the
cursor is in leading whitespace (e.g., at position 0 in " deder"), the code
incorrectly identifies it as the current word and then maps it to the wrong
token index at lines 60-63, returning 0 instead of the documented -1. Fix this
by ensuring isCurrentWord is only set to true when the cursor position is
actually within a word token, not in leading whitespace before any words. Adjust
the condition at line 25 to check that the cursor position falls within a valid
token's boundaries before setting isCurrentWord to true.
Comment on lines
+198
to
+206
| /** Deduplicate lines in a META-INF/services/ file. | ||
| * Keeps comments (#) and non-empty unique service provider entries. | ||
| */ | ||
| private def deduplicateServiceLines(raw: String): String = { | ||
| val lines = raw.linesIterator.toSeq | ||
| val (comments, entries) = lines.partition(_.startsWith("#")) | ||
| val uniqueEntries = entries.filter(_.nonEmpty).distinct | ||
| (comments ++ uniqueEntries).mkString("\n") | ||
| } |
There was a problem hiding this comment.
Trim lines before partitioning and filtering.
The Java ServiceLoader specification requires whitespace to be trimmed from each line. Without trimming:
- Entries like
"com.example.Provider"and" com.example.Provider"won't be deduplicated. - Whitespace-only lines (e.g.,
" ") won't be filtered out by.nonEmpty.
This defeats the purpose of deduplication and violates the services file format.
🛠️ Proposed fix
private def deduplicateServiceLines(raw: String): String = {
- val lines = raw.linesIterator.toSeq
+ val lines = raw.linesIterator.map(_.trim).filter(_.nonEmpty).toSeq
val (comments, entries) = lines.partition(_.startsWith("#"))
- val uniqueEntries = entries.filter(_.nonEmpty).distinct
+ val uniqueEntries = entries.distinct
(comments ++ uniqueEntries).mkString("\n")
}📝 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.
Suggested change
| /** Deduplicate lines in a META-INF/services/ file. | |
| * Keeps comments (#) and non-empty unique service provider entries. | |
| */ | |
| private def deduplicateServiceLines(raw: String): String = { | |
| val lines = raw.linesIterator.toSeq | |
| val (comments, entries) = lines.partition(_.startsWith("#")) | |
| val uniqueEntries = entries.filter(_.nonEmpty).distinct | |
| (comments ++ uniqueEntries).mkString("\n") | |
| } | |
| private def deduplicateServiceLines(raw: String): String = { | |
| val lines = raw.linesIterator.map(_.trim).filter(_.nonEmpty).toSeq | |
| val (comments, entries) = lines.partition(_.startsWith("#")) | |
| val uniqueEntries = entries.distinct | |
| (comments ++ uniqueEntries).mkString("\n") | |
| } |
🤖 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 `@server/src/ba/sake/deder/jvm/JarUtils.scala` around lines 198 - 206, The
deduplicateServiceLines method does not trim whitespace from lines before
partitioning and filtering, which violates the Java ServiceLoader specification
and prevents proper deduplication. Modify the method to trim each line
immediately after splitting the raw string, ensuring that entries with leading
or trailing whitespace are normalized before being partitioned into comments and
entries, and that whitespace-only lines are properly filtered out by the
.nonEmpty check. Apply the trim operation to the lines sequence right after
calling linesIterator.toSeq.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dead code removal:
Refactors:
Fix:
Tests:
Summary by CodeRabbit
Refactor
Tests