ADFA-3753: handle file and build events to update kt source file index#1214
ADFA-3753: handle file and build events to update kt source file index#1214itsaky-adfa wants to merge 8 commits intostagefrom
Conversation
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
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 58 minutes and 29 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds EventBus subscribers to KotlinLanguageServer to sync KtSymbolIndex with file lifecycle and build events; introduces CompilationKind-aware indexing, coroutine-driven scanning/indexing workers, new KtSymbolIndex APIs (submit/remove/onFileMoved/refresh), and index command for removals. Changes
Sequence Diagram(s)sequenceDiagram
participant FS as File System
participant EB as EventBus
participant KLS as KotlinLanguageServer
participant CMP as Compiler
participant KSI as KtSymbolIndex
participant IW as IndexWorker
participant SW as ScanningWorker
FS->>EB: file create / delete / rename
EB->>KLS: deliver file event (`@Subscribe`)
KLS->>KLS: determine kind via CompilationKind.acceptsFile(path)
alt create or rename → accepted
KLS->>KSI: submitForIndexing(path)
else delete or rename from accepted
KLS->>KSI: removeFromIndex(path)
else rename (both accepted)
KLS->>KSI: onFileMoved(from, to)
end
KSI->>IW: enqueue index/remove command
IW->>IW: process command
alt RemoveFromIndex
IW->>IW: remove entries from fileIndex & sourceIndex
else IndexSourceFile
IW->>SW: schedule scan commands
end
sequenceDiagram
participant Build as Build System
participant EB as EventBus
participant KLS as KotlinLanguageServer
participant CMP as Compiler
participant KSI as KtSymbolIndex
Build->>EB: BuildCompleted
EB->>KLS: deliver build-completed (`@Subscribe`)
KLS->>CMP: refreshSources()
CMP->>KSI: refreshSources()
KSI->>KSI: cancel/restart scanning & indexing jobs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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: 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>
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>
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
4 participants
See ADFA-3753 for more details.