ADFA-3753: handle file and build events to update kt source file index#1214
Merged
Conversation
Contributor
dara-abijo-adfa
left a comment
dara-abijo-adfa
left a comment
There was a problem hiding this comment.
Can this PR be broken down into smaller ones?
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
itsaky-adfa
force-pushed
the
fix/ADFA-3753
branch
from
5fb592e to
2bffbaf
Compare
Contributor
|
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:
📝 WalkthroughWalkthroughEventBus subscribers are added to ChangesKotlin Source Indexing via File Lifecycle Events
Sequence DiagramsequenceDiagram
actor FileSystem as File System
participant LSP as KotlinLanguageServer
participant EventBus as EventBus
participant Compiler as Compiler
participant Env as CompilationEnvironment
participant Index as KtSymbolIndex
participant ModuleScope as AbstractKtModule<br/>Search Scope
FileSystem->>EventBus: FileCreationEvent / FileRenameEvent
EventBus->>LSP: onFileCreated / onFileRenamed
LSP->>Compiler: compilationEnvironmentFor(path)
Compiler->>Env: onFileCreated / onFileMoved
Env->>Env: notifyElementModifiedForPath()
Env->>ModuleScope: invalidateSearchScope()
Env->>Index: submitForIndexing(path)
Index->>Index: scan + index commands
FileSystem->>EventBus: BuildCompletedEvent
EventBus->>LSP: onBuildCompleted
LSP->>Compiler: refreshSources()
Compiler->>Env: refreshSources()
Env->>Index: refreshSources()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/Compiler.kt (1)
72-82:⚠️ Potential issue | 🔴 Critical
.ktsfiles will crash the new EventBus handlers.
DocumentUtils.isKotlinFile(file)returnstruefor both.ktand.kts, so for a.ktspathcompilationEnvironmentFor(file)falls through tocompilationEnvironmentFor(CompilationKind.Script)which throwsUnsupportedOperationExceptionat Line 81. The new subscribers inKotlinLanguageServer(onFileCreated,onFileDeleted,onFileRenamed) all invokecompiler?.compilationEnvironmentFor(path)for any Kotlin path, so creating/deleting/renaming a.ktsfile will crash insidescope.launch { ... }and silently skip index maintenance (and log a coroutine exception).Either return
nullfor kinds that don't yet have an environment, or have the callers guard againstScript:🛠️ Suggested fix
fun compilationEnvironmentFor(compilationKind: CompilationKind): CompilationEnvironment = when (compilationKind) { CompilationKind.Default -> defaultCompilationEnv - CompilationKind.Script -> throw UnsupportedOperationException("Not supported yet") + // Script environment is not yet implemented; callers treat a null env + // as "no indexing/analysis for this file kind". + CompilationKind.Script -> defaultCompilationEnv // or: null, with signature change }If you prefer to keep the explicit failure, please guard every caller (including the new rename handler that looks up both
fromEnvandtoEnv) againstCompilationKind.Script.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/Compiler.kt` around lines 72 - 82, The crash happens because compilationEnvironmentFor(file: Path) delegates .kts files to compilationEnvironmentFor(CompilationKind.Script) which throws UnsupportedOperationException; change the behavior so compilationEnvironmentFor(compilationKind: CompilationKind) returns null for CompilationKind.Script (instead of throwing) and propagate null back to callers, or alternatively update callers in KotlinLanguageServer (onFileCreated, onFileDeleted, onFileRenamed) to guard against CompilationKind.Script by checking for null/unsupported before using the returned CompilationEnvironment; locate the two overloads compilationEnvironmentFor(file: Path) and compilationEnvironmentFor(compilationKind: CompilationKind) and update the Script branch to return null (or add null checks in the three KotlinLanguageServer subscriber methods) so .kts files do not trigger an exception.lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt (1)
183-188:⚠️ Potential issue | 🔴 CriticalCancel
scanningJobbefore joining to prevent potential hang — submittingStopdoes not cancel the scanning coroutine.The current implementation has a race condition:
ScanningWorker.scan()continuously submits commands viaindexWorker.submitCommand(), which callsscanChannel.send(). This suspend function blocks when the channel is full. Afterclose()submitsIndexCommand.Stop, theIndexWorkerstops consuming from the queue. IfScanningWorkeris mid-submission when this happens,scanChannel.send()will suspend indefinitely. TheisActivechecks inScanningWorkeronly trigger on explicit coroutine cancellation, not on theStopcommand. Therefore,scanningJob?.join()will hang waiting for a coroutine that is blocked trying to send to a full, no-longer-consumed channel.Use
scanningJob?.cancelAndJoin()to explicitly cancel the scanning coroutine before joining, or re-introduce a directscanningWorker.stop()method that sets an internal flag checked during iteration.Suggested change
suspend fun close() { + scanningJob?.cancelAndJoin() indexWorker.submitCommand(IndexCommand.Stop) - - scanningJob?.join() indexingJob?.join() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt` around lines 183 - 188, close() can hang because submitting IndexCommand.Stop doesn't cancel a scanning coroutine blocked in IndexWorker.submitCommand/scanChannel.send(); explicitly cancel the scanning job before joining. Update close() to call scanningJob?.cancelAndJoin() (or call a new ScanningWorker.stop() that sets a cancellation flag and then cancelAndJoin) prior to submitting IndexCommand.Stop and before indexingJob?.join(), ensuring ScanningWorker.scan() responds to cancellation and IndexWorker.submitCommand/scanChannel.send() cannot leave the coroutine permanently suspended.
🧹 Nitpick comments (2)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/ScanningWorker.kt (1)
29-34: Potentially noisy warning for legitimately-filtered files.When the Default-kind
ScanningWorkerwalks source roots that contain.kts(build scripts, tests, Gradle files) or other non-.ktfiles, each one will emit aWARNlog through this branch. Filtering by kind is expected behavior, so logging atdebug(or only warning whentoNioPathOrNull()itself returnsnull) would avoid polluting warnings with routine filtering:- .filter { - it.toNioPathOrNull()?.let { path -> kind.acceptsFile(path) } ?: run { - logger.warn("rejecting {} from kt source index", it) - false - } - } + .filter { + val path = it.toNioPathOrNull() + if (path == null) { + logger.warn("Could not convert {} to nio path; skipping", it) + return@filter false + } + kind.acceptsFile(path) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/ScanningWorker.kt` around lines 29 - 34, The logger.warn inside ScanningWorker's filtering lambda is noisy for routinely filtered files; change the branch so that when toNioPathOrNull() returns a Path and kind.acceptsFile(path) is false you log at debug (or not at all), but if toNioPathOrNull() returns null (indicating an unexpected/unparseable VirtualFile) keep or escalate to logger.warn including the file id; update the filter using the same toNioPathOrNull()/kind.acceptsFile check to only warn on null and use logger.debug for normal rejects to avoid spamming WARNs.lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt (1)
320-399: ConsiderThreadMode.ASYNCfor consistency with the other subscribers.All the pre-existing document-lifecycle subscribers in this class (
onDocumentOpen,onDocumentChange,onDocumentClose,onDocumentSaved) are annotated with@Subscribe(threadMode = ThreadMode.ASYNC). The newonBuildCompleted,onFileCreated,onFileDeleted, andonFileRenamedhandlers use the defaultPOSTINGmode, so their body (includingDocumentUtils.isKotlinFile,event.file.toPath(), and thescope.launchdispatch itself) runs synchronously on whichever thread posts the event — typically the main thread for file/build events. Matching the existing convention keeps index maintenance off the posting thread:- `@Subscribe` + `@Subscribe`(threadMode = ThreadMode.ASYNC) `@Suppress`("unused") fun onBuildCompleted(event: BuildCompletedEvent) {(Repeat for
onFileCreated,onFileDeleted,onFileRenamed.)Also worth flagging: because of the
UnsupportedOperationExceptionthrown byCompiler.compilationEnvironmentFor(CompilationKind.Script), these handlers will crash insidescope.launchfor any.ktspath (see the comment onCompiler.kt). Once that root cause is addressed, the handlers themselves look correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt` around lines 320 - 399, Update the four handlers (onBuildCompleted, onFileCreated, onFileDeleted, onFileRenamed) to use `@Subscribe`(threadMode = ThreadMode.ASYNC) and move any work that runs on the posting thread (calls to event.file.toPath(), DocumentUtils.isKotlinFile, and any index operations) inside the coroutine started by scope.launch so the posting thread does minimal work; also wrap calls to compiler?.compilationEnvironmentFor(...) in a try/catch (or null-check) to guard against the UnsupportedOperationException from Compiler.compilationEnvironmentFor(CompilationKind.Script) to avoid crashing when a .kts path is encountered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.kt`:
- Around line 64-68: The RemoveFromIndex branch computing filePath uses
cmd.vf.path fallback which differs from the insertion normalization in
SourceFileIndexer (which uses (virtualFile.toNioPathOrNull() ?:
backingFilePath)!!.pathString), causing mismatched keys on some platforms;
change the removal key computation in the IndexCommand.RemoveFromIndex handler
to use the same normalization used for insertion (i.e., prefer toNioPathOrNull()
and fall back to the same backingFilePath-derived Path then .pathString) so
fileIndex.remove and sourceIndex.removeBySource target the exact stored key, and
also remove or guard the use of !! so you handle the rare case both conversions
fail (e.g., skip removal or log an error) to avoid an NPE.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt`:
- Around line 115-120: The warning message in getVirtualFileOrWarn is hardcoded
to "cannot submit {} for indexing" though the function is used by
submitForIndexing, removeFromIndex, and onFileMoved; change getVirtualFileOrWarn
to accept a context/action string parameter (e.g., "submit for indexing",
"remove from index", "reindex after move") and update all callers
(submitForIndexing, removeFromIndex, onFileMoved) to pass the appropriate action
so the logger.warn prints the correct, contextual message when
path.toVirtualFileOrNull() returns null.
- Around line 130-144: removeFromIndex and onFileMoved silently return when
getVirtualFileOrWarn(path) fails; change the removal flow to use the path rather
than requiring a VirtualFile. Update the IndexCommand.RemoveFromIndex variant to
accept a java.nio.file.Path (or add an overload RemoveFromIndex(Path)) and
modify callers: in removeFromIndex submit IndexCommand.RemoveFromIndex(path)
instead of resolving VF, and in onFileMoved always submit RemoveFromIndex(from)
(by path) then submit ScanSourceFile(toVf) / IndexSourceFile(toVf) only after
resolving the target VF; also adjust indexWorker command handling to remove
entries by path from sourceIndex/fileIndex accordingly.
- Around line 86-113: syncIndexInBackground/refreshSources can start new
coroutines while previous ones are still doing pre-queue producer work and the
completing job can overwrite the shared job reference; fix by making indexingJob
and scanningJob atomic (e.g., AtomicReference<Job?>), when cancelling call
cancel() and then await completion (join or invokeOnCompletion/await) before
launching a replacement, and inside startIndexing/startScanning capture the
launched Job into a local val and only clear the shared AtomicReference via
compareAndSet(currentJob, null) when that local job completes; additionally move
any producer-side work that mutates shared state (e.g.,
sourceIndex.setActiveSources(), module iteration before enqueueing) into the
serialized worker or protect it with a mutex so two scans cannot concurrently
modify sourceIndex while one is still cancelling.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt`:
- Around line 385-398: The code incorrectly uses reference equality of
CompilationEnvironment (via compiler.compilationEnvironmentFor) to decide
same-kind moves, which can mis-handle .kt ↔ .kts renames and also can throw;
change the logic to determine the compilation kind for fromPath and toPath
(e.g., via a safe compilationKindFor(path) helper or by querying
CompilationEnvironment.getKind) and compare the CompilationKind values instead
of env instances; if kinds are equal and non-null call
ktSymbolIndex.onFileMoved(fromPath, toPath), otherwise ensure you
removeFromIndex(fromPath) on the old kind and submitForIndexing(toPath) on the
new kind; also guard calls to compiler.compilationEnvironmentFor (or the helper)
with try/catch so UnsupportedOperationException results in treating that side as
a distinct/unsupported kind (fallthrough to remove+submit).
---
Outside diff comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/Compiler.kt`:
- Around line 72-82: The crash happens because compilationEnvironmentFor(file:
Path) delegates .kts files to compilationEnvironmentFor(CompilationKind.Script)
which throws UnsupportedOperationException; change the behavior so
compilationEnvironmentFor(compilationKind: CompilationKind) returns null for
CompilationKind.Script (instead of throwing) and propagate null back to callers,
or alternatively update callers in KotlinLanguageServer (onFileCreated,
onFileDeleted, onFileRenamed) to guard against CompilationKind.Script by
checking for null/unsupported before using the returned CompilationEnvironment;
locate the two overloads compilationEnvironmentFor(file: Path) and
compilationEnvironmentFor(compilationKind: CompilationKind) and update the
Script branch to return null (or add null checks in the three
KotlinLanguageServer subscriber methods) so .kts files do not trigger an
exception.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt`:
- Around line 183-188: close() can hang because submitting IndexCommand.Stop
doesn't cancel a scanning coroutine blocked in
IndexWorker.submitCommand/scanChannel.send(); explicitly cancel the scanning job
before joining. Update close() to call scanningJob?.cancelAndJoin() (or call a
new ScanningWorker.stop() that sets a cancellation flag and then cancelAndJoin)
prior to submitting IndexCommand.Stop and before indexingJob?.join(), ensuring
ScanningWorker.scan() responds to cancellation and
IndexWorker.submitCommand/scanChannel.send() cannot leave the coroutine
permanently suspended.
---
Nitpick comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/ScanningWorker.kt`:
- Around line 29-34: The logger.warn inside ScanningWorker's filtering lambda is
noisy for routinely filtered files; change the branch so that when
toNioPathOrNull() returns a Path and kind.acceptsFile(path) is false you log at
debug (or not at all), but if toNioPathOrNull() returns null (indicating an
unexpected/unparseable VirtualFile) keep or escalate to logger.warn including
the file id; update the filter using the same toNioPathOrNull()/kind.acceptsFile
check to only warn on null and use logger.debug for normal rejects to avoid
spamming WARNs.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt`:
- Around line 320-399: Update the four handlers (onBuildCompleted,
onFileCreated, onFileDeleted, onFileRenamed) to use `@Subscribe`(threadMode =
ThreadMode.ASYNC) and move any work that runs on the posting thread (calls to
event.file.toPath(), DocumentUtils.isKotlinFile, and any index operations)
inside the coroutine started by scope.launch so the posting thread does minimal
work; also wrap calls to compiler?.compilationEnvironmentFor(...) in a try/catch
(or null-check) to guard against the UnsupportedOperationException from
Compiler.compilationEnvironmentFor(CompilationKind.Script) to avoid crashing
when a .kts path is encountered.
🪄 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: ad4d50d3-c66c-4668-a757-130024025058
📒 Files selected for processing (10)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationKind.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/Compiler.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexCommand.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/ScanningWorker.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DeclarationsProvider.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/utils/VirtualFileExts.kt
💤 Files with no reviewable changes (1)
- lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DeclarationsProvider.kt
Signed-off-by: Akash Yadav <itsaky01@gmail.com>
Signed-off-by: Akash Yadav <itsaky01@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt (1)
108-113:⚠️ Potential issue | 🟠 Major
refreshSources()can no-op after a prior cancellation leaves a staleindexingJobreference.
indexingJob ?: startIndexing()only starts indexing when the reference isnull, butstartIndexing()only nulls it via the normal-completion path at Line 97. When a previoussyncIndexInBackground()calledindexingJob?.cancel(), the coroutine typically propagatesCancellationExceptionout ofindexWorker.start(), so Line 97 is skipped and the reference stays non-null. A subsequent build-triggeredrefreshSources()will then see a non-null (but cancelled) job and never restart indexing — silently dropping all subsequent scan/index commands.Safer: either clear the reference in a
finallyinstartIndexing(), or checkindexingJob?.isActive == truehere.🛠️ Proposed fix
- private fun startIndexing() { - indexingJob = scope.launch { - indexWorker.start() - indexingJob = null - } - } - - private fun startScanning() { - scanningJob = scope.launch { - scanningWorker.scan() - scanningJob = null - } - } + private fun startIndexing() { + indexingJob = scope.launch { + try { + indexWorker.start() + } finally { + indexingJob = null + } + } + } + + private fun startScanning() { + scanningJob = scope.launch { + try { + scanningWorker.scan() + } finally { + scanningJob = null + } + } + } fun refreshSources() { - indexingJob ?: startIndexing() + if (indexingJob?.isActive != true) startIndexing() scanningJob?.cancel() startScanning() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt` around lines 108 - 113, refreshSources() can see a non-null but cancelled indexingJob and skip restarting; change its startup guard to check activity (e.g. call startIndexing() when indexingJob?.isActive != true) instead of only nullity, and also make startIndexing() robust by clearing the shared indexingJob reference in a finally block (set indexingJob = null) so a cancelled coroutine cannot leave a stale non-active reference; refer to refreshSources(), startIndexing(), indexingJob, syncIndexInBackground(), and indexWorker.start() when locating where to apply these changes.
🧹 Nitpick comments (2)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt (2)
122-124: Minor: indentation mixes spaces with the file's tab convention.Lines 123 and 136 use leading spaces while the surrounding code uses tabs. Likely an accidental paste — a quick reformat keeps the file consistent.
Also applies to: 135-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt` around lines 122 - 124, Lines in the suspend function submitForIndexing (around the getVirtualFileOrWarn call) mix spaces with the file's tab indentation; update those lines (including the ones at 123 and 135-137) to use the project's tab-based indentation style so the file is consistent, then run the file reformat/save to ensure no other mixed-indentation remains (look for submitForIndexing and getVirtualFileOrWarn to locate the area).
23-23: Remove unused imports from FIR internals.
org.jetbrains.kotlin.fir.resolve.toArrayOfFactoryName(line 23) andorg.jetbrains.kotlin.com.intellij.openapi.vfs.VirtualFileManager(line 21) are not referenced in this file and appear to be auto-import artifacts. Remove both to avoid unnecessary dependencies on FIR internals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt` at line 23, Remove the unused FIR/internal imports in KtSymbolIndex.kt: delete the import lines for org.jetbrains.kotlin.fir.resolve.toArrayOfFactoryName and org.jetbrains.kotlin.com.intellij.openapi.vfs.VirtualFileManager in the top of the file (they are not referenced anywhere in the KtSymbolIndex class or its methods), then run a quick compile or search to confirm no remaining references to those symbols (e.g., check usages around the KtSymbolIndex class and its methods) and commit the cleaned import list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt`:
- Around line 135-144: onFileMoved currently returns early when
getVirtualFileOrWarn(to, ...) yields null and therefore skips enqueuing the
path-based removal; always submit IndexCommand.RemoveFromIndex(from)
unconditionally before the VFS lookup, and only gate submitting
IndexCommand.ScanSourceFile(toVf) and IndexCommand.IndexSourceFile(toVf) on toVf
being non-null (i.e., call getVirtualFileOrWarn, but do not return early —
submit RemoveFromIndex(from) first, then if toVf != null submit the
ScanSourceFile and IndexSourceFile commands to indexWorker).
---
Duplicate comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt`:
- Around line 108-113: refreshSources() can see a non-null but cancelled
indexingJob and skip restarting; change its startup guard to check activity
(e.g. call startIndexing() when indexingJob?.isActive != true) instead of only
nullity, and also make startIndexing() robust by clearing the shared indexingJob
reference in a finally block (set indexingJob = null) so a cancelled coroutine
cannot leave a stale non-active reference; refer to refreshSources(),
startIndexing(), indexingJob, syncIndexInBackground(), and indexWorker.start()
when locating where to apply these changes.
---
Nitpick comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt`:
- Around line 122-124: Lines in the suspend function submitForIndexing (around
the getVirtualFileOrWarn call) mix spaces with the file's tab indentation;
update those lines (including the ones at 123 and 135-137) to use the project's
tab-based indentation style so the file is consistent, then run the file
reformat/save to ensure no other mixed-indentation remains (look for
submitForIndexing and getVirtualFileOrWarn to locate the area).
- Line 23: Remove the unused FIR/internal imports in KtSymbolIndex.kt: delete
the import lines for org.jetbrains.kotlin.fir.resolve.toArrayOfFactoryName and
org.jetbrains.kotlin.com.intellij.openapi.vfs.VirtualFileManager in the top of
the file (they are not referenced anywhere in the KtSymbolIndex class or its
methods), then run a quick compile or search to confirm no remaining references
to those symbols (e.g., check usages around the KtSymbolIndex class and its
methods) and commit the cleaned import 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
Run ID: f2d2d8cc-2718-424f-8f43-df1292d669be
📒 Files selected for processing (3)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexCommand.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt
Signed-off-by: Akash Yadav <itsaky01@gmail.com>
itsaky-adfa
force-pushed
the
fix/ADFA-3753
branch
from
622a577 to
671f6f5
Compare
Signed-off-by: Akash Yadav <itsaky01@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt (1)
193-198:⚠️ Potential issue | 🟠 Major
close()can hang waiting onscanningJob.
Stopis only handled byIndexWorker(via the index queue), soscanningWorker.scan()is never signalled to stop andscanningJob?.join()will block until scanning naturally completes. Ifclose()is invoked while a long scan is in progress, shutdown stalls. Cancel the scanning job explicitly before joining (or join indexing first, since indexing depends on scanning's output, then cancel+join scanning).♻️ Proposed fix
suspend fun close() { indexWorker.submitCommand(IndexCommand.Stop) - scanningJob?.join() indexingJob?.join() + scanningJob?.cancelAndJoin() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt` around lines 193 - 198, close() can hang because IndexCommand.Stop is handled only by IndexWorker and does not signal scanningWorker.scan(), so scanningJob?.join() may block; fix by ensuring scanningJob is cancelled before waiting: first wait for indexingJob (indexingJob?.join()) since indexing consumes scanning output, then call scanningJob?.cancel() and finally scanningJob?.join() to avoid a stuck shutdown, updating the close() implementation that submits IndexCommand.Stop to IndexWorker to perform this ordering and cancellation using the scanningJob and indexingJob symbols.
♻️ Duplicate comments (2)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt (2)
146-154:⚠️ Potential issue | 🟠 Major
onFileMovedstill skips removing the old entry when the destination VFS lookup fails.When
getVirtualFileOrWarn(to)returnsnull(e.g., the destination isn't yet visible to the VFS at event-handling time), the early return at Line 147 bypassesRemoveFromIndex(from)on Line 150. The index then keeps stale entries for the old path even though the move definitely happened — exactly what the path-basedRemoveFromIndexwas introduced to prevent. Removal doesn't depend on the VFS, so it should be enqueued unconditionally and only the scan/index steps gated ontoVf.🛠️ Proposed fix
suspend fun onFileMoved(from: Path, to: Path) { - val toVf = getVirtualFileOrWarn(to) ?: return + indexWorker.submitCommand(IndexCommand.RemoveFromIndex(from)) + val toVf = getVirtualFileOrWarn(to) ?: return indexWorker.apply { - submitCommand(IndexCommand.RemoveFromIndex(from)) submitCommand(IndexCommand.ScanSourceFile(toVf)) submitCommand(IndexCommand.IndexSourceFile(toVf)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt` around lines 146 - 154, In onFileMoved, always enqueue the path-based removal so stale entries are cleared: call indexWorker.submitCommand(IndexCommand.RemoveFromIndex(from)) unconditionally (before or regardless of getVirtualFileOrWarn(to) result), then only if getVirtualFileOrWarn(to) returns a non-null toVf submit the ScanSourceFile(toVf) and IndexSourceFile(toVf) commands; adjust the logic around getVirtualFileOrWarn, IndexCommand.RemoveFromIndex, IndexCommand.ScanSourceFile, IndexCommand.IndexSourceFile and indexWorker.submitCommand to enforce this ordering.
86-125:⚠️ Potential issue | 🟠 MajorCancel-then-relaunch without awaiting completion still allows two workers to consume the shared queue concurrently.
Job.cancel()is asynchronous, sosyncIndexInBackground()andrefreshSources()can launch a freshindexWorker.start()/scanningWorker.scan()while the previous one is still inside itscoroutineScope { while (isActive) … }loop. Both coroutines then read from the sameWorkerQueueand mutate the sharedfileIndex/sourceIndexconcurrently — including the locally constructedmodifiedFileIndexerdebouncer andscanCount/sourceIndexCountcounters inIndexWorker. The job-self-check in thefinallyblock fixes the ref-overwrite, but not the overlap.Additionally,
refreshSources()usesindexingJob ?: startIndexing(). A cancelled-but-not-yet-completedindexingJobis still non-null, so the truthy check skips restart and may leave no indexer running until the dangling coroutine finishes its finally block.Suggest awaiting cancellation (
cancelAndJoin()) before relaunching:♻️ Sketch
- fun syncIndexInBackground() { - indexingJob?.cancel() - startIndexing() - - scanningJob?.cancel() - startScanning() - } + fun syncIndexInBackground() { + scope.launch { + indexingJob?.cancelAndJoin() + startIndexing() + + scanningJob?.cancelAndJoin() + startScanning() + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt` around lines 86 - 125, syncIndexInBackground() and refreshSources() currently call indexingJob?.cancel() / scanningJob?.cancel() and immediately relaunch workers, allowing the old coroutine to still run and cause concurrent access; change both to await cancellation before starting new work by calling cancelAndJoin() on existing indexingJob and scanningJob (e.g., indexingJob?.cancelAndJoin(); scanningJob?.cancelAndJoin()) and only then call startIndexing() / startScanning(); also update refreshSources() so it restarts indexing when indexingJob is null or not active/completed (check job.isActive or use cancelAndJoin semantics) to avoid skipping a restart when a job is cancelled but not yet finished, and handle CancellationException/Interrupted exceptions from cancelAndJoin appropriately to preserve finally blocks in startIndexing()/startScanning().
🧹 Nitpick comments (1)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.kt (1)
62-136: Consider guarding each command dispatch against unexpected exceptions.A failure in any branch (e.g., a
nullbackingFilePathtriggering NPE on Line 100, a transient PSI error inIndexSourceFile, or an IO error inindexSourceFile(...)) propagates out oftake()and terminates the entire worker loop. After that, no furtherIndexCommand(includingRemoveFromIndex,Stop, modified-file events) will ever be processed for thisKtSymbolIndex, leaving the index permanently stale until the language server is restarted.Consider wrapping each iteration's
when (...)in a try/catch (rethrowingCancellationException) and logging+continuing on other failures so a single bad command does not silently kill background indexing.♻️ Sketch
while (isActive) { - when (val cmd = queue.take()) { + val cmd = queue.take() + try { + when (cmd) { is IndexCommand.RemoveFromIndex -> { val filePath = cmd.path.pathString fileIndex.remove(filePath) sourceIndex.removeBySource(filePath) } ... IndexCommand.Stop -> break + } + } catch (ce: kotlinx.coroutines.CancellationException) { + throw ce + } catch (t: Throwable) { + logger.error("Failed to process index command {}", cmd, t) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.kt` around lines 62 - 136, The worker loop in IndexWorker.kt currently processes commands from queue.take() without catching unexpected exceptions, so a single failure (e.g., NPE in IndexCommand.IndexModifiedFile via cmd.ktFile.backingFilePath, IO/PSI errors in indexSourceFile, etc.) can terminate the loop and leave the index stale; wrap the body that handles the when (val cmd = queue.take()) { ... } in a try/catch that rethrows CancellationException but catches Throwable for other failures, log the exception along with the command type (use logger.error with cmd::class or the IndexCommand variant), and continue to the next iteration so RemoveFromIndex, Stop and future commands are still processed; ensure IndexCommand.Stop is still honored (i.e., when Stop is received, break the loop after handling) and avoid swallowing errors from cancellation by rethrowing CancellationException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt`:
- Line 23: The import org.jetbrains.kotlin.fir.resolve.toArrayOfFactoryName in
KtSymbolIndex.kt is accidental and unused; remove that import statement from the
file (delete the line importing toArrayOfFactoryName), ensure there are no
references to toArrayOfFactoryName elsewhere in KtSymbolIndex (or any accidental
FIR symbol usage), and run a build/typecheck to confirm nothing else relied on
it.
---
Outside diff comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt`:
- Around line 193-198: close() can hang because IndexCommand.Stop is handled
only by IndexWorker and does not signal scanningWorker.scan(), so
scanningJob?.join() may block; fix by ensuring scanningJob is cancelled before
waiting: first wait for indexingJob (indexingJob?.join()) since indexing
consumes scanning output, then call scanningJob?.cancel() and finally
scanningJob?.join() to avoid a stuck shutdown, updating the close()
implementation that submits IndexCommand.Stop to IndexWorker to perform this
ordering and cancellation using the scanningJob and indexingJob symbols.
---
Duplicate comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt`:
- Around line 146-154: In onFileMoved, always enqueue the path-based removal so
stale entries are cleared: call
indexWorker.submitCommand(IndexCommand.RemoveFromIndex(from)) unconditionally
(before or regardless of getVirtualFileOrWarn(to) result), then only if
getVirtualFileOrWarn(to) returns a non-null toVf submit the ScanSourceFile(toVf)
and IndexSourceFile(toVf) commands; adjust the logic around
getVirtualFileOrWarn, IndexCommand.RemoveFromIndex, IndexCommand.ScanSourceFile,
IndexCommand.IndexSourceFile and indexWorker.submitCommand to enforce this
ordering.
- Around line 86-125: syncIndexInBackground() and refreshSources() currently
call indexingJob?.cancel() / scanningJob?.cancel() and immediately relaunch
workers, allowing the old coroutine to still run and cause concurrent access;
change both to await cancellation before starting new work by calling
cancelAndJoin() on existing indexingJob and scanningJob (e.g.,
indexingJob?.cancelAndJoin(); scanningJob?.cancelAndJoin()) and only then call
startIndexing() / startScanning(); also update refreshSources() so it restarts
indexing when indexingJob is null or not active/completed (check job.isActive or
use cancelAndJoin semantics) to avoid skipping a restart when a job is cancelled
but not yet finished, and handle CancellationException/Interrupted exceptions
from cancelAndJoin appropriately to preserve finally blocks in
startIndexing()/startScanning().
---
Nitpick comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.kt`:
- Around line 62-136: The worker loop in IndexWorker.kt currently processes
commands from queue.take() without catching unexpected exceptions, so a single
failure (e.g., NPE in IndexCommand.IndexModifiedFile via
cmd.ktFile.backingFilePath, IO/PSI errors in indexSourceFile, etc.) can
terminate the loop and leave the index stale; wrap the body that handles the
when (val cmd = queue.take()) { ... } in a try/catch that rethrows
CancellationException but catches Throwable for other failures, log the
exception along with the command type (use logger.error with cmd::class or the
IndexCommand variant), and continue to the next iteration so RemoveFromIndex,
Stop and future commands are still processed; ensure IndexCommand.Stop is still
honored (i.e., when Stop is received, break the loop after handling) and avoid
swallowing errors from cancellation by rethrowing CancellationException.
🪄 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: 41fc43b1-42db-4ffb-85cc-e0bc12c64c6e
📒 Files selected for processing (5)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexCommand.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/IndexWorker.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/ScanningWorker.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/ScanningWorker.kt
- lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt
Signed-off-by: Akash Yadav <itsaky01@gmail.com>
Signed-off-by: Akash Yadav <itsaky01@gmail.com>
itsaky-adfa
force-pushed
the
fix/ADFA-3753
branch
from
732f12c to
4094775
Compare
jatezzz
approved these changes
Apr 27, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt (1)
84-123:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftJob replacement still races with in-flight scan/index work.
Job.cancel()on Lines 85, 88, and 121 is asynchronous, so bothsyncIndexInBackground()andrefreshSources()can launch a replacement job before the previous one has actually finished. That still leaves a window where two scans overlap, and Line 119 also treats a cancelled-but-not-completeindexingJobas “running” because it only checks fornull.#!/bin/bash set -euo pipefail echo "=== KtSymbolIndex lifecycle ===" sed -n '84,123p' lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt echo echo "=== ScanningWorker pre-queue work ===" fd ScanningWorker.kt lsp/kotlin --type f -x rg -n -C3 'fun\s+scan\s*\(|setActiveSources|submitCommand' {} echo echo "=== IndexWorker start loop ===" fd IndexWorker.kt lsp/kotlin --type f -x rg -n -C3 'fun\s+start\s*\(|submitCommand|take|poll|Stop' {}The expected result is that
scan()still does producer-side work before enqueuing commands, which meanscancel()alone is not enough to prevent overlap; the relaunch path needsjoin/serialization rather than a null check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt` around lines 84 - 123, syncIndexInBackground() and refreshSources() race because cancel() is asynchronous and you can start new indexing/scanning before the previous coroutine has completed; change the launch-restart logic to wait for the old job to finish (serialize restarts) by calling indexingJob?.cancel() followed by indexingJob?.join() (and likewise for scanningJob) before starting a new job, and keep the existing finally block that nulls indexingJob/scanningJob but rely on equality comparison to the stored job instance (indexingJob / scanningJob) rather than coroutineContext identity to avoid races when replacing jobs.
🧹 Nitpick comments (2)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AbstractKtModule.kt (1)
35-41: ⚡ Quick winRedundant synchronization:
@Synchronizedplussynchronized(searchScopeLock)is double-locking.The
@Synchronizedannotation already synchronizes onthis, while the body synchronizes onsearchScopeLock. Either remove the annotation or remove the inner synchronized block for consistency with the rest of the class.♻️ Proposed fix
- `@Synchronized` fun invalidateSearchScope() { synchronized(searchScopeLock) { _baseSearchScope = null _contentScope = null } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AbstractKtModule.kt` around lines 35 - 41, The method invalidateSearchScope is double-locking via `@Synchronized` and an inner synchronized(searchScopeLock) block; remove the `@Synchronized` annotation and keep the synchronized(searchScopeLock) block so the method consistently synchronizes on searchScopeLock (update the declaration of invalidateSearchScope to no longer have `@Synchronized`).lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt (1)
326-330: 💤 Low valueConsider specifying
threadModeforonBuildCompletedor offloadingrefreshSources()to a coroutine.Unlike the document event handlers that use
ThreadMode.ASYNC,onBuildCompleteduses the defaultThreadMode.POSTING, meaning it runs on the poster's thread. Ifcompiler?.refreshSources()performs I/O or significant computation, this could block the calling thread.♻️ Suggested approach
- `@Subscribe` + `@Subscribe`(threadMode = ThreadMode.ASYNC) `@Suppress`("unused") fun onBuildCompleted(event: BuildCompletedEvent) { compiler?.refreshSources() }Or launch into a coroutine to match the pattern used by file event handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt` around lines 326 - 330, The onBuildCompleted subscriber currently runs on the poster's thread (default ThreadMode.POSTING) and may block; change the `@Subscribe` to specify ThreadMode.ASYNC or instead offload work by launching a coroutine and calling compiler?.refreshSources() on a background dispatcher (e.g., CoroutineScope(Dispatchers.IO).launch { compiler?.refreshSources() }) so refreshSources() does not block the poster thread; update the onBuildCompleted method accordingly and ensure any coroutine scope used is appropriate for the component lifecycle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt`:
- Around line 404-410: refreshSources() currently invalidates
ResolutionScopeProvider and ktSymbolIndex but doesn't clear module-level search
scopes, causing stale AbstractKtModule.searchScope caches to persist after
build-triggered refreshes; update refreshSources() to iterate the project's
Kotlin modules (use the same module collection used elsewhere in
CompilationEnvironment or reference AbstractKtModule instances) and call
invalidateSearchScope() on each module (or invoke the existing module-level
invalidation helper) before or after ktSymbolIndex.refreshSources() so module
search scopes are cleared during full refreshes.
- Around line 453-461: The delete/move handlers don't update the in-memory
open-file maps so stale keys remain; update onFileDeleted and onFileMoved to
also reconcile open-file state by calling the appropriate ktSymbolIndex methods
that manage open-buffer registrations and notify ProjectStructureProvider (e.g.,
invoke the same logic used when files are closed/opened) — specifically, after
invalidateOn(...) and ktSymbolIndex.removeFromIndex(path) in onFileDeleted call
the ktSymbolIndex method that unregisters open buffers and inform
ProjectStructureProvider to remove the path from its in-memory registration;
likewise in onFileMoved, after invalidateOn(fromPath)/invalidateOn(toPath) and
ktSymbolIndex.onFileMoved(fromPath,toPath), update ktSymbolIndex's open-file key
(move the buffer from fromPath to toPath) and notify ProjectStructureProvider so
the open buffer is registered under toPath and the stale fromPath key is cleared
(mirror the onFileClosed/onFileOpened reconciliation logic).
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AbstractKtModule.kt`:
- Around line 59-79: The equals/hashCode currently include mutable/derived
cached fields causing races and side effects; change equals and hashCode to use
only stable identity fields (e.g., id and directRegularDependencies) and remove
references to _baseSearchScope, _contentScope, searchScopeLock,
baseContentScope, contentScope, and direct* friend/dependsOn collections; ensure
neither equals nor hashCode calls getters that can populate caches (like
getOrCreateBaseSearchScope()) and do not read cache fields without acquiring
searchScopeLock or calling invalidateSearchScope(); update
AbstractKtModule.equals and AbstractKtModule.hashCode to compute equality/hash
solely from immutable identifiers and stable dependency sets.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtLibraryModule.kt`:
- Around line 127-152: KtLibraryModule's hashCode() is out of sync with
equals(): it includes applicationEnvironment, baseContentScope, libraryName,
binaryRoots, binaryVirtualFiles, and targetPlatform (and accesses
baseContentScope which triggers lazy computation) while equals() only compares
super.equals, id, contentRoots, isSdk, jvmTarget, and librarySources. Fix
hashCode() in class KtLibraryModule so it only uses the same fields equals()
checks (call super.hashCode(), then incorporate isSdk, id, contentRoots,
jvmTarget, and librarySources in the same nullable-safe way) and remove any
access to baseContentScope or other extra properties (applicationEnvironment,
libraryName, binaryRoots, binaryVirtualFiles, targetPlatform) to avoid side
effects and maintain symmetry with equals().
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtSourceModule.kt`:
- Around line 107-124: The equals/hashCode contract is broken: hashCode()
includes module, contentRoots, name, languageVersionSettings and targetPlatform
which equals() does not check (equals currently uses super.equals(other),
project and versions). Fix by making the two consistent—either remove those
extra fields from hashCode or add corresponding comparisons into equals; I
recommend updating equals(other: Any?) to, after the super.equals(other) check,
also compare module, contentRoots, name, languageVersionSettings and
targetPlatform (e.g., return super.equals(other) && project == other.project &&
versions == other.versions && module == other.module && contentRoots ==
other.contentRoots && name == other.name && languageVersionSettings ==
other.languageVersionSettings && targetPlatform == other.targetPlatform) so
every field used in hashCode() is also used in equals().
---
Duplicate comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.kt`:
- Around line 84-123: syncIndexInBackground() and refreshSources() race because
cancel() is asynchronous and you can start new indexing/scanning before the
previous coroutine has completed; change the launch-restart logic to wait for
the old job to finish (serialize restarts) by calling indexingJob?.cancel()
followed by indexingJob?.join() (and likewise for scanningJob) before starting a
new job, and keep the existing finally block that nulls indexingJob/scanningJob
but rely on equality comparison to the stored job instance (indexingJob /
scanningJob) rather than coroutineContext identity to avoid races when replacing
jobs.
---
Nitpick comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AbstractKtModule.kt`:
- Around line 35-41: The method invalidateSearchScope is double-locking via
`@Synchronized` and an inner synchronized(searchScopeLock) block; remove the
`@Synchronized` annotation and keep the synchronized(searchScopeLock) block so the
method consistently synchronizes on searchScopeLock (update the declaration of
invalidateSearchScope to no longer have `@Synchronized`).
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt`:
- Around line 326-330: The onBuildCompleted subscriber currently runs on the
poster's thread (default ThreadMode.POSTING) and may block; change the
`@Subscribe` to specify ThreadMode.ASYNC or instead offload work by launching a
coroutine and calling compiler?.refreshSources() on a background dispatcher
(e.g., CoroutineScope(Dispatchers.IO).launch { compiler?.refreshSources() }) so
refreshSources() does not block the poster thread; update the onBuildCompleted
method accordingly and ensure any coroutine scope used is appropriate for the
component lifecycle.
🪄 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: 4542f6ac-82a8-40fe-97ff-ca88e7edbde1
📒 Files selected for processing (12)
lsp/jvm-symbol-index/src/main/kotlin/org/appdevforall/codeonthego/indexing/jvm/KtFileMetadataIndex.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/KtSymbolIndex.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AbstractKtModule.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtLibraryModule.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtSourceModule.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/ProjectStructureProvider.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/ResolutionScopeProvider.ktlsp/kotlin/src/main/resources/META-INF/kt-lsp/kt-lsp.xmlsubprojects/kotlin-analysis-api/build.gradle.kts
…deleted Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt (2)
410-414:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalidate analysis caches during full refresh.
A build-triggered refresh currently updates only
ktSymbolIndex. The resolution-scope cache and eachAbstractKtModulesearch-scope cache survive, so post-build analysis can keep using pre-refresh file sets.Suggested fix
fun refreshSources() { + ResolutionScopeProvider.getInstance(project).invalidateAll() + modules.asFlatSequence().forEach { module -> + (module as? AbstractKtModule)?.invalidateSearchScope() + } ktSymbolIndex.refreshSources() // TODO: Should also update/notify Java file services about possibly changed Java files // But that's a bit problematic right now, scheduled for later }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt` around lines 410 - 414, refreshSources() currently only updates ktSymbolIndex, leaving the resolution-scope cache and each AbstractKtModule search-scope cache intact so post-build analysis can use stale data; modify refreshSources() to also invalidate the global resolution-scope cache and call the appropriate invalidate/clear on every AbstractKtModule search-scope (or invoke their provided reset/refresh methods) so all analysis caches are cleared during a full refresh; locate refreshSources(), ktSymbolIndex, and the AbstractKtModule search-scope/cache implementations to add calls that clear or rebuild those caches (ensure thread-safety consistent with existing refresh logic).
493-504:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove/move should also clear open-buffer state.
onFileRemoved()only mutates the index. If the file is open, pending diagnostics, thektSymbolIndexopen-file entry, andProjectStructureProvider's in-memory registration stay keyed to the deleted path;onFileMoved()inherits the same stale state by delegating to remove/create.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt` around lines 493 - 504, onFileRemoved currently only updates the symbol index and leaves open-buffer state, diagnostics, and ProjectStructureProvider registrations stale; update onFileRemoved(Path) to also clear any editor buffer state and pending diagnostics for that path, remove any open-file entry from ktSymbolIndex's open-file map, and unregister the path from ProjectStructureProvider's in-memory registration (use the same identifiers used when opening files). For onFileMoved(fromPath,toPath), instead of simply delegating to onFileRemoved/from onFileCreated, preserve or transfer open-buffer state and diagnostics if the file was open: remove registrations keyed by fromPath, move or re-key the buffer/diagnostic entries and ktSymbolIndex open-file entry to toPath, and update ProjectStructureProvider to re-register under toPath so no stale references remain.lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt (1)
381-389:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse kind equality for same-kind renames, not env instance equality.
After resolving
fromKind/toKind, Line 386 still decides "same-kind move" viafromEnv == toEnv. That keeps the.kt↔.ktsrename path fragile, and env resolution at Lines 383-384 can still throw becauseCompilationKind.Scripthas no environment yet.Suggested fix
- val fromEnv = fromKind?.let { compiler?.compilationEnvironmentFor(it) } - val toEnv = toKind?.let { compiler?.compilationEnvironmentFor(it) } + val fromEnv = runCatching { + fromKind?.let { compiler?.compilationEnvironmentFor(it) } + }.getOrNull() + val toEnv = runCatching { + toKind?.let { compiler?.compilationEnvironmentFor(it) } + }.getOrNull() - if (fromKind != null && fromEnv == toEnv && toEnv != null) { + if (fromKind != null && fromKind == toKind && toEnv != null) { // file was renamed within the same compilation environment toEnv.onFileMoved(fromPath, toPath) return@launch }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt` around lines 381 - 389, The current rename check compares environment instances (fromEnv == toEnv) which can be null or unavailable for some CompilationKind (e.g., Script); change the logic to first compare kinds using fromKind and toKind (e.g., if (fromKind != null && fromKind == toKind) { ... }) and only then obtain the compilation environment via compilationEnvironmentFor to call onFileMoved; ensure you guard the onFileMoved call by checking the resolved env is non-null (use fromKind/toKind, compilationEnvironmentFor, fromEnv/toEnv, and onFileMoved identifiers) so same-kind renames work even when one kind has no pre-existing env.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt`:
- Around line 443-457: The early return when path.toVirtualFileOrNull() is null
prevents the module/global fallback from running; instead, change the flow so
that virtualFile absence only skips the PsiManager/KtFile lookup and
KaSourceModificationService call, but still allows calling
ProjectStructureProvider.getInstance(project).findModuleForSourceId(path.pathString)
as the fallback: keep structureProvider assignment, set ktFile only when
virtualFile != null (using PsiManager.getInstance(project).findFile(virtualFile)
as? KtFile), call
KaSourceModificationService.getInstance(project).handleElementModification(ktFile,
typeProvider(ktFile)) only when ktFile != null, and then compute module using
the existing conditional that uses structureProvider.getModule(ktFile, null)
when ktFile != null else
structureProvider.findModuleForSourceId(path.pathString) and cast to
AbstractKtModule.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.kt`:
- Line 52: The unguarded "!!" when computing filePath can throw an unhelpful NPE
if both virtualFile?.toNioPathOrNull() and backingFilePath are null; in
SourceFileIndexer (the assignment to filePath used when converting KtFile to
metadata) replace the "!!" with checkNotNull (or requireNotNull) around the
entire expression and supply a clear message that includes which KtFile (or its
name/psi element) is missing both path sources so the error is descriptive;
ensure you reference the same expression virtualFile?.toNioPathOrNull() ?:
backingFilePath and keep the resulting .pathString access after the null-check.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AbstractKtModule.kt`:
- Around line 46-47: The contentScope getter dereferences _contentScope outside
the searchScopeLock because getOrCreateBaseSearchScope() releases the lock
before accessing _contentScope; to fix, ensure the read of _contentScope happens
while holding searchScopeLock (e.g., acquire searchScopeLock around both
getOrCreateBaseSearchScope() call and the subsequent `_contentScope!!` read or
refactor getOrCreateBaseSearchScope() to return the already-locked
_contentScope), so that invalidateSearchScope() cannot null out _contentScope
between those operations; update the contentScope property to perform the
null-check and return inside the lock using the existing searchScopeLock,
referencing contentScope, getOrCreateBaseSearchScope(), _contentScope,
searchScopeLock, and invalidateSearchScope().
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt`:
- Around line 327-337: The handlers onFileCreated (and the corresponding
onFileDeleted) call compiler.compilationEnvironmentFor(path) for all Kotlin
files, but that function throws for script files (CompilationKind.Script); guard
the call by detecting and skipping Kotlin script files (e.g., check
path.extension == "kts" or a DocumentUtils.isKotlinScript(path) helper) before
invoking compilationEnvironmentFor(path), and apply the same guard in the delete
handler so .kts events are skipped/logged instead of aborting the coroutine.
---
Duplicate comments:
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.kt`:
- Around line 410-414: refreshSources() currently only updates ktSymbolIndex,
leaving the resolution-scope cache and each AbstractKtModule search-scope cache
intact so post-build analysis can use stale data; modify refreshSources() to
also invalidate the global resolution-scope cache and call the appropriate
invalidate/clear on every AbstractKtModule search-scope (or invoke their
provided reset/refresh methods) so all analysis caches are cleared during a full
refresh; locate refreshSources(), ktSymbolIndex, and the AbstractKtModule
search-scope/cache implementations to add calls that clear or rebuild those
caches (ensure thread-safety consistent with existing refresh logic).
- Around line 493-504: onFileRemoved currently only updates the symbol index and
leaves open-buffer state, diagnostics, and ProjectStructureProvider
registrations stale; update onFileRemoved(Path) to also clear any editor buffer
state and pending diagnostics for that path, remove any open-file entry from
ktSymbolIndex's open-file map, and unregister the path from
ProjectStructureProvider's in-memory registration (use the same identifiers used
when opening files). For onFileMoved(fromPath,toPath), instead of simply
delegating to onFileRemoved/from onFileCreated, preserve or transfer open-buffer
state and diagnostics if the file was open: remove registrations keyed by
fromPath, move or re-key the buffer/diagnostic entries and ktSymbolIndex
open-file entry to toPath, and update ProjectStructureProvider to re-register
under toPath so no stale references remain.
In
`@lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.kt`:
- Around line 381-389: The current rename check compares environment instances
(fromEnv == toEnv) which can be null or unavailable for some CompilationKind
(e.g., Script); change the logic to first compare kinds using fromKind and
toKind (e.g., if (fromKind != null && fromKind == toKind) { ... }) and only then
obtain the compilation environment via compilationEnvironmentFor to call
onFileMoved; ensure you guard the onFileMoved call by checking the resolved env
is non-null (use fromKind/toKind, compilationEnvironmentFor, fromEnv/toEnv, and
onFileMoved identifiers) so same-kind renames work even when one kind has no
pre-existing env.
🪄 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: cd677913-0042-4ea8-8927-646ffeaab27c
📒 Files selected for processing (13)
lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/KotlinLanguageServer.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/CompilationEnvironment.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/Compiler.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/index/SourceFileIndexer.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AbstractKtModule.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtFileExts.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtSourceModule.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/DeclarationsProvider.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/ProjectStructureProvider.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/ResolutionScopeProvider.ktlsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/diagnostic/KotlinDiagnosticProvider.ktlsp/kotlin/src/main/resources/META-INF/kt-lsp/kt-lsp.xmlsubprojects/kotlin-analysis-api/build.gradle.kts
✅ Files skipped from review due to trivial changes (3)
- lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/KtSourceModule.kt
- lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/Compiler.kt
- subprojects/kotlin-analysis-api/build.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (2)
- lsp/kotlin/src/main/java/com/itsaky/androidide/lsp/kotlin/compiler/services/ProjectStructureProvider.kt
- lsp/kotlin/src/main/resources/META-INF/kt-lsp/kt-lsp.xml
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
jatezzz
approved these changes
May 5, 2026
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
5 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.
See ADFA-3753 for more details.