ADFA-4747: Fix Add-import code action not appearing on unresolved references#1547
Merged
Conversation
…nces AddImportAction.prepare() gated visibility on findSymbolBySimpleName(name, 0), but the helper ended with Sequence.take(limit) and take(0) returns empty, so the action was invisible for every unresolved-reference diagnostic - dead since ADFA-3754. Treat limit <= 0 as unbounded, honoring the documented ReadableIndex.query contract the helper was violating. Also extract hasImportableClassifier and a non-suspend computeImportCandidates (runCatching, KtFile fetched before project.read) for testability and crash-safety, and drop the no-op CMD_FORMAT_CODE. Adds AddImportActionTest (visibility-gate regression) and EditExtsTest.
Replace the SQLite-backed visibility gate with an in-memory check on the unresolved-reference marker. prepare() runs synchronously on the UI thread during menu build, so resolving against the symbol index there was main-thread disk I/O; that resolution now happens only in the background execAction.
![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.
Contributor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough
Risks / best-practice concerns
WalkthroughThe Kotlin code-action flow now uses typed diagnostic extras for add-import and null-safety actions. Symbol lookup treats non-positive limits as unbounded, add-import failures display an error, tooltip metadata is specialized, and coverage expands for candidate resolution and import insertion. ChangesKotlin code-action resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DiagnosticProvider
participant AddImportAction
participant SymbolIndex
participant SourceFile
DiagnosticProvider->>AddImportAction: provide ResolveReference extra
AddImportAction->>SymbolIndex: find classifier candidates
SymbolIndex-->>AddImportAction: return matching symbols
AddImportAction->>SourceFile: compute import edits
SourceFile-->>AddImportAction: return candidate edits
AddImportAction->>AddImportAction: flash error when result is empty
Possibly related PRs
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 |
Signed-off-by: Akash Yadav <[email protected]>
jatezzz
approved these changes
Jul 21, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.kt (1)
49-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the changed action lifecycle paths.
This suite tests candidate computation but not the changed
prepare()visibility branches orpostExec()’smsg_no_imports_foundpath. Add focused tests for valid/invalid diagnostic metadata and empty execution results.As per coding guidelines, changed non-UI code should cover error and edge paths.
🤖 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/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.kt` around lines 49 - 107, Add focused tests for AddImportAction.prepare() covering both valid diagnostic metadata and invalid or missing metadata visibility branches, then add a postExec() test that exercises an empty execution result and verifies the msg_no_imports_found path. Reuse the existing test environment and action setup, keeping the tests narrowly scoped to these changed lifecycle behaviors.Source: Coding guidelines
🤖 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/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.kt`:
- Around line 11-13: Migrate both AddImportActionTest and EditExtsTest from
JUnit 4 to JUnit Jupiter by replacing their test imports and converting all
assertions to Truth assertions; apply the corresponding changes in
lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.kt
lines 11-13 and
lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/EditExtsTest.kt
lines 6-8.
---
Nitpick comments:
In
`@lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.kt`:
- Around line 49-107: Add focused tests for AddImportAction.prepare() covering
both valid diagnostic metadata and invalid or missing metadata visibility
branches, then add a postExec() test that exercises an empty execution result
and verifies the msg_no_imports_found path. Reuse the existing test environment
and action setup, keeping the tests narrowly scoped to these changed lifecycle
behaviors.
🪄 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
Run ID: 69d65373-72a2-44b1-8dc0-135ce64243ef
📒 Files selected for processing (5)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportActionTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/utils/EditExtsTest.ktresources/src/main/res/values/strings.xml
Contributor
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/NullSafetyAction.kt`:
- Around line 49-53: Update NullSafetyAction.execAction so cancellation is never
converted into an empty result: preserve the existing runCatching recovery for
specific recoverable failures, but rethrow any CancellationException caught
within it before returning the fallback list.
🪄 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: 984f8f54-427c-4b19-ab67-d5a4f540323b
📒 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/AddImportAction.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/BaseKotlinCodeAction.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/diagnostic/KotlinDiagnosticProvider.kt
💤 Files with no reviewable changes (1)
- lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/ImplementMembersAction.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/actions/AddImportAction.kt
Signed-off-by: Akash Yadav <[email protected]>
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
2 participants
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.
Jira: ADFA-4747
What & why
The "Add import" code action (shipped in ADFA-3754) never appeared on an unresolved-reference diagnostic - dead since it shipped.
KtSymbolIndex.findSymbolBySimpleNameended withSequence.take(limit)and its only caller passedlimit = 0, sotake(0)returned empty andprepare()always went invisible.limit <= 0as unbounded, honouring theReadableIndex.querycontract. ExtractcomputeImportCandidates(non-suspend,runCatching,KtFilebeforeproject.read); drop the no-opCMD_FORMAT_CODE.prepare()- decide from the in-memoryunresolvedReferencemarker; index resolution moves entirely to the backgroundexecAction.flashError) when a shown action resolves nothing.Tests
:lsp:kotlin:testV7DebugUnitTest --tests AddImportActionTest5/5 (incl. thetake(0)-unbounded regression guard);EditExtsTestcovers sorted-insert/dedup;:lsp:kotlin:assembleV8Debugbuilds.Merge gate (manual QA)
On-device: "No imports found" on a typo; single-candidate auto-apply vs multi-candidate picker; no main-thread jank.
Follow-ups (out of scope)
insertImportdoes not dedup a concrete import already covered by a wildcard (no impact today).Supersedes the PR originally opened under ADFA-4610 (that subtask tracks the original feature, delivered via ADFA-3754); the same commits are re-attached here under the correctly-scoped bug ADFA-4747.