ADFA-4613: Surround with try/catch code action - #1524
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 Walkthrough
WalkthroughAdds a Kotlin editor action that wraps selected whole lines in a ChangesKotlin try/catch code action
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant KotlinCodeActionsMenu
participant SurroundWithTryCatchAction
participant SurroundWithTryCatchUtils
participant LanguageClient
KotlinCodeActionsMenu->>SurroundWithTryCatchAction: expose try/catch action
SurroundWithTryCatchAction->>SurroundWithTryCatchUtils: resolve selection and compute TextEdit
SurroundWithTryCatchUtils-->>SurroundWithTryCatchAction: return TextEdit
SurroundWithTryCatchAction->>LanguageClient: submit formatted DocumentChange
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/SurroundWithTryCatch.kt (1)
28-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the block indentation token from the surrounding code.
The implementation hardcodes
\tfor the added block depth. If the user's codebase uses spaces (e.g.,baseIndentconsists of spaces), this creates a mix of spaces and tabs (e.g.,\te.printStackTrace()). While the follow-up formatter might clean this up, inferring the indent token ensures the raw text edit remains consistent in case the formatter is unavailable or fails.♻️ Proposed refactor to dynamically infer indentation
val baseIndent = selected.first(String::isNotBlank).takeWhile { it == ' ' || it == '\t' } - val body = selected.joinToString("\n") { if (it.isBlank()) it else "\t$it" } + val indentToken = if (baseIndent.startsWith(" ")) " " else "\t" + val body = selected.joinToString("\n") { if (it.isBlank()) it else "$indentToken$it" } val newText = buildString { append(baseIndent).append("try {\n") append(body).append('\n') append(baseIndent).append("} catch (e: Exception) {\n") - append(baseIndent).append("\te.printStackTrace()\n") + append(baseIndent).append(indentToken).append("e.printStackTrace()\n") append(baseIndent).append("}") }🤖 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 `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/SurroundWithTryCatch.kt` around lines 28 - 38, Update the indentation logic in the newText builder to derive the block indentation token from the surrounding code instead of hardcoding tab characters. Use the indentation style represented by baseIndent when indenting body lines and the catch statement, while preserving the existing try/catch structure and relative nesting.
🤖 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.
Nitpick comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/SurroundWithTryCatch.kt`:
- Around line 28-38: Update the indentation logic in the newText builder to
derive the block indentation token from the surrounding code instead of
hardcoding tab characters. Use the indentation style represented by baseIndent
when indenting body lines and the catch statement, while preserving the existing
try/catch structure and relative nesting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 499f9ecd-3320-4fb2-845e-d5c61d16d8fa
📒 Files selected for processing (6)
idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinCodeActionsMenu.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/SurroundWithTryCatchAction.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/SurroundWithTryCatch.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/SurroundWithTryCatchTest.ktresources/src/main/res/values/strings.xml
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
There was a problem hiding this comment.
Actionable comments posted: 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
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.kt`:
- Around line 33-34: Update ImplementMembersAction’s titleTextRes and tooltipTag
to the implement-members metadata that matches computeImplementMembersEdit and
its implementMembers action ID. Keep the action’s existing implementation
behavior unchanged rather than advertising override-superclass methods.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 16597bf3-5c70-4581-9982-3581eaa64f37
📒 Files selected for processing (6)
idetooltips/src/main/java/com/itsaky/androidide/idetooltips/TooltipTag.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/NullSafetyAction.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/OrganizeImportsAction.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/SurroundWithTryCatchAction.ktresources/src/main/res/values/strings.xml
💤 Files with no reviewable changes (1)
- resources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/SurroundWithTryCatchAction.kt
The tooltip work in "fix: tooltips" belongs to ADFA-4867, which wires every Kotlin code action to its own tag. Leaving it here made a try/catch PR the only thing carrying tag fixes for three already-merged tickets. Reverted from this branch: - The editor.codeactions.kotlin.organizeimports, .overridesuper and .nullsafetyfix constants, plus the tooltipTag wiring on Implement members, Null-safety fixes and Organize imports. - The Implement members label rename to "Override superclass methods", which also deleted the action_implement_members string. Per ADFA-4821 the Kotlin action covers properties and methods and stays "Implement members", so the string is restored. Deleting it would also have broken the build once ADFA-4867 landed, since that branch still references it. Kept: editor.codeactions.kotlin.trycatch and its wiring on the new Surround with try/catch action. This leaves the branch diff as try/catch only.
3 participants
Summary
Adds a Surround with try/catch code action to the Kotlin K2 LSP. On a selection in the editor's "Code actions" menu, it wraps the selected lines in:
Jira: ADFA-4613 (subtask of ADFA-3317, Integrate K2 compiler with LSP)
Approach
AddThrowsAction). The action operates on the current selection; with no selection it wraps the current line.computeSurroundWithTryCatchEdit(text, startLine, endLine)computes a single whole-line replaceTextEdit(columns ignored, matchingCommentLineAction). It computes indentation itself, so the result is correct even if the follow-up formatter is unavailable.SurroundWithTryCatchAction : BaseKotlinCodeActionreads the editor selection, calls the helper, and routes the edit throughlanguageClient.performCodeAction(...)withCommand.CMD_FORMAT_CODE, mirroringAddImportAction. Registered inKotlinCodeActionsMenu.Fixed
catch (e: Exception)with ane.printStackTrace()body. Out of scope (deliberately):@Throws/exception-type inference, a diagnostic-triggered variant, PSI statement-snapping, and surround-with for other constructs.Changes
lsp/kotlin/.../utils/SurroundWithTryCatch.kt- pure edit helper (new)lsp/kotlin/.../actions/SurroundWithTryCatchAction.kt- the code action (new)lsp/kotlin/.../KotlinCodeActionsMenu.kt- register the actionidetooltips/.../TooltipTag.kt- tooltip tag constantresources/.../values/strings.xml-action_surround_with_try_catchstringTesting
SurroundWithTryCatchTest, 5 cases: single line, indented multi-line block, blank-line preservation, whitespace-only no-op, out-of-range guards), asserting exactnewTextandRangeincluding computed indices.:lsp:kotlin:testV7DebugUnitTest-> 5/5 pass, no regressions..ktfile, run "Surround with try/catch" from the Code actions menu, confirm the block is wrapped and reindented.