fix: Cross-platform file open handler for .unit files (macOS double-click support) #528
Conversation
…acts - Add ARTIFACT type to AgentType enum - Create ArtifactAgentTemplate with Claude Artifacts-style prompt - Implement ArtifactAgent for HTML/JS/Python artifact generation - Add ArtifactRenderer interface for artifact-specific rendering - Create ArtifactPage with WebView preview and console panel - Implement platform-specific ArtifactPreviewPanel (JVM/Android/iOS/JS/WASM) - Add ArtifactCli with 7 test scenarios for HTML generation testing - Integrate artifact mode into AgentInterfaceRouter Related: #526
…s references to use new design system (Void, Text, Signal) - Fix VerticalResizableSplitPane parameters (top/bottom instead of first/second) - Add ARTIFACT case to all AgentType when expressions - Replace System.currentTimeMillis with kotlinx.datetime.Clock - Add SelectionContainer import for JVM preview panel - Simplify JVM ArtifactPreviewPanel with browser open button
…Panel - JVM: Use WebView with JS bridge for console.log capture - iOS: Use WebView with JS bridge for console.log capture - Android: Already uses native WebView (unchanged) - JS/WASM: Fallback to source view (WebView not available) - Add toggle between preview and source view - Add save and open in browser buttons
- Use ComposeRenderer directly instead of extending it - Reuse AgentMessageList for chat interface - Reuse DevInEditorInput for user input - Use KcefManager for WebView initialization status - Fix JS Bridge message handling (use JSON parsing) - Simplify iOS implementation to source view only - Add proper KCEF status display (idle/loading/error)
- Add StreamingArtifact data class for real-time preview during generation - Parse artifact content in real-time as chunks arrive - Show preview panel immediately when artifact tag is detected (not after complete) - Add streaming indicator overlay on preview panel - Add streaming indicator in top bar - Disable export during streaming - Fix unrelated try-catch around Composable in WebEditView.jvm.kt
- Use Main dispatcher for ViewModel state updates to trigger recomposition - Use derivedStateOf for proper state tracking - Show StreamingCodeView during generation (auto-scroll, real-time updates) - Switch to WebView preview only after generation completes - Similar to ThinkingBlockRenderer auto-scroll behavior - Fix smart cast issues with delegated properties
- BaseRenderer: only treat partial '<devin' as incomplete devin block - Avoid blocking streaming output for HTML/XML-heavy responses - JS: fix URL.createObjectURL usage in ArtifactPreviewPanel.js.kt
ArtifactAgent: fallback parse for fenced html blocks and raw HTML docs when autodev-artifact is missing Extract title from <title> when available
ArtifactAgent and subagents now use collect{} to preserve true streaming
- ArtifactBundle: self-contained artifact package with metadata - ArtifactBundlePacker: ZIP-based pack/unpack (JVM + JS stubs) - ARTIFACT.md: YAML frontmatter for progressive disclosure - context.json: AI context for Load-Back support - Update exportArtifact to support both .html and .unit formats
- Register .unit file type in nativeDistributions (macOS UTI, Windows, Linux) - UnitFileHandler: detect and load .unit files from command line args - Auto-switch to Artifact mode when launched with .unit file - DesktopUiState: support initial agent type
- Replace json.encodeToString(buildMap) with manual JSON string building - Add generateTestUnit task to verify ZIP file creation - Add ArtifactBundlePackerTest for pack/unpack verification
- Fix default file filter to use .unit format instead of HTML - Add exportArtifactBundle() for full bundle export with conversation history - Add loadFromBundle() and createBundleForExport() to ArtifactAgentViewModel - Support initialBundle parameter in ArtifactPage for Load-Back - Add exportArtifactBundle actual implementations for all platforms
…preview - Start WebView from about:blank and inject HTML after bridge is ready to avoid early console.log drops - Add pending logs queue and retry mechanism for bridge availability - Improve console message parsing to handle various JSON formats - Configure WebView settings to reduce internal
- Patch kmpJsBridge.onCallback to ignore callbackId=-1 (same workaround as WebEdit)
- Extract WebView console bridge to ArtifactConsoleBridgeJvm - Make console patching idempotent to avoid double-wrapping during streaming - Keep existing -1 onCallback suppression + buffering until bridge ready
- Color-coded levels with dot + badge (info/warn/error/log) - Merge consecutive identical logs into a single row with ×N counter
- Add initialBundle parameter to DesktopAutoDevApp, AutoDevApp, and AgentInterfaceRouter - Pass pending bundle from UnitFileHandler to DesktopAutoDevApp for .unit file launches - Clear pending bundle after consumption to prevent re-loading
- Add detailed logging for .unit file detection and bundle processing - Fix bundle persistence by storing pending bundle in local state - Improve agent type selection logic to respect ARTIFACT mode from .unit files - Add logging throughout bundle reception and loading flow
- Create FileOpenHandler expect/actual interface for cross-platform file opening - JVM: Uses Desktop.getDesktop().setOpenFileHandler() for macOS/Windows/Linux - Android/iOS/JS/WASM: Provide no-op implementations - Fixes compilation errors on non-JVM platforms - Follows AGENTS.md KMP best practices Closes #526
|
Warning Rate limit exceeded@phodal has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 52 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds an Artifact agent and end-to-end .unit bundle workflow: streaming LLM generation, artifact parsing/fallback/validation, ArtifactBundle model and packer (expect/actual platform implementations), platform preview/export UIs, file-open/startup wiring and CLI, executor parallelization default, and non-destructive shell timeout handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant ArtifactPage as ArtifactPage (UI)
participant VM as ArtifactAgentViewModel
participant Agent as ArtifactAgent
participant LLM as LLM Service
participant Renderer as ArtifactRenderer
participant Packer as ArtifactBundlePacker
User->>ArtifactPage: submit prompt
ArtifactPage->>VM: executeTask(prompt)
VM->>Agent: generate(prompt, onProgress)
Agent->>LLM: stream request
LLM-->>Agent: chunk
Agent->>VM: onProgress(chunk)
VM->>Renderer: render streaming preview
par Streaming continues
LLM-->>Agent: chunk...
Agent->>VM: onProgress(chunk)
VM->>Renderer: update preview
end
Agent->>Agent: parseArtifacts(fullResponse)
Agent->>Renderer: renderArtifact(id, type, title, content)
VM->>VM: save conversation/history
User->>ArtifactPage: export
ArtifactPage->>VM: createBundleForExport
VM->>Packer: pack(bundle, outputPath)
Packer-->>ArtifactPage: PackResult.Success
ArtifactPage->>User: notify success (path)
sequenceDiagram
autonumber
participant Desktop as Desktop App
participant Main as Main.kt
participant FileOpen as FileOpenHandler
participant UnitHandler as UnitFileHandler
participant Packer as ArtifactBundlePacker
participant Router as AgentInterfaceRouter
participant ArtifactPage as ArtifactPage
Desktop->>Main: start(args)
Main->>UnitHandler: hasUnitFile(args)?
alt unit file present
UnitHandler->>Packer: unpack(filePath)
Packer-->>UnitHandler: UnpackResult.Success(bundle)
UnitHandler->>Main: pendingBundle set
Main->>Router: start with initialBundle
Router->>ArtifactPage: load bundle
end
Note right of FileOpen: while running
Desktop->>FileOpen: OS file-open event
FileOpen->>Main: onFileOpen(path)
Main->>UnitHandler: processArgs([path])
UnitHandler->>Packer: unpack(path)
Packer-->>UnitHandler: UnpackResult.Success(bundle)
Main->>Router: switch to ARTIFACT, pass bundle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Pull request overview
This PR implements cross-platform file open handling for .unit artifact bundle files, enabling macOS double-click support through the expect/actual pattern. It introduces a comprehensive artifact generation system inspired by Claude's Artifacts, including artifact bundling, WebView preview, and Load-Back support.
Key Changes:
- Cross-platform
FileOpenHandlerusing expect/actual pattern for OS-level file association handling - Complete artifact system:
ArtifactAgent,ArtifactBundle,ArtifactBundlePackerfor reversible artifact generation - Platform-specific artifact preview panels with WebView integration and console bridging
- Parallel tool execution enabled by default with improved timeout handling
- macOS .unit file association configuration in build.gradle.kts
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| FileOpenHandler.kt (commonMain) | Expect interface for cross-platform file open events |
| FileOpenHandler.jvm.kt | Desktop API implementation for macOS/Windows/Linux |
| FileOpenHandler.{android,ios,js,wasmJs}.kt | Platform-specific no-op implementations |
| UnitFileHandler.kt | JVM handler for loading .unit bundle files |
| ArtifactAgent.kt | Core agent for generating HTML/Python/React artifacts |
| ArtifactBundle.kt | Bundle format with reversibility support |
| ArtifactBundlePacker.{kt,jvm.kt,js.kt} | Cross-platform ZIP pack/unpack |
| ArtifactPreviewPanel.*.kt | Platform-specific artifact preview with WebView |
| Main.kt | Integration of file open handler and bundle loading |
| CodingAgentExecutor.kt | Parallel tool execution implementation |
| PtyShellExecutor.kt, DefaultShellExecutor.jvm.kt | Timeout handling without process termination |
| build.gradle.kts | macOS file associations and ArtifactCli task |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // IMPORTANT: | ||
| // We only want to strip partial "<devin" tags (e.g. "<d", "<de", "<dev", "<devi", "<devin"), | ||
| // not any dangling "<" which is extremely common in streaming HTML/XML and would break streaming UX. | ||
| filtered = filtered.replace(Regex("<d(?:e(?:v(?:i(?:n)?)?)?)?$"), "") |
There was a problem hiding this comment.
The regex pattern change removes the match for a dangling < at the end of the string. While the comment explains this is intentional to avoid breaking streaming HTML/XML, this could miss incomplete <devin tags that are genuinely part of the DSL and not HTML. The pattern <d(?:e(?:v(?:i(?:n)?)?)?)?$ only matches sequences starting with <d, so a lone < at the end will no longer be detected. Consider if this could cause issues with incomplete devin blocks that don't start with <d.
| // Timeout occurred - DO NOT kill the process | ||
| // Let the AI decide whether to continue waiting or terminate | ||
| // The process remains alive and the session is still managed | ||
| logger().info { "Session ${session.sessionId} timed out after ${timeoutMs}ms, but process remains alive" } |
There was a problem hiding this comment.
The timeout behavior change is potentially problematic. When a timeout occurs, the process is left running but the function throws a ToolException. However, there's no mechanism shown here to track or clean up these orphaned processes. Over time, this could lead to resource leaks with many zombie processes accumulating. Consider adding process tracking or a cleanup mechanism for timed-out processes.
| // Timeout occurred - DO NOT kill the process | |
| // Let the AI decide whether to continue waiting or terminate | |
| // The process remains alive and the session is still managed | |
| logger().info { "Session ${session.sessionId} timed out after ${timeoutMs}ms, but process remains alive" } | |
| // Timeout occurred - clean up the underlying process to avoid leaks | |
| logger().info { "Session ${session.sessionId} timed out after ${timeoutMs}ms, terminating process" } | |
| val timeoutExitCode = try { | |
| // Attempt to forcibly terminate the process and wait briefly for it to exit | |
| ptyHandle.destroyForcibly() | |
| if (ptyHandle.waitFor(5, TimeUnit.SECONDS)) { | |
| runCatching { ptyHandle.exitValue() }.getOrDefault(-1) | |
| } else { | |
| -1 | |
| } | |
| } catch (cleanupError: Exception) { | |
| logger().warn(cleanupError) { | |
| "Failed to clean up timed-out PTY process for session ${session.sessionId}: ${cleanupError.message}" | |
| } | |
| -1 | |
| } | |
| session.markCompleted(timeoutExitCode) | |
| managedSession?.markCompleted(timeoutExitCode) |
| /** | ||
| * When true, only execute the first tool call per LLM response. | ||
| * This enforces the "one tool per response" rule even when LLM returns multiple tool calls. | ||
| * Default is true to prevent LLM from executing multiple tools in one iteration. | ||
| * Default is false to enable parallel tool execution for better performance. | ||
| */ | ||
| private val singleToolPerIteration: Boolean = true | ||
| private val singleToolPerIteration: Boolean = false |
There was a problem hiding this comment.
The parallel tool execution change switches singleToolPerIteration from true to false by default. This is a significant behavioral change that could affect system stability and resource usage. Multiple concurrent tool executions (especially shell commands) could overwhelm the system or cause race conditions. Consider making this configurable rather than changing the default, or add resource limits for parallel execution.
|
|
||
| ZipInputStream(BufferedInputStream(FileInputStream(inputFile))).use { zis -> | ||
| var entry: ZipEntry? = zis.nextEntry | ||
| while (entry != null) { | ||
| val outputFile = File(outputDirectory, entry.name) | ||
|
|
||
| // Security check: prevent zip slip attack | ||
| if (!outputFile.canonicalPath.startsWith(outputDirectory.canonicalPath)) { | ||
| throw SecurityException("Zip entry outside target directory: ${entry.name}") |
There was a problem hiding this comment.
The ZIP slip security check is good, but the canonicalPath comparison could fail on Windows with different drive letters or UNC paths. Additionally, the error message exposes the entry name which could leak sensitive path information. Consider using a more secure path validation approach and sanitizing the error message.
| ZipInputStream(BufferedInputStream(FileInputStream(inputFile))).use { zis -> | |
| var entry: ZipEntry? = zis.nextEntry | |
| while (entry != null) { | |
| val outputFile = File(outputDirectory, entry.name) | |
| // Security check: prevent zip slip attack | |
| if (!outputFile.canonicalPath.startsWith(outputDirectory.canonicalPath)) { | |
| throw SecurityException("Zip entry outside target directory: ${entry.name}") | |
| val targetDirPath = outputDirectory.canonicalFile.toPath().normalize() | |
| ZipInputStream(BufferedInputStream(FileInputStream(inputFile))).use { zis -> | |
| var entry: ZipEntry? = zis.nextEntry | |
| while (entry != null) { | |
| val outputFile = File(outputDirectory, entry.name) | |
| val outputPath = outputFile.toPath().toAbsolutePath().normalize() | |
| // Security check: prevent zip slip attack | |
| if (!outputPath.startsWith(targetDirPath)) { | |
| throw SecurityException("Zip entry is outside the target directory") |
| var webViewError by remember { mutableStateOf<String?>(null) } | ||
|
|
||
| if (webViewError != null) { | ||
| // Show error state instead of crashing | ||
| androidx.compose.foundation.layout.Box( | ||
| modifier = modifier, | ||
| contentAlignment = androidx.compose.ui.Alignment.Center | ||
| ) { | ||
| androidx.compose.material3.Text( | ||
| text = "WebView initialization failed: ${webViewError}", | ||
| color = androidx.compose.material3.MaterialTheme.colorScheme.error | ||
| ) | ||
| } | ||
| } else { | ||
| // Render WebView directly - errors should be handled at the KCEF initialization level | ||
| WebView( | ||
| state = webViewState, | ||
| navigator = webViewNavigator, | ||
| modifier = modifier, | ||
| captureBackPresses = false, | ||
| webViewJsBridge = jsBridge | ||
| ) | ||
| } |
There was a problem hiding this comment.
The webViewError state is introduced but never set to a value. There's no error handling that would populate this state when WebView initialization fails. This means the error UI path (lines 384-394) is unreachable dead code. Either implement proper error handling to set this state, or remove the unused error handling code.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/PtyShellExecutor.kt (2)
107-112: Excessive cleanup timeout.Line 110 waits for 3000000ms (50 minutes) after
destroyForcibly(). This should be 3000ms (3 seconds) or 5000ms (5 seconds) for process cleanup.🔎 Proposed fix
if (completed == null) { outputJob?.cancel() ptyProcess.destroyForcibly() - ptyProcess.waitFor(3000000, TimeUnit.MILLISECONDS) + ptyProcess.waitFor(3000, TimeUnit.MILLISECONDS) throw ToolException("Process timed out", ToolErrorType.TIMEOUT) }
61-66: Fix excessive cleanup timeout value.The cleanup timeout at line 64 uses
3000000ms(approximately 833 hours), which is clearly excessive. This appears to be a typo and should likely be3000ms(3 seconds) or5000ms(5 seconds) for a reasonable forcible termination grace period.Note: The difference in timeout behavior between this method (which kills the process) and
waitForSession(which intentionally leaves the process alive per its explicit comment) is an intentional design choice, not an inconsistency to fix.if (result == null) { // Timeout occurred - terminate process ptyProcess.destroyForcibly() - ptyProcess.waitFor(3000000, TimeUnit.MILLISECONDS) + ptyProcess.waitFor(3000, TimeUnit.MILLISECONDS) throw ToolException("Command timed out after ${config.timeoutMs}ms", ToolErrorType.TIMEOUT) }Similar issue exists at line 110 in
executeWithPtymethod.
🧹 Nitpick comments (22)
mpp-viewer-web/src/jvmMain/kotlin/cc/unitmesh/viewer/web/webedit/WebEditView.jvm.kt (1)
382-404: Complete the error handling or remove unused infrastructure.The
webViewErrorstate is declared but never set anywhere in the code, making the error UI path (lines 384-394) unreachable. This represents incomplete error handling infrastructure.Additionally, the comment on line 396 states "errors should be handled at the KCEF initialization level," which contradicts the presence of this local error state.
Consider one of the following:
- If error handling is needed here: Add logic to capture WebView initialization errors and set
webViewError. For example, wrap theWebViewcomposable in a try-catch or add error callbacks.- If errors are truly handled elsewhere: Remove the unused
webViewErrorstate and conditional logic to simplify the code.- If this is future-proofing: Add a TODO comment explaining when this state will be populated.
Example: Adding error capturing
If you intend to capture WebView errors here, you might add error handling like:
WebView( state = webViewState, navigator = webViewNavigator, modifier = modifier, captureBackPresses = false, webViewJsBridge = jsBridge, onCreated = { // Success case }, onError = { error -> webViewError = error.message ?: "Unknown WebView error" } )Note: Check the actual WebView API for available error callbacks.
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/schema/ToolResultFormatter.kt (1)
70-77: Consider renaming for clarity.The variable
isParallelis derived fromtoolResults.size > 1, which indicates multiple tools were executed but doesn't confirm they ran in parallel vs sequentially. A name likeisMultiToolorhasMultipleResultswould be more accurate.This is a minor naming nitpick; the formatting logic itself is correct.
mpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.js.kt (1)
9-20: LGTM - correct no-op implementation for JS platform.The implementation correctly follows the expect/actual pattern with appropriate no-op behavior and diagnostic logging for web apps.
Minor note: The log message uses emoji
📦which is fine for JS but consider consistency with the WASM implementation that uses the same emoji. As per coding guidelines, "Avoid emoji and UTF-8 characters in WASM code" - you may want to remove the emoji from both JS and WASM for consistency.mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/artifact/GenerateTestUnit.kt (1)
34-34: Consider cross-platform path handling.The hardcoded
/tmp/demo.unitpath is Unix-specific and won't work on Windows. Since this is a development utility, consider usingSystem.getProperty("java.io.tmpdir")for cross-platform compatibility.🔎 Proposed fix
- val outputPath = "/tmp/demo.unit" + val outputPath = "${System.getProperty("java.io.tmpdir")}/demo.unit"mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt (1)
147-159: Avoid side-effects directly in Composable body; useLaunchedEffect.The logging call on line 149 executes on every recomposition. Wrap it in a
LaunchedEffectkeyed oninitialBundleto log only when the value changes, consistent with howDesktopAutoDevApp.kthandles the same logging pattern.Also, as per coding guidelines, avoid emoji/UTF-8 characters in code that runs on WASM targets.
🔎 Proposed fix
AgentType.ARTIFACT -> { - // Log when ArtifactPage is rendered - cc.unitmesh.agent.logging.AutoDevLogger.info("AgentInterfaceRouter") { "📦 Rendering ArtifactPage with initialBundle: ${initialBundle?.name ?: "null"}" } + // Log when ArtifactPage is rendered (use LaunchedEffect to avoid logging on every recomposition) + androidx.compose.runtime.LaunchedEffect(initialBundle) { + cc.unitmesh.agent.logging.AutoDevLogger.info("AgentInterfaceRouter") { "Rendering ArtifactPage with initialBundle: ${initialBundle?.name ?: "null"}" } + } ArtifactPage(mpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.ios.kt (1)
75-93: Placeholder exports are acceptable.The no-op implementations with user notifications are appropriate for a platform that doesn't yet have share-sheet integration. The TODOs clearly document the intended future work.
Would you like me to help create an issue to track the iOS share-sheet export implementation?
mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePackerTest.kt (1)
106-155: Consider cleaning up temp files in CI environments.The test intentionally leaves files for manual inspection (lines 109, 151-154), which is useful during development but may cause disk pollution in CI. Consider conditionally cleaning up based on an environment variable or CI detection.
🔎 Proposed fix
@Test fun packedFileCanBeUnzippedWithStandardTools() = runBlocking { val tempDir = createTempDir("unzip") - // Don't delete this one to allow manual inspection + val keepForInspection = System.getenv("CI").isNullOrBlank() + + try { val artifact = ArtifactAgent.Artifact( // ... rest of test ) // ... test body - // Keep the file for manual inspection - println("\n📦 Test file kept at: ${outputFile.absolutePath}") - println(" To inspect: unzip -l ${outputFile.absolutePath}") - println(" To extract: unzip ${outputFile.absolutePath} -d /tmp/extracted") + if (keepForInspection) { + println("\n📦 Test file kept at: ${outputFile.absolutePath}") + println(" To inspect: unzip -l ${outputFile.absolutePath}") + println(" To extract: unzip ${outputFile.absolutePath} -d /tmp/extracted") + } + } finally { + if (!keepForInspection) { + tempDir.deleteRecursively() + } + } }mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.js.kt (1)
18-24: Unused variableserializedin pack function.The
serializedvariable is computed but never used. Either use it for the output or remove it to avoid dead code.🔎 Proposed fix
return try { // Use JSON serialization as fallback (can be enhanced with JSZip) - val serialized = ArtifactBundleUtils.serializeBundle(bundle) + ArtifactBundleUtils.serializeBundle(bundle) // Validate serialization works // In browser, this could trigger a download // In Node.js, this could write to file system console.log("Bundle packed (JSON fallback): ${bundle.name}")mpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/desktop/UnitFileHandler.kt (1)
106-110: Minor race condition potential inconsumePendingBundle.If multiple callers invoke
consumePendingBundleconcurrently, both could read the same non-null value before either sets it to null, returning the same bundle twice. Consider usinggetAndUpdatesemantics if strict single-consumption is required.🔎 Proposed fix using atomic update
fun consumePendingBundle(): ArtifactBundle? { - val bundle = _pendingBundle.value - _pendingBundle.value = null - return bundle + return _pendingBundle.getAndUpdate { null } }mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.jvm.kt (1)
50-62: Consider memory implications for large bundles.The
unpackfunction reads all ZIP entries into an in-memoryfilesmap before reconstruction. For very large bundles, this could cause memory pressure. For typical artifact sizes this should be acceptable, but consider documenting size limits or using streaming reconstruction for future scalability.mpp-ui/src/androidMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.android.kt (1)
70-88: LGTM!The export function stubs appropriately notify users that the feature is not yet implemented on Android. The TODO comments clearly indicate the intended implementation approach using Android's share intent and SAF.
Would you like me to help generate an implementation using Android's ShareCompat or Storage Access Framework for these export functions?
mpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.jvm.kt (1)
8-8: Unused import:java.io.File.The
Fileclass is imported but not used in this file. The files are accessed viaOpenFilesEvent.fileswhich returnsList<File>directly.🔎 Proposed fix
import java.awt.Desktop import java.awt.desktop.OpenFilesEvent import java.awt.desktop.OpenFilesHandler -import java.io.Filempp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactAgentViewModel.kt (2)
219-221: Use logger instead of println for consistency.The
printlnstatement is inconsistent with the rest of the codebase which usesAutoDevLogger. This affects log aggregation and filtering capabilities.🔎 Proposed fix
} catch (e: Exception) { - println("[ERROR] Failed to save conversation history: ${e.message}") + cc.unitmesh.agent.logging.AutoDevLogger.error("ArtifactAgentViewModel") { + "Failed to save conversation history: ${e.message}" + } }
132-168: Consider memoizing regex patterns for performance.The
artifactStartPatternRegex is recreated on every call toupdateStreamingArtifact. Since this is called for every streaming chunk, consider moving the pattern to a companion object or class-level property.🔎 Proposed fix
+ companion object { + private val ARTIFACT_START_PATTERN = Regex( + """<autodev-artifact\s+([^>]*)>""", + RegexOption.IGNORE_CASE + ) + } + private fun updateStreamingArtifact(content: String) { - val artifactStartPattern = Regex( - """<autodev-artifact\s+([^>]*)>""", - RegexOption.IGNORE_CASE - ) - - val startMatch = artifactStartPattern.find(content) ?: return + val startMatch = ARTIFACT_START_PATTERN.find(content) ?: returnmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/Main.kt (2)
83-95: Arbitrary delay is fragile; consider a more deterministic approach.The 500ms delay on line 89 to "ensure bundle is passed down" is a race-condition-prone pattern. If recomposition takes longer, the bundle could be cleared prematurely; if it's faster, there's unnecessary latency.
Also, the fully-qualified
kotlinx.coroutines.delayis inconsistent with the imports at the top of the file.🔎 Suggested approach
Consider clearing the pending bundle only after confirmation that the UI has consumed it (e.g., via a callback or state acknowledgment), rather than relying on a fixed delay. If a delay is truly needed, at minimum import
delayat the top:-import kotlinx.coroutines.runBlocking -import kotlinx.coroutines.delay +import kotlinx.coroutines.delay +import kotlinx.coroutines.runBlockingAnd then:
- kotlinx.coroutines.delay(500) // Longer delay to ensure bundle is passed down + delay(500) // TODO: Replace with deterministic consumption acknowledgment
235-239: Minor: Logging in composition may produce repeated output.The
.also { ... }block logs on every recomposition whereit != null. This is acceptable for debugging but consider guarding withLaunchedEffect(it)if log spam becomes an issue in production.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.kt (2)
86-97: Frontmatter parsing is simplistic; may fail on complex YAML.The
parseArtifactMdFrontmatterfunction splits lines by:with limit 2, which works for simplekey: valuepairs but fails for:
- Multi-line values
- Quoted strings containing colons
- Nested structures
Since the ARTIFACT.md format is controlled by this codebase (via
generateArtifactMd()), this is acceptable if the generated content stays simple. Consider adding a comment documenting this limitation.
143-149: Consider logging context parsing failures.The catch block silently swallows exceptions when parsing
context.json, falling back to an emptyArtifactContext. While the fallback is reasonable, logging the error would aid debugging when bundles fail to load correctly.🔎 Suggested improvement
val context = contextJson?.let { try { json.decodeFromString<ArtifactContext>(it) } catch (e: Exception) { + // Log parsing failure for debugging (consider adding a logger) + println("Warning: Failed to parse context.json: ${e.message}") ArtifactContext() } } ?: ArtifactContext()mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPage.kt (1)
131-133: SideEffect logs on every recomposition; consider guarding for production.
SideEffectruns after every successful recomposition, which could produce excessive logs. For debugging this is fine, but consider wrapping with a debug flag or removing before release.🔎 Suggested approach
- // Log for debugging using SideEffect to run on every recomposition - androidx.compose.runtime.SideEffect { - cc.unitmesh.agent.logging.AutoDevLogger.info("ArtifactPage") { "📦 [Render] displayArtifact: ${displayArtifact?.title ?: "null"}, showPreview=$showPreview, currentArtifact=${currentArtifact?.title}, viewModel.lastArtifact=${viewModel.lastArtifact?.title}" } - } + // Uncomment for debugging recomposition issues + // androidx.compose.runtime.SideEffect { + // cc.unitmesh.agent.logging.AutoDevLogger.info("ArtifactPage") { ... } + // }mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundle.kt (1)
108-112:generateIdcould produce collisions under high concurrency.The ID generation uses millisecond timestamp plus a random number from 0-999999. If multiple artifacts are created in the same millisecond, there's a ~0.0001% collision probability per pair. For typical usage this is fine, but consider using
kotlin.uuid.Uuid.random()if available, or a larger random range.mpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/ArtifactCli.kt (1)
180-191: Consider using a map for provider type lookup.The
whenexpression for provider mapping could be simplified to a map, making it easier to maintain and extend.🔎 Suggested simplification
private val providerMap = mapOf( "openai" to LLMProviderType.OPENAI, "anthropic" to LLMProviderType.ANTHROPIC, "google" to LLMProviderType.GOOGLE, "deepseek" to LLMProviderType.DEEPSEEK, "ollama" to LLMProviderType.OLLAMA, "openrouter" to LLMProviderType.OPENROUTER, "glm" to LLMProviderType.GLM, "qwen" to LLMProviderType.QWEN, "kimi" to LLMProviderType.KIMI ) // Usage: val providerType = providerMap[activeConfig.provider.lowercase()] ?: LLMProviderType.CUSTOM_OPENAI_BASEmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/ArtifactAgent.kt (1)
249-252: Self-closing tag list is incomplete but adequate for typical artifacts.The void elements filter (
br,hr,img,input,meta,link) covers common cases but misses some HTML5 void elements (area,base,col,embed,source,track,wbr). For AI-generated artifacts this is typically sufficient; consider expanding if validation issues arise.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
mpp-core/build.gradle.ktsmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/ArtifactAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/ArtifactAgentTemplate.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundle.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/CodingAgentExecutor.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolOrchestrator.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/ArtifactRenderer.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/BaseRenderer.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/scoring/LLMMetadataReranker.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/ChartAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/NanoDSLAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/PlotDSLAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/schema/ToolResultFormatter.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.js.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.jvm.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/artifact/GenerateTestUnit.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/DefaultShellExecutor.jvm.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/PtyShellExecutor.ktmpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePackerTest.ktmpp-ui/build.gradle.ktsmpp-ui/src/androidMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.android.ktmpp-ui/src/androidMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.android.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/CodingAgentPage.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactAgentViewModel.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPage.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuDesktop.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.ktmpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.ios.ktmpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.ios.ktmpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.js.ktmpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.js.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/Main.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/DesktopAutoDevApp.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactConsoleBridge.jvm.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.jvm.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.jvm.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/desktop/UnitFileHandler.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/ArtifactCli.ktmpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.wasmJs.ktmpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.wasmJs.ktmpp-viewer-web/src/commonMain/kotlin/cc/unitmesh/viewer/web/webedit/WebEditBridgeScript.ktmpp-viewer-web/src/jvmMain/kotlin/cc/unitmesh/viewer/web/webedit/WebEditView.jvm.kt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualpattern for platform-specific code (for example:Platformimplementations)
Use concrete classes (not interfaces) andPromise(notFlow) for @JsExport annotations
Avoid emoji and UTF-8 characters in WASM code
When modifyingCodingAgentRenderer, update ALL implementations:DefaultCodingAgentRenderer,ComposeRenderer,JewelRenderer,ServerSideRenderer, andJsRendererAdapterin Kotlin
When modifyingCodingAgentRenderer, update JVM CLI implementations:CodingCliRendererandConsoleRenderer
UseAutoDevColorsorMaterialTheme.colorSchemefor Compose styling
Files:
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/DefaultShellExecutor.jvm.ktmpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.wasmJs.ktmpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.ios.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/BaseRenderer.ktmpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.ios.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/PlotDSLAgent.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.ktmpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.js.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.jvm.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/artifact/GenerateTestUnit.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/desktop/UnitFileHandler.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.ktmpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.js.ktmpp-ui/src/androidMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.android.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.jvm.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/scoring/LLMMetadataReranker.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/schema/ToolResultFormatter.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/DesktopAutoDevApp.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactConsoleBridge.jvm.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/PtyShellExecutor.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/CodingAgentPage.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/CodingAgentExecutor.ktmpp-viewer-web/src/commonMain/kotlin/cc/unitmesh/viewer/web/webedit/WebEditBridgeScript.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactAgentViewModel.ktmpp-viewer-web/src/jvmMain/kotlin/cc/unitmesh/viewer/web/webedit/WebEditView.jvm.ktmpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePackerTest.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.js.ktmpp-ui/src/androidMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.android.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/ArtifactAgent.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.jvm.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/NanoDSLAgent.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/ArtifactCli.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolOrchestrator.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuDesktop.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/ChartAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/ArtifactAgentTemplate.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/ArtifactRenderer.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundle.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/Main.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPage.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.ktmpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.wasmJs.kt
**/*.{kt,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use hardcoded colors - always use design tokens from the design system
Files:
mpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/DefaultShellExecutor.jvm.ktmpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.wasmJs.ktmpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.ios.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/BaseRenderer.ktmpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.ios.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/PlotDSLAgent.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.ktmpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.js.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/AutoDevApp.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.jvm.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/artifact/GenerateTestUnit.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/desktop/UnitFileHandler.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.ktmpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.js.ktmpp-ui/src/androidMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.android.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.jvm.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/scoring/LLMMetadataReranker.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/tool/schema/ToolResultFormatter.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/DesktopAutoDevApp.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactConsoleBridge.jvm.ktmpp-core/src/jvmMain/kotlin/cc/unitmesh/agent/tool/shell/PtyShellExecutor.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/CodingAgentPage.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/CodingAgentExecutor.ktmpp-viewer-web/src/commonMain/kotlin/cc/unitmesh/viewer/web/webedit/WebEditBridgeScript.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactAgentViewModel.ktmpp-viewer-web/src/jvmMain/kotlin/cc/unitmesh/viewer/web/webedit/WebEditView.jvm.ktmpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePackerTest.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.js.ktmpp-ui/src/androidMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.android.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/ArtifactAgent.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.jvm.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/NanoDSLAgent.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/ArtifactCli.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/orchestrator/ToolOrchestrator.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuDesktop.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/ChartAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/ArtifactAgentTemplate.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/ArtifactRenderer.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundle.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/Main.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPage.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.ktmpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.wasmJs.kt
🧠 Learnings (6)
📚 Learning: 2025-12-07T08:40:10.859Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.859Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update ALL implementations: `DefaultCodingAgentRenderer`, `ComposeRenderer`, `JewelRenderer`, `ServerSideRenderer`, and `JsRendererAdapter` in Kotlin
Applied to files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/BaseRenderer.ktmpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.ios.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/PlotDSLAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/AgentType.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuMobile.ktmpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.js.ktmpp-ui/src/androidMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.android.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.jvm.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/DesktopAutoDevApp.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactConsoleBridge.jvm.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/CodingAgentPage.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/CodingAgentExecutor.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactAgentViewModel.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/ArtifactAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/NanoDSLAgent.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/ArtifactCli.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuDesktop.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/ArtifactAgentTemplate.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/ArtifactRenderer.ktmpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.wasmJs.kt
📚 Learning: 2025-12-07T08:40:10.859Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.859Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update JVM CLI implementations: `CodingCliRenderer` and `ConsoleRenderer`
Applied to files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/BaseRenderer.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/PlotDSLAgent.ktmpp-ui/build.gradle.ktsmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactConsoleBridge.jvm.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/CodingAgentPage.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/executor/CodingAgentExecutor.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/ArtifactAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/subagent/NanoDSLAgent.ktmpp-ui/src/jvmMain/kotlin/cc/unitmesh/server/cli/ArtifactCli.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/chat/TopBarMenuDesktop.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/ArtifactRenderer.kt
📚 Learning: 2025-12-07T08:40:10.859Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.859Z
Learning: Run `./gradlew :mpp-ui:generateI18n4kFiles` command when updating i18n resources
Applied to files:
mpp-core/build.gradle.kts
📚 Learning: 2025-12-07T08:40:10.859Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.859Z
Learning: Applies to **/*.kt : Use `expect`/`actual` pattern for platform-specific code (for example: `Platform` implementations)
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.kt
📚 Learning: 2025-12-07T08:40:10.859Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.859Z
Learning: Applies to mpp-vscode/src/**/*.ts : When modifying `CodingAgentRenderer`, update VSCode implementations: `mpp-vscode/src/bridge/mpp-core.ts` and `mpp-vscode/src/providers/chat-view.ts`
Applied to files:
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/CodingAgentPage.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/ArtifactRenderer.kt
📚 Learning: 2025-12-07T08:40:10.859Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.859Z
Learning: Applies to **/*.ts : When modifying `CodingAgentRenderer`, update ALL TypeScript implementations: `BaseRenderer.ts`, `CliRenderer.ts`, `ServerRenderer.ts`, and `TuiRenderer.ts`
Applied to files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/ArtifactRenderer.kt
🧬 Code graph analysis (5)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/AgentInterfaceRouter.kt (1)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt (1)
onAgentTypeChange(223-236)
mpp-core/src/jvmTest/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePackerTest.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (2)
assertTrue(29-33)assertEquals(45-49)
mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.js.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundle.kt (1)
getAllFiles(265-274)
mpp-ui/src/jvmMain/kotlin/cc/unitmesh/devins/ui/Main.kt (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/state/DesktopUiState.kt (1)
rememberDesktopUiState(90-93)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPage.kt (3)
mpp-ui/src/iosMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.ios.kt (2)
ArtifactPreviewPanel(21-69)exportArtifactBundle(87-93)mpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.js.kt (2)
ArtifactPreviewPanel(30-121)exportArtifactBundle(163-187)mpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.wasmJs.kt (2)
ArtifactPreviewPanel(23-85)exportArtifactBundle(101-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Build
| tasks.register<JavaExec>("generateTestUnit") { | ||
| group = "verification" | ||
| description = "Generate a test .unit file for verification" | ||
|
|
||
| val jvmCompilation = kotlin.jvm().compilations.getByName("main") | ||
| classpath(jvmCompilation.output, configurations["jvmRuntimeClasspath"]) | ||
| mainClass.set("cc.unitmesh.agent.artifact.GenerateTestUnitKt") | ||
| } |
There was a problem hiding this comment.
Add dependsOn("jvmMainClasses") for task reliability.
Unlike the runNanoDslScenarioHarness task (line 330), this task doesn't declare a dependency on jvmMainClasses. This may cause failures when running the task on a clean build.
🔎 Proposed fix
tasks.register<JavaExec>("generateTestUnit") {
group = "verification"
description = "Generate a test .unit file for verification"
+ dependsOn("jvmMainClasses")
val jvmCompilation = kotlin.jvm().compilations.getByName("main")
classpath(jvmCompilation.output, configurations["jvmRuntimeClasspath"])
mainClass.set("cc.unitmesh.agent.artifact.GenerateTestUnitKt")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tasks.register<JavaExec>("generateTestUnit") { | |
| group = "verification" | |
| description = "Generate a test .unit file for verification" | |
| val jvmCompilation = kotlin.jvm().compilations.getByName("main") | |
| classpath(jvmCompilation.output, configurations["jvmRuntimeClasspath"]) | |
| mainClass.set("cc.unitmesh.agent.artifact.GenerateTestUnitKt") | |
| } | |
| tasks.register<JavaExec>("generateTestUnit") { | |
| group = "verification" | |
| description = "Generate a test .unit file for verification" | |
| dependsOn("jvmMainClasses") | |
| val jvmCompilation = kotlin.jvm().compilations.getByName("main") | |
| classpath(jvmCompilation.output, configurations["jvmRuntimeClasspath"]) | |
| mainClass.set("cc.unitmesh.agent.artifact.GenerateTestUnitKt") | |
| } |
🤖 Prompt for AI Agents
In mpp-core/build.gradle.kts around lines 336 to 343, the generateTestUnit
JavaExec task lacks a declared dependency on the jvmMainClasses compilation,
which can cause failures on clean builds; add dependsOn("jvmMainClasses") to the
task registration so the JVM main classes are built before the task runs,
ensuring reliable execution.
| fun generatePackageJson(): String = buildString { | ||
| appendLine("{") | ||
| appendLine(" \"name\": \"${id.replace(Regex("[^a-z0-9-]"), "-").lowercase()}\",") | ||
| appendLine(" \"version\": \"$version\",") | ||
| appendLine(" \"description\": \"${description.replace("\"", "\\\"")}\",") | ||
|
|
||
| when (type) { | ||
| ArtifactType.HTML -> { | ||
| appendLine(" \"main\": \"index.html\",") | ||
| } | ||
| ArtifactType.REACT -> { | ||
| appendLine(" \"main\": \"index.js\",") | ||
| appendLine(" \"scripts\": {") | ||
| appendLine(" \"start\": \"react-scripts start\",") | ||
| appendLine(" \"build\": \"react-scripts build\",") | ||
| appendLine(" \"setup\": \"npm install\"") | ||
| appendLine(" },") | ||
| } | ||
| ArtifactType.PYTHON -> { | ||
| appendLine(" \"main\": \"index.py\",") | ||
| appendLine(" \"scripts\": {") | ||
| appendLine(" \"start\": \"python index.py\",") | ||
| appendLine(" \"setup\": \"pip install -r requirements.txt\"") | ||
| appendLine(" },") | ||
| } | ||
| else -> { | ||
| appendLine(" \"main\": \"index.${type.extension}\",") | ||
| } | ||
| } | ||
|
|
||
| if (dependencies.isNotEmpty()) { | ||
| appendLine(" \"dependencies\": {") | ||
| dependencies.entries.forEachIndexed { index, (key, value) -> | ||
| val comma = if (index < dependencies.size - 1) "," else "" | ||
| appendLine(" \"$key\": \"$value\"$comma") | ||
| } | ||
| appendLine(" },") | ||
| } | ||
|
|
||
| appendLine(" \"engines\": {") | ||
| appendLine(" \"node\": \">=18\"") | ||
| appendLine(" },") | ||
| appendLine(" \"artifact\": {") | ||
| appendLine(" \"type\": \"${type.name.lowercase()}\",") | ||
| appendLine(" \"generated\": true,") | ||
| appendLine(" \"loadBackSupported\": true") | ||
| appendLine(" }") | ||
| appendLine("}") | ||
| } |
There was a problem hiding this comment.
JSON escaping in generatePackageJson is incomplete.
Line 205 only escapes double quotes but not other JSON special characters (backslash, newlines, tabs). If description or id contains these characters, the generated JSON will be malformed.
🔎 Suggested fix
Create a helper function for proper JSON string escaping:
private fun escapeJsonString(s: String): String = s
.replace("\\", "\\\\")
.replace("\"", "\\\"")
.replace("\n", "\\n")
.replace("\r", "\\r")
.replace("\t", "\\t")
fun generatePackageJson(): String = buildString {
appendLine("{")
appendLine(" \"name\": \"${id.replace(Regex("[^a-z0-9-]"), "-").lowercase()}\",")
appendLine(" \"version\": \"$version\",")
appendLine(" \"description\": \"${escapeJsonString(description)}\",")
// ... rest of the method
}Alternatively, use kotlinx.serialization's Json.encodeToString() for the entire object.
| private fun extractHtmlFallback(response: String): String? { | ||
| // 1) Markdown fenced ```html block | ||
| val fenced = Regex("(?is)```\\s*html\\s*\\n([\\s\\S]*?)\\n```").find(response)?.groupValues?.get(1) | ||
| if (!fenced.isNullOrBlank()) return fenced | ||
|
|
||
| // 2) Raw HTML document | ||
| val lower = response.lowercase() | ||
| val doctypeIndex = lower.indexOf("<!doctype html") | ||
| val htmlIndex = lower.indexOf("<html") | ||
| val start = when { | ||
| doctypeIndex >= 0 -> doctypeIndex | ||
| htmlIndex >= 0 -> htmlIndex | ||
| else -> -1 | ||
| } | ||
| if (start < 0) return null | ||
| return response.substring(start) | ||
| } |
There was a problem hiding this comment.
HTML fallback may include trailing non-HTML content.
extractHtmlFallback finds the start of HTML (<!doctype or <html) but returns substring(start) without finding the closing </html>. If the LLM response has text after the HTML, it will be included in the artifact.
🔎 Suggested improvement
Consider finding the end of the HTML document:
if (start < 0) return null
- return response.substring(start)
+ // Try to find the end of the HTML document
+ val endIndex = response.lowercase().lastIndexOf("</html>")
+ return if (endIndex > start) {
+ response.substring(start, endIndex + 7) // 7 = "</html>".length
+ } else {
+ response.substring(start)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun extractHtmlFallback(response: String): String? { | |
| // 1) Markdown fenced ```html block | |
| val fenced = Regex("(?is)```\\s*html\\s*\\n([\\s\\S]*?)\\n```").find(response)?.groupValues?.get(1) | |
| if (!fenced.isNullOrBlank()) return fenced | |
| // 2) Raw HTML document | |
| val lower = response.lowercase() | |
| val doctypeIndex = lower.indexOf("<!doctype html") | |
| val htmlIndex = lower.indexOf("<html") | |
| val start = when { | |
| doctypeIndex >= 0 -> doctypeIndex | |
| htmlIndex >= 0 -> htmlIndex | |
| else -> -1 | |
| } | |
| if (start < 0) return null | |
| return response.substring(start) | |
| } | |
| private fun extractHtmlFallback(response: String): String? { | |
| // 1) Markdown fenced |
🤖 Prompt for AI Agents
In mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/ArtifactAgent.kt around
lines 195 to 211, the function returns response.substring(start) which can
include non-HTML text after the document; update it to locate the HTML document
end by searching (case-insensitively) for the last occurrences of closing tags
(preferably </html>, or fallback to </body> if present), and if found return the
substring from start through the end of that closing tag (include the tag
length); if no closing tag is found, fall back to the existing behavior and
return substring(start).
| private val llmService: KoogLLMService?, | ||
| private val chatHistoryManager: ChatHistoryManager? = null | ||
| ) { | ||
| private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) |
There was a problem hiding this comment.
Missing CoroutineScope cleanup - potential resource leak.
The scope created with SupervisorJob() is never cancelled. When this ViewModel is discarded, the scope remains active. Consider adding a cleanup() or onCleared() method to cancel the scope.
🔎 Proposed fix
class ArtifactAgentViewModel(
private val llmService: KoogLLMService?,
private val chatHistoryManager: ChatHistoryManager? = null
) {
private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default)
+ /**
+ * Clean up resources when ViewModel is no longer needed
+ */
+ fun cleanup() {
+ scope.cancel()
+ }
+
// ... rest of class📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) | |
| private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) | |
| /** | |
| * Clean up resources when ViewModel is no longer needed | |
| */ | |
| fun cleanup() { | |
| scope.cancel() | |
| } |
🤖 Prompt for AI Agents
In
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactAgentViewModel.kt
around line 27, the CoroutineScope created with SupervisorJob() +
Dispatchers.Default is never cancelled causing a potential resource leak; add
lifecycle cleanup by overriding onCleared() (or adding a public cleanup/close
method called by the owner) and call scope.cancel() there, or alternatively
store the Job and cancel it, ensuring any active coroutines are terminated when
the ViewModel is disposed.
| private fun openInNewTab(html: String) { | ||
| try { | ||
| val blob = Blob(arrayOf(html), BlobPropertyBag(type = "text/html")) | ||
| val url = URL.createObjectURL(blob) | ||
| window.open(url, "_blank") | ||
| } catch (e: Exception) { | ||
| println("Failed to open in new tab: ${e.message}") | ||
| } | ||
| } |
There was a problem hiding this comment.
Memory leak: Blob URL not revoked in openInNewTab.
The Blob URL created with URL.createObjectURL is never revoked. While browsers may garbage collect these eventually, explicitly revoking prevents memory accumulation when users repeatedly open new tabs.
🔎 Proposed fix
private fun openInNewTab(html: String) {
try {
val blob = Blob(arrayOf(html), BlobPropertyBag(type = "text/html"))
val url = URL.createObjectURL(blob)
window.open(url, "_blank")
+ // Revoke after a delay to allow the new tab to load the content
+ window.setTimeout({ URL.revokeObjectURL(url) }, 1000)
} catch (e: Exception) {
println("Failed to open in new tab: ${e.message}")
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun openInNewTab(html: String) { | |
| try { | |
| val blob = Blob(arrayOf(html), BlobPropertyBag(type = "text/html")) | |
| val url = URL.createObjectURL(blob) | |
| window.open(url, "_blank") | |
| } catch (e: Exception) { | |
| println("Failed to open in new tab: ${e.message}") | |
| } | |
| } | |
| private fun openInNewTab(html: String) { | |
| try { | |
| val blob = Blob(arrayOf(html), BlobPropertyBag(type = "text/html")) | |
| val url = URL.createObjectURL(blob) | |
| window.open(url, "_blank") | |
| // Revoke after a delay to allow the new tab to load the content | |
| window.setTimeout({ URL.revokeObjectURL(url) }, 1000) | |
| } catch (e: Exception) { | |
| println("Failed to open in new tab: ${e.message}") | |
| } | |
| } |
🤖 Prompt for AI Agents
In
mpp-ui/src/jsMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPreviewPanel.js.kt
around lines 126 to 134, the Blob URL created with URL.createObjectURL is never
revoked which can leak memory; update openInNewTab to revoke the object URL
after opening the new tab — e.g., keep the created url in a variable, call
window.open(url, "_blank"), then schedule URL.revokeObjectURL(url) (using a
short setTimeout like 1000ms) or place the revoke call in a finally block that
runs after open so the browser can still load the resource; ensure exceptions
are still caught and revoke runs even on error.
| private fun exportAsUnitBundle( | ||
| bundle: cc.unitmesh.agent.artifact.ArtifactBundle, | ||
| outputFile: File, | ||
| onNotification: (String, String) -> Unit | ||
| ) { | ||
| try { | ||
| // Pack bundle using coroutines | ||
| kotlinx.coroutines.runBlocking { | ||
| val packer = cc.unitmesh.agent.artifact.ArtifactBundlePacker() | ||
| when (val result = packer.pack(bundle, outputFile.absolutePath)) { | ||
| is cc.unitmesh.agent.artifact.PackResult.Success -> { | ||
| onNotification("success", "Artifact bundle exported to ${result.outputPath}") | ||
| } | ||
| is cc.unitmesh.agent.artifact.PackResult.Error -> { | ||
| onNotification("error", "Failed to export bundle: ${result.message}") | ||
| } | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| onNotification("error", "Failed to create bundle: ${e.message}") | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid runBlocking in UI code path.
runBlocking on line 504 blocks the calling thread. Since this is called from exportArtifact which runs on the UI thread, it can freeze the UI during packing. Consider making this a suspend function or accepting a coroutine scope.
🔎 Proposed fix
-/**
- * Export artifact as .unit bundle format
- */
-private fun exportAsUnitBundle(
+private suspend fun exportAsUnitBundle(
bundle: cc.unitmesh.agent.artifact.ArtifactBundle,
outputFile: File,
onNotification: (String, String) -> Unit
) {
try {
- // Pack bundle using coroutines
- kotlinx.coroutines.runBlocking {
- val packer = cc.unitmesh.agent.artifact.ArtifactBundlePacker()
- when (val result = packer.pack(bundle, outputFile.absolutePath)) {
- is cc.unitmesh.agent.artifact.PackResult.Success -> {
- onNotification("success", "Artifact bundle exported to ${result.outputPath}")
- }
- is cc.unitmesh.agent.artifact.PackResult.Error -> {
- onNotification("error", "Failed to export bundle: ${result.message}")
- }
+ val packer = cc.unitmesh.agent.artifact.ArtifactBundlePacker()
+ val result = kotlinx.coroutines.withContext(kotlinx.coroutines.Dispatchers.IO) {
+ packer.pack(bundle, outputFile.absolutePath)
+ }
+ when (result) {
+ is cc.unitmesh.agent.artifact.PackResult.Success -> {
+ onNotification("success", "Artifact bundle exported to ${result.outputPath}")
+ }
+ is cc.unitmesh.agent.artifact.PackResult.Error -> {
+ onNotification("error", "Failed to export bundle: ${result.message}")
}
}
} catch (e: Exception) {
onNotification("error", "Failed to create bundle: ${e.message}")
}
}Then update callers to use a coroutine scope or LaunchedEffect.
Committable suggestion skipped: line range outside the PR's diff.
| AutoDevLogger.info("FileOpenHandler") { | ||
| "📦 FileOpenHandler: Not applicable for WASM apps" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove emoji from WASM log message.
As per coding guidelines: "Avoid emoji and UTF-8 characters in WASM code." The 📦 emoji in the log message should be removed.
🔎 Proposed fix
AutoDevLogger.info("FileOpenHandler") {
- "📦 FileOpenHandler: Not applicable for WASM apps"
+ "FileOpenHandler: Not applicable for WASM apps"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AutoDevLogger.info("FileOpenHandler") { | |
| "📦 FileOpenHandler: Not applicable for WASM apps" | |
| } | |
| AutoDevLogger.info("FileOpenHandler") { | |
| "FileOpenHandler: Not applicable for WASM apps" | |
| } |
🤖 Prompt for AI Agents
In
mpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/ui/desktop/FileOpenHandler.wasmJs.kt
around lines 12 to 14, the log message includes an emoji which violates the
guideline to avoid emoji/UTF-8 characters in WASM code; remove the emoji so the
logged string uses only plain ASCII (e.g., change "📦 FileOpenHandler: Not
applicable for WASM apps" to "FileOpenHandler: Not applicable for WASM apps")
and keep the rest of the logging call unchanged.
🤖 Augment PR SummarySummary: This PR adds first-class support for AutoDev “artifact bundles” (the Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| val outputFile = File(outputDirectory, entry.name) | ||
|
|
||
| // Security check: prevent zip slip attack | ||
| if (!outputFile.canonicalPath.startsWith(outputDirectory.canonicalPath)) { |
There was a problem hiding this comment.
The zip-slip guard outputFile.canonicalPath.startsWith(outputDirectory.canonicalPath) can be bypassed by prefix-matching paths (e.g., .../demo_bad/... starts with .../demo). Consider requiring a path separator boundary (or equivalent canonical-path check) to make the extraction directory constraint strict.
🤖 Was this useful? React with 👍 or 👎
| environment = emptyMap(), | ||
| timeout = asyncShellConfig.maxWaitTimeoutMs // Use max timeout for shell commands | ||
| ) | ||
| val executionJobs = toolsToExecute.map { indexedToolCall -> |
There was a problem hiding this comment.
Now that tool calls run in parallel, toolOrchestrator.executeToolCall(...) (and downstream renderer/session state like ComposeRenderer.sessionResultChannels / _timeline) may be accessed concurrently, but several of these are backed by unsynchronized mutable state and were previously protected by sequential execution. This looks like a real race/consistency risk. (Guideline: thread_safety)
🤖 Was this useful? React with 👍 or 👎
- Convert McpBackend to expect/actual pattern for cross-platform support - JVM: Full implementation with MCP SDK integration for resources and tools - Android/JS/WASM: Basic implementations with MCP SDK support (read/write stubbed) - iOS: Stub implementation since MCP SDK is unavailable - Update tests to reflect platform-specific behavior - Add McpServerConfig data class for server configuration
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
mpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.wasmJs.kt (1)
51-53: Consider error handling for invalid JSON input.
bundleFromJsonreturns a non-nullableArtifactBundle, but ifArtifactBundleUtils.deserializeBundlethrows on malformed JSON, callers have no safe way to handle invalid input. Consider either returning a nullable type or catching/wrapping exceptions.🔎 Suggested approach (nullable return)
- fun bundleFromJson(json: String): ArtifactBundle { - return ArtifactBundleUtils.deserializeBundle(json) + fun bundleFromJson(json: String): ArtifactBundle? { + return try { + ArtifactBundleUtils.deserializeBundle(json) + } catch (e: Exception) { + null + } }mpp-core/src/iosMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.ios.kt (1)
51-53: Consider handling deserialization errors gracefully.
bundleFromJsonreturns a non-nullableArtifactBundle, butdeserializeBundlecan throw on invalid JSON input. This may cause unexpected crashes for callers who don't expect exceptions from this method.🔎 Option 1: Return nullable type with exception handling
- fun bundleFromJson(json: String): ArtifactBundle { - return ArtifactBundleUtils.deserializeBundle(json) + fun bundleFromJson(json: String): ArtifactBundle? { + return try { + ArtifactBundleUtils.deserializeBundle(json) + } catch (e: Exception) { + null + } }Alternatively, add
@Throwsannotation to document the throwing behavior for Swift interop.xiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/db/DatabaseDriverFactory.kt (1)
18-51: Consider adding runtime safety improvements to NoopSqlDriver.The current implementation has inconsistent behavior: core operations throw exceptions while lifecycle methods are silent no-ops. Consider these improvements:
- Add console warnings when core operations are called (before throwing) to aid debugging
- Document the behavior with KDoc explaining which methods throw vs which are safe
- Consider making the behavior more consistent across all methods
🔎 Proposed improvements for runtime safety
+/** + * No-op SQLDelight driver for WASM. + * Core database operations throw UnsupportedOperationException. + * Lifecycle and listener methods are safe no-ops. + * This driver exists only to satisfy compilation; database features are not supported on WASM. + */ private object NoopSqlDriver : SqlDriver { override fun <R> executeQuery( identifier: Int?, sql: String, mapper: (SqlCursor) -> QueryResult<R>, parameters: Int, binders: (SqlPreparedStatement.() -> Unit)?, ): QueryResult<R> { + console.warn("SQLDelight executeQuery called on WASM (not supported)") throw UnsupportedOperationException("SQLDelight driver not available on WASM") } override fun execute( identifier: Int?, sql: String, parameters: Int, binders: (SqlPreparedStatement.() -> Unit)?, ): QueryResult<Long> { + console.warn("SQLDelight execute called on WASM (not supported)") throw UnsupportedOperationException("SQLDelight driver not available on WASM") } override fun newTransaction(): QueryResult<Transacter.Transaction> { + console.warn("SQLDelight newTransaction called on WASM (not supported)") throw UnsupportedOperationException("SQLDelight driver not available on WASM") }xiuper-fs/build.gradle.kts (1)
16-24: Consider using a version catalog or property for Kotlin version.Hardcoding
2.2.0in multipleforce()calls creates a maintenance burden. If the project already defines a Kotlin version elsewhere (e.g., ingradle.propertiesor the version catalog), reference it here to ensure consistency and simplify upgrades.🔎 Suggested approach
+val kotlinVersion = libs.versions.kotlin.get() // or project property + configurations.all { resolutionStrategy { - force("org.jetbrains.kotlin:kotlin-stdlib:2.2.0") - force("org.jetbrains.kotlin:kotlin-stdlib-common:2.2.0") - force("org.jetbrains.kotlin:kotlin-stdlib-jdk8:2.2.0") - force("org.jetbrains.kotlin:kotlin-reflect:2.2.0") + force("org.jetbrains.kotlin:kotlin-stdlib:$kotlinVersion") + force("org.jetbrains.kotlin:kotlin-stdlib-common:$kotlinVersion") + force("org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlinVersion") + force("org.jetbrains.kotlin:kotlin-reflect:$kotlinVersion") } }xiuper-fs/src/iosMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.ios.kt (1)
17-43:IosMcpBackendclass appears unused.Since
createMcpBackendalways returnsnullon iOS (Line 50), this class is never instantiated. Consider removing it to reduce dead code, or document its purpose if it's intended for future use or testing.xiuper-fs/src/androidMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.android.kt (1)
19-57: Significant code duplication with WASM implementation.
AndroidMcpBackendis nearly identical toWasmMcpBackend. Consider extracting the shared logic into a common base implementation or utility functions in a shared source set (e.g., ajvmAndroidMainintermediate source set or helper functions in commonMain) to reduce maintenance burden.xiuper-fs/src/jsMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.js.kt (1)
61-68: Consider adding error logging for future implementation.The try-catch block silently suppresses exceptions when creating the MCP backend. While this is currently a placeholder returning null, when the TODO is implemented, silent exception handling will make debugging initialization failures difficult.
🔎 Suggested improvement
When implementing the MCP client creation, consider logging errors:
actual suspend fun createMcpBackend(serverConfig: McpServerConfig): McpBackend? { return try { // TODO: Implement actual MCP client creation for JS null // Placeholder for now } catch (e: Exception) { + console.error("Failed to create MCP backend for JS: ${e.message}", e) null } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
mpp-core/build.gradle.ktsmpp-core/src/androidMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.android.ktmpp-core/src/iosMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.ios.ktmpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.wasmJs.ktmpp-viewer-web/build.gradle.ktsxiuper-fs/build.gradle.ktsxiuper-fs/src/androidMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.android.ktxiuper-fs/src/commonMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.ktxiuper-fs/src/commonTest/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackendTest.ktxiuper-fs/src/commonTest/kotlin/cc/unitmesh/xiuper/fs/policy/PolicyTest.ktxiuper-fs/src/iosMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.ios.ktxiuper-fs/src/jsMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.js.ktxiuper-fs/src/jvmMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.jvm.ktxiuper-fs/src/jvmTest/kotlin/cc/unitmesh/xiuper/fs/mcp/McpFilesystemServerIntegrationTest.ktxiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/db/DatabaseDriverFactory.ktxiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.wasmJs.ktxiuper-ui/build.gradle.kts
💤 Files with no reviewable changes (1)
- mpp-viewer-web/build.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (1)
- mpp-core/build.gradle.kts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualpattern for platform-specific code (for example:Platformimplementations)
Use concrete classes (not interfaces) andPromise(notFlow) for @JsExport annotations
Avoid emoji and UTF-8 characters in WASM code
When modifyingCodingAgentRenderer, update ALL implementations:DefaultCodingAgentRenderer,ComposeRenderer,JewelRenderer,ServerSideRenderer, andJsRendererAdapterin Kotlin
When modifyingCodingAgentRenderer, update JVM CLI implementations:CodingCliRendererandConsoleRenderer
UseAutoDevColorsorMaterialTheme.colorSchemefor Compose styling
Files:
xiuper-fs/src/commonTest/kotlin/cc/unitmesh/xiuper/fs/policy/PolicyTest.ktxiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/db/DatabaseDriverFactory.ktxiuper-fs/src/commonMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.ktxiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.wasmJs.ktxiuper-fs/src/commonTest/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackendTest.ktxiuper-fs/src/jvmMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.jvm.ktmpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.wasmJs.ktmpp-core/src/iosMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.ios.ktmpp-core/src/androidMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.android.ktxiuper-fs/src/androidMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.android.ktxiuper-fs/src/jsMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.js.ktxiuper-fs/src/iosMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.ios.ktxiuper-fs/src/jvmTest/kotlin/cc/unitmesh/xiuper/fs/mcp/McpFilesystemServerIntegrationTest.kt
**/*.{kt,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use hardcoded colors - always use design tokens from the design system
Files:
xiuper-fs/src/commonTest/kotlin/cc/unitmesh/xiuper/fs/policy/PolicyTest.ktxiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/db/DatabaseDriverFactory.ktxiuper-fs/src/commonMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.ktxiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.wasmJs.ktxiuper-fs/src/commonTest/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackendTest.ktxiuper-fs/src/jvmMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.jvm.ktmpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.wasmJs.ktmpp-core/src/iosMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.ios.ktmpp-core/src/androidMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.android.ktxiuper-fs/src/androidMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.android.ktxiuper-fs/src/jsMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.js.ktxiuper-fs/src/iosMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.ios.ktxiuper-fs/src/jvmTest/kotlin/cc/unitmesh/xiuper/fs/mcp/McpFilesystemServerIntegrationTest.kt
🧠 Learnings (2)
📚 Learning: 2025-12-07T08:40:10.859Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.859Z
Learning: Applies to **/*.kt : Use `expect`/`actual` pattern for platform-specific code (for example: `Platform` implementations)
Applied to files:
xiuper-fs/src/commonTest/kotlin/cc/unitmesh/xiuper/fs/policy/PolicyTest.kt
📚 Learning: 2025-12-07T08:40:10.859Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.859Z
Learning: Always consider all platforms when working with Kotlin Multiplatform: JS, WASM, Desktop JVM, Android, iOS
Applied to files:
xiuper-fs/build.gradle.kts
🧬 Code graph analysis (1)
xiuper-fs/src/commonTest/kotlin/cc/unitmesh/xiuper/fs/policy/PolicyTest.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (2)
assertTrue(29-33)assertFalse(38-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test mpp-core (KMP)
- GitHub Check: Build
🔇 Additional comments (19)
mpp-core/src/wasmJsMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.wasmJs.kt (1)
10-27: LGTM - Proper stub implementation for WASM platform limitations.The
actualimplementation correctly follows the expect/actual pattern per coding guidelines. The error results with descriptive messages appropriately communicate the platform's limitations and guide users toward browser-based alternatives.xiuper-fs/src/commonTest/kotlin/cc/unitmesh/xiuper/fs/policy/PolicyTest.kt (5)
9-10: LGTM: Standard test library imports added.The addition of
assertTrueandassertFalseimports fromkotlin.testsupports the explicit assertion improvements throughout the file.
53-62: LGTM: Explicit type-check assertions improve test clarity.The conversion to
assertTruefor type checks is idiomatic and provides clearer failure messages when assertions fail.
64-72: LGTM: Explicit boolean assertions enhance test readability.Using
assertTruefor positive matches andassertFalsefor negative matches clearly expresses the test intent and follows Kotlin testing best practices.
75-94: LGTM: Explicit assertion improves test clarity.The use of
assertTrue(approvalRequested)is more explicit and idiomatic than the genericassert().
106-122: LGTM:assertFalseis clearer than negated assert.Using
assertFalse(approvalRequested)is more readable and idiomatic thanassert(!approvalRequested).mpp-core/src/androidMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.android.kt (2)
16-49: LGTM!The pack function correctly uses
withContext(Dispatchers.IO)for blocking I/O, properly manages resources with.use {}, and handles errors appropriately by wrapping exceptions intoPackResult.Error.
51-77: LGTM!The unpack function correctly reads ZIP entries into memory and delegates to
ArtifactBundleUtils.reconstructBundlefor deserialization. Resource management and error handling are properly implemented.mpp-core/src/iosMain/kotlin/cc/unitmesh/agent/artifact/ArtifactBundlePacker.ios.kt (1)
10-27: LGTM!This is a reasonable stub implementation for iOS. The error messages are clear and informative, guiding developers toward platform-specific solutions. This follows the expect/actual pattern as per the coding guidelines.
xiuper-ui/build.gradle.kts (2)
118-125: No changes needed. TheiosMainsource set is correctly configured for Kotlin 2.2.0's implicit hierarchical structure. The source set containsResourceLoader.ios.ktand requires no explicitdependsOn()declarations since the Gradle KMP plugin automatically establishes the dependency hierarchy (iosMain depends on commonMain, and platform-specific iOS targets depend on iosMain).Likely an incorrect or invalid review comment.
16-24: No action needed. Kotlin stdlib version 2.2.0 is correctly aligned with the Kotlin Gradle Plugin version.The forced Kotlin stdlib version already matches the Kotlin Gradle Plugin version defined in
gradle/libs.versions.toml(both are 2.2.0), so there is no version mismatch risk. The resolutionStrategy is appropriately ensuring dependency consistency.Likely an incorrect or invalid review comment.
xiuper-fs/build.gradle.kts (1)
99-148: Platform-specific MCP SDK dependency wiring looks correct.The per-target MCP SDK additions properly follow KMP best practices. iOS is explicitly documented as unsupported with a TODO for future support. Based on learnings, the approach correctly considers all platforms (JS, WASM, Desktop JVM, Android, iOS).
xiuper-fs/src/iosMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.ios.kt (1)
9-11: iOS stub implementation follows the expect/actual pattern correctly.The interface and factory function properly indicate MCP unavailability on iOS, returning
nullfrom the factory. This aligns with the PR's KMP best practices approach.Also applies to: 49-51
xiuper-fs/src/jvmMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.jvm.kt (1)
208-216: Factory returnsnulldespite full implementation being available.
JvmMcpBackendis fully implemented, butcreateMcpBackendalways returnsnull. This means the backend is unusable through the factory API. The integration test (Line 66 in McpFilesystemServerIntegrationTest.kt) bypasses this by directly constructingJvmMcpBackend(client).Is this placeholder intentional for this PR, with actual MCP client creation planned for a follow-up?
xiuper-fs/src/jvmTest/kotlin/cc/unitmesh/xiuper/fs/mcp/McpFilesystemServerIntegrationTest.kt (1)
27-27: Test correctly adapted to useJvmMcpBackend.The test properly instantiates
JvmMcpBackend(client)directly, which is necessary given the factory placeholder. The test name update accurately reflects the backend being tested.Also applies to: 66-66
xiuper-fs/src/commonTest/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackendTest.kt (1)
16-30: Config creation test validates data class properties correctly.The test properly verifies
McpServerConfigconstruction and property access.xiuper-fs/src/commonMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.kt (1)
20-25: Clean expect/actual pattern implementation.The expect interface correctly defines the minimum contract (
isAvailable), allowing platform-specific actuals to add additional members likemcpClientwhere applicable.McpServerConfigis well-designed with sensible defaults. This follows KMP best practices as per the coding guidelines.Also applies to: 31-31, 36-41
xiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.wasmJs.kt (1)
10-13: No action required. The interface pattern is correct and follows Kotlin's expect/actual mechanism.Each platform's actual interface correctly implements the expect interface from commonMain (
isAvailable). Platform-specific implementations can add additional members beyond what's in the expect—this is valid and intentional. iOS omitsmcpClientbecause MCP is not supported, while Android and WASM include it because they have MCP SDK support. No compilation issues exist with this design.Likely an incorrect or invalid review comment.
xiuper-fs/src/jsMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.js.kt (1)
10-13: LGTM! Proper use of expect/actual pattern.The interface declaration correctly implements the platform-specific actual interface for the JavaScript target, following the KMP best practices outlined in the coding guidelines.
| ZipInputStream(FileInputStream(inputPath)).use { zipIn -> | ||
| var entry: ZipEntry? = zipIn.nextEntry | ||
| while (entry != null) { | ||
| if (!entry.isDirectory) { | ||
| val outputFile = File(outputDirectory, entry.name) | ||
| outputFile.parentFile?.mkdirs() | ||
|
|
||
| FileOutputStream(outputFile).use { fileOut -> | ||
| zipIn.copyTo(fileOut) | ||
| } | ||
| } | ||
| zipIn.closeEntry() | ||
| entry = zipIn.nextEntry | ||
| } | ||
| } |
There was a problem hiding this comment.
Zip Slip vulnerability: validate entry paths before extraction.
The code directly uses entry.name from the ZIP file without validation. A malicious ZIP could contain entries with path traversal sequences (e.g., ../../../sensitive-file) or absolute paths, allowing files to be written outside the intended output directory.
🔎 Proposed fix to prevent Zip Slip
ZipInputStream(FileInputStream(inputPath)).use { zipIn ->
var entry: ZipEntry? = zipIn.nextEntry
while (entry != null) {
if (!entry.isDirectory) {
val outputFile = File(outputDirectory, entry.name)
+ // Prevent Zip Slip: ensure the output file is within the target directory
+ if (!outputFile.canonicalPath.startsWith(outputDirectory.canonicalPath + File.separator)) {
+ throw SecurityException("Zip entry is outside of the target dir: ${entry.name}")
+ }
outputFile.parentFile?.mkdirs()
FileOutputStream(outputFile).use { fileOut ->
zipIn.copyTo(fileOut)
}
}
zipIn.closeEntry()
entry = zipIn.nextEntry
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ZipInputStream(FileInputStream(inputPath)).use { zipIn -> | |
| var entry: ZipEntry? = zipIn.nextEntry | |
| while (entry != null) { | |
| if (!entry.isDirectory) { | |
| val outputFile = File(outputDirectory, entry.name) | |
| outputFile.parentFile?.mkdirs() | |
| FileOutputStream(outputFile).use { fileOut -> | |
| zipIn.copyTo(fileOut) | |
| } | |
| } | |
| zipIn.closeEntry() | |
| entry = zipIn.nextEntry | |
| } | |
| } | |
| ZipInputStream(FileInputStream(inputPath)).use { zipIn -> | |
| var entry: ZipEntry? = zipIn.nextEntry | |
| while (entry != null) { | |
| if (!entry.isDirectory) { | |
| val outputFile = File(outputDirectory, entry.name) | |
| // Prevent Zip Slip: ensure the output file is within the target directory | |
| if (!outputFile.canonicalPath.startsWith(outputDirectory.canonicalPath + File.separator)) { | |
| throw SecurityException("Zip entry is outside of the target dir: ${entry.name}") | |
| } | |
| outputFile.parentFile?.mkdirs() | |
| FileOutputStream(outputFile).use { fileOut -> | |
| zipIn.copyTo(fileOut) | |
| } | |
| } | |
| zipIn.closeEntry() | |
| entry = zipIn.nextEntry | |
| } | |
| } |
| actual suspend fun createMcpBackend(serverConfig: McpServerConfig): McpBackend? { | ||
| return try { | ||
| // TODO: Implement actual MCP client creation for Android | ||
| null // Placeholder for now | ||
| } catch (e: Exception) { | ||
| null | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Same issues as WASM: unnecessary try/catch and misleading availability.
The try/catch wraps nothing that can throw, and isAvailable = true in the class contradicts the factory always returning null.
🔎 Suggested fix
actual suspend fun createMcpBackend(serverConfig: McpServerConfig): McpBackend? {
- return try {
- // TODO: Implement actual MCP client creation for Android
- null // Placeholder for now
- } catch (e: Exception) {
- null
- }
+ // TODO: Implement actual MCP client creation for Android
+ return null
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| actual suspend fun createMcpBackend(serverConfig: McpServerConfig): McpBackend? { | |
| return try { | |
| // TODO: Implement actual MCP client creation for Android | |
| null // Placeholder for now | |
| } catch (e: Exception) { | |
| null | |
| } | |
| } | |
| actual suspend fun createMcpBackend(serverConfig: McpServerConfig): McpBackend? { | |
| // TODO: Implement actual MCP client creation for Android | |
| return null | |
| } |
| @Test | ||
| fun createMcpBackendReturnsNullForUnsupportedPlatforms() = runTest { | ||
| val config = McpServerConfig( | ||
| name = "test-server", | ||
| command = "test-command" | ||
| ) | ||
|
|
||
| // This should return null on platforms where MCP is not supported | ||
| // or when no actual MCP server is available | ||
| val backend = createMcpBackend(config) | ||
|
|
||
| // We don't assert anything specific about the result since it's platform-dependent | ||
| // On JVM/Android with proper MCP server setup, it might return a backend | ||
| // On iOS or without MCP server, it should return null | ||
| println("MCP backend creation result: ${backend != null}") | ||
| } |
There was a problem hiding this comment.
Test lacks meaningful assertions and cannot fail.
This test only prints the result without asserting anything. A test that cannot fail provides no value. Consider adding platform-specific expected values or at minimum asserting the return type.
🔎 Suggested improvement
@Test
fun createMcpBackendReturnsNullForUnsupportedPlatforms() = runTest {
val config = McpServerConfig(
name = "test-server",
command = "test-command"
)
- // This should return null on platforms where MCP is not supported
- // or when no actual MCP server is available
val backend = createMcpBackend(config)
- // We don't assert anything specific about the result since it's platform-dependent
- // On JVM/Android with proper MCP server setup, it might return a backend
- // On iOS or without MCP server, it should return null
- println("MCP backend creation result: ${backend != null}")
+ // Currently all platforms return null as factory is not implemented
+ // This test documents expected behavior and will catch regressions
+ assertTrue(backend == null, "Expected null from placeholder factory")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| fun createMcpBackendReturnsNullForUnsupportedPlatforms() = runTest { | |
| val config = McpServerConfig( | |
| name = "test-server", | |
| command = "test-command" | |
| ) | |
| // This should return null on platforms where MCP is not supported | |
| // or when no actual MCP server is available | |
| val backend = createMcpBackend(config) | |
| // We don't assert anything specific about the result since it's platform-dependent | |
| // On JVM/Android with proper MCP server setup, it might return a backend | |
| // On iOS or without MCP server, it should return null | |
| println("MCP backend creation result: ${backend != null}") | |
| } | |
| @Test | |
| fun createMcpBackendReturnsNullForUnsupportedPlatforms() = runTest { | |
| val config = McpServerConfig( | |
| name = "test-server", | |
| command = "test-command" | |
| ) | |
| val backend = createMcpBackend(config) | |
| // Currently all platforms return null as factory is not implemented | |
| // This test documents expected behavior and will catch regressions | |
| assertTrue(backend == null, "Expected null from placeholder factory") | |
| } |
🤖 Prompt for AI Agents
xiuper-fs/src/commonTest/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackendTest.kt
around lines 32-47: the test only prints the backend creation result and has no
assertions so it can never fail; update the test to assert expected behavior per
platform—use platform detection or JUnit assumptions to branch: on known
unsupported platforms assertNull(createMcpBackend(config)), on platforms where
MCP is expected assertNotNull(...) (or at minimum assertThat(result,
instanceOf(McpBackend::class.java)) to validate type), and use
assumeTrue/assumeFalse to skip platform-inapplicable assertions so the test
fails when behavior differs from the expected platform-specific outcome.
| override suspend fun delete(path: FsPath) { | ||
| throw FsException(FsErrorCode.EACCES, "Delete not supported for MCP resources") | ||
| } | ||
|
|
||
| override suspend fun mkdir(path: FsPath) { | ||
| throw FsException(FsErrorCode.EACCES, "Create directory not supported for MCP resources") | ||
| } |
There was a problem hiding this comment.
Inconsistent error codes: use ENOTSUP instead of EACCES for unimplemented operations.
The delete and mkdir methods throw FsException with EACCES (access denied), while read and write correctly use ENOTSUP (operation not supported). Since these operations are not implemented for the JS target rather than being permission-restricted, they should use ENOTSUP to avoid misleading callers about the nature of the failure.
🔎 Proposed fix
override suspend fun delete(path: FsPath) {
- throw FsException(FsErrorCode.EACCES, "Delete not supported for MCP resources")
+ throw FsException(FsErrorCode.ENOTSUP, "Delete not supported for MCP resources")
}
override suspend fun mkdir(path: FsPath) {
- throw FsException(FsErrorCode.EACCES, "Create directory not supported for MCP resources")
+ throw FsException(FsErrorCode.ENOTSUP, "Create directory not supported for MCP resources")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| override suspend fun delete(path: FsPath) { | |
| throw FsException(FsErrorCode.EACCES, "Delete not supported for MCP resources") | |
| } | |
| override suspend fun mkdir(path: FsPath) { | |
| throw FsException(FsErrorCode.EACCES, "Create directory not supported for MCP resources") | |
| } | |
| override suspend fun delete(path: FsPath) { | |
| throw FsException(FsErrorCode.ENOTSUP, "Delete not supported for MCP resources") | |
| } | |
| override suspend fun mkdir(path: FsPath) { | |
| throw FsException(FsErrorCode.ENOTSUP, "Create directory not supported for MCP resources") | |
| } |
🤖 Prompt for AI Agents
In xiuper-fs/src/jsMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.js.kt around
lines 49 to 55, the delete and mkdir methods throw FsException with
FsErrorCode.EACCES which incorrectly signals a permissions issue; update both to
throw FsException(FsErrorCode.ENOTSUP, "...") like read/write do so the error
correctly indicates the operation is not supported for the JS MCP backend.
| private var resourceCache: List<Resource>? = null | ||
| private var toolCache: List<Tool>? = null | ||
| private val toolArgsCache = mutableMapOf<String, JsonObject>() |
There was a problem hiding this comment.
Thread-safety concern with mutable caches.
resourceCache, toolCache, and toolArgsCache can be accessed concurrently by multiple coroutines. Since getResources() and getTools() use check-then-act patterns without synchronization, race conditions could cause redundant MCP calls or inconsistent state.
🔎 Suggested fix using Mutex
+import kotlinx.coroutines.sync.Mutex
+import kotlinx.coroutines.sync.withLock
class JvmMcpBackend(
override val mcpClient: Client
) : McpBackend {
override val isAvailable: Boolean = true
+ private val cacheMutex = Mutex()
private var resourceCache: List<Resource>? = null
private var toolCache: List<Tool>? = null
private val toolArgsCache = mutableMapOf<String, JsonObject>()
// ... in getResources():
- private suspend fun getResources(): List<Resource> {
- if (resourceCache == null) {
- val result = mcpClient.listResources()
- resourceCache = result.resources
- }
- return resourceCache ?: emptyList()
- }
+ private suspend fun getResources(): List<Resource> = cacheMutex.withLock {
+ resourceCache ?: mcpClient.listResources().resources.also { resourceCache = it }
+ }Committable suggestion skipped: line range outside the PR's diff.
| val resource = resources.find { it.uri == uri } | ||
| if (resource != null) { | ||
| FsStat(path, isDirectory = false, size = resource.name?.length?.toLong() ?: 0) | ||
| } else { | ||
| throw FsException(FsErrorCode.ENOENT, "Resource not found: $uri") | ||
| } |
There was a problem hiding this comment.
Incorrect size calculation: using name.length instead of actual content size.
Line 38 uses resource.name?.length?.toLong() as the file size, but the name length has no relation to the resource's actual content size. This will produce misleading FsStat values.
🔎 Suggested fix
val resource = resources.find { it.uri == uri }
if (resource != null) {
- FsStat(path, isDirectory = false, size = resource.name?.length?.toLong() ?: 0)
+ // Size is unknown until content is read; use null or 0 as placeholder
+ FsStat(path, isDirectory = false, size = null)
} else {Note: Also applies to Line 85 in list().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val resource = resources.find { it.uri == uri } | |
| if (resource != null) { | |
| FsStat(path, isDirectory = false, size = resource.name?.length?.toLong() ?: 0) | |
| } else { | |
| throw FsException(FsErrorCode.ENOENT, "Resource not found: $uri") | |
| } | |
| val resource = resources.find { it.uri == uri } | |
| if (resource != null) { | |
| // Size is unknown until content is read; use null or 0 as placeholder | |
| FsStat(path, isDirectory = false, size = null) | |
| } else { | |
| throw FsException(FsErrorCode.ENOENT, "Resource not found: $uri") | |
| } |
🤖 Prompt for AI Agents
In xiuper-fs/src/jvmMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.jvm.kt
around lines 36 to 41, the FsStat size currently uses resource.name?.length
which is incorrect; change it to compute the actual content size in bytes (e.g.,
use a byte-array/ByteArray length or a provided size field such as
resource.content?.size or resource.data?.size?.toLong(), or read the resource
input stream and get its available/byte count) and fall back to 0 if
unavailable; apply the same fix at the similar usage in list() around line 85 so
FsStat reflects real content size instead of name length.
| result.contents.firstOrNull()?.let { content -> | ||
| // TODO: Fix MCP SDK content access - API structure needs investigation | ||
| val bytes = content.toString().toByteArray() | ||
| ReadResult(bytes = bytes) | ||
| } ?: ReadResult(bytes = ByteArray(0)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n xiuper-fs/src/jvmMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.jvm.kt | head -150Repository: phodal/auto-dev
Length of output: 7044
🏁 Script executed:
cd xiuper-fs && find . -name "*.kt" -type f | head -20Repository: phodal/auto-dev
Length of output: 1439
🌐 Web query:
MCP Kotlin SDK ReadResourceResult content access API
💡 Result:
Short answer — ReadResourceResult exposes a contents list (List) where each item is either TextResourceContents or BlobResourceContents. For text resources read TextResourceContents.text; for binary resources read BlobResourceContents.blob (base64) and decode it. [1][2][3]
Kotlin example:
- server read handler returns ReadResourceResult(contents = listOf(TextResourceContents(text = "…", uri = req.uri, mimeType = "text/plain")))
- client reads and accesses:
val result = client.readResource(ReadResourceRequest(uri = "..."))
val first = result.contents.firstOrNull()
when (first) {
is TextResourceContents -> val text = first.text
is BlobResourceContents -> val bytes = Base64.getDecoder().decode(first.blob)
}
Sources: Kotlin SDK addResource / examples (ReadResourceResult usage). [1] ReadResourceResult/contents description (protocol SDK docs). [2] TextResourceContents docs. [3] BlobResourceContents docs.
References:
[1] https://modelcontextprotocol.github.io/kotlin-sdk/kotlin-sdk-server/io.modelcontextprotocol.kotlin.sdk.server/-server/add-resource.html
[2] https://modelcontextprotocol.github.io/csharp-sdk/api/ModelContextProtocol.Protocol.ReadResourceResult.html
[3] https://modelcontextprotocol.github.io/csharp-sdk/api/ModelContextProtocol.Protocol.BlobResourceContents.html
Use typed access for MCP SDK ResourceContents instead of toString().
The content object is either TextResourceContents or BlobResourceContents, not a generic object. Replace content.toString().toByteArray() with a when expression: access .text property for TextResourceContents and decode the base64 .blob property for BlobResourceContents. Calling toString() returns the object's debug representation rather than actual content.
🤖 Prompt for AI Agents
In xiuper-fs/src/jvmMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.jvm.kt
around lines 123-127, the code uses content.toString().toByteArray() which
yields a debug string instead of actual resource bytes; replace this with a
typed when on the ResourceContents: if it's TextResourceContents read the .text
property and get bytes (with proper charset), if it's BlobResourceContents
base64-decode the .blob string to bytes, then pass those bytes into
ReadResult(bytes=...) and keep the empty ByteArray fallback for the null case.
| actual class DatabaseDriverFactory { | ||
| actual fun createDriver(): SqlDriver { | ||
| // Requires browser environment; wasmJs tests/build should still compile. | ||
| return WebWorkerDriver( | ||
| WorkerJs.worker(), | ||
| ) | ||
| // WASM doesn't have a built-in SQLDelight driver here. | ||
| // This keeps compilation working; DB backend should not be used on WASM until a driver is provided. | ||
| return NoopSqlDriver | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and find the expect class definition
cd xiuper-fs
find . -name "DatabaseDriverFactory.kt" -type fRepository: phodal/auto-dev
Length of output: 493
🏁 Script executed:
# Check for the expect definition (likely in commonMain)
rg -l "expect.*DatabaseDriverFactory" --type=ktRepository: phodal/auto-dev
Length of output: 85
🏁 Script executed:
# Search for DatabaseDriverFactory usages
rg -n "DatabaseDriverFactory" --type=kt -A 2 -B 2Repository: phodal/auto-dev
Length of output: 85
🏁 Script executed:
# Search for SqlDriver usages that might inadvertently call the driver
rg -n "SqlDriver\|\.createDriver\(\)" --type=kt -A 2 -B 2 | head -100Repository: phodal/auto-dev
Length of output: 85
🏁 Script executed:
# Check for database operation invocations (executeQuery, execute, newTransaction)
rg -n "\.(executeQuery|execute|newTransaction)\(" --type=kt -A 2 -B 2 | head -100Repository: phodal/auto-dev
Length of output: 85
🏁 Script executed:
# Search specifically in wasmJsMain for database-related code
find . -path "*/wasmJsMain/*" -name "*.kt" | head -20Repository: phodal/auto-dev
Length of output: 1753
🏁 Script executed:
# Check if there's any conditional logic or @OptIn annotations that might bypass the noop driver
rg -n "expect\|actual\|OptIn\|platform" xiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/db/DatabaseDriverFactory.ktRepository: phodal/auto-dev
Length of output: 41
🏁 Script executed:
# Read the wasmJsMain DatabaseDriverFactory file
cat src/xiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/db/DatabaseDriverFactory.ktRepository: phodal/auto-dev
Length of output: 176
🏁 Script executed:
# Find the expect class definition in commonMain
cat src/xiuper-fs/src/commonMain/kotlin/cc/unitmesh/xiuper/fs/db/DatabaseDriverFactory.ktRepository: phodal/auto-dev
Length of output: 176
🏁 Script executed:
# Search for DatabaseDriverFactory usages across the codebase (without file type restriction)
rg "DatabaseDriverFactory" -A 3 -B 1 | head -50Repository: phodal/auto-dev
Length of output: 4322
🏁 Script executed:
# Search for createDriver calls and usage patterns
rg "createDriver\(\)" -A 3 -B 2 | head -80Repository: phodal/auto-dev
Length of output: 7382
🏁 Script executed:
# Search for classes/objects that might instantiate the driver (looking for db properties/fields)
rg "db\s*[:=]|Database|Driver" src/xiuper-fs/src/commonMain --max-count=20 | head -50Repository: phodal/auto-dev
Length of output: 127
🏁 Script executed:
# Get the full content of the commonMain DatabaseDriverFactory
cat xiuper-fs/src/commonMain/kotlin/cc/unitmesh/xiuper/fs/db/DatabaseDriverFactory.ktRepository: phodal/auto-dev
Length of output: 3097
🏁 Script executed:
# Get the full content of the wasmJsMain implementation
cat xiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/db/DatabaseDriverFactory.ktRepository: phodal/auto-dev
Length of output: 1772
🏁 Script executed:
# Search for where createDatabase is invoked to see if there are guards
rg "createDatabase\(" -B 5 -A 5 xiuper-fs/Repository: phodal/auto-dev
Length of output: 11857
🏁 Script executed:
# Search for platform checks or conditionals that might prevent WASM DB usage
rg "isWasm|wasmJs|platform\(\)" xiuper-fs/ | head -30Repository: phodal/auto-dev
Length of output: 333
🏁 Script executed:
# Check if WebWorkerDriver is actually used anywhere
rg "WebWorkerDriver|webWorker" xiuper-fs/Repository: phodal/auto-dev
Length of output: 387
🏁 Script executed:
# Look for any usages of DbFsBackend in WASM context
rg "DbFsBackend\|DatabaseDriverFactory" xiuper-fs/src/wasmJsMain/Repository: phodal/auto-dev
Length of output: 41
🏁 Script executed:
# Check if there are any platform-specific implementations of backends for WASM
find xiuper-fs/src/wasmJsMain -name "*.kt" -type f | xargs grep -l "Backend\|createDatabase" 2>/dev/nullRepository: phodal/auto-dev
Length of output: 137
🏁 Script executed:
# Check the build.gradle.kts to see what WebWorkerDriver library is available
cat xiuper-fs/build.gradle.kts | grep -A 20 -B 5 "webWorker\|sqldelight"Repository: phodal/auto-dev
Length of output: 2949
🏁 Script executed:
# Check if there are any imports or examples of WebWorkerDriver usage in the codebase
rg "WebWorkerDriver" --type=kotlinRepository: phodal/auto-dev
Length of output: 332
🏁 Script executed:
# Look at the McpBackend.wasmJs to see how it handles database-dependent code
cat xiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.wasmJs.ktRepository: phodal/auto-dev
Length of output: 2123
🏁 Script executed:
# Search for any documentation or comments about WASM database setup
rg "webWorker|sql\.js|IndexedDB|WASM.*[Dd]atabase" xiuper-fs/ | head -40Repository: phodal/auto-dev
Length of output: 469
🏁 Script executed:
# Check the mpp-ui WASM DatabaseDriverFactory implementation to see the correct pattern
cat mpp-ui/src/wasmJsMain/kotlin/cc/unitmesh/devins/db/DatabaseDriverFactory.ktRepository: phodal/auto-dev
Length of output: 910
🏁 Script executed:
# Verify if xiuper-fs WASM code ever initializes DbFsBackend or createDatabase
rg "DbFsBackend\(|createDatabase\(" xiuper-fs/ --max-count=20Repository: phodal/auto-dev
Length of output: 2096
🏁 Script executed:
# Check if there are any other DatabaseDriverFactory implementations with WebWorkerDriver pattern
rg "createDefaultWebWorkerDriver" --type=kotlinRepository: phodal/auto-dev
Length of output: 332
🏁 Script executed:
# Verify the README claim about WASM being production-ready
cat xiuper-fs/README.md | grep -A 10 -B 5 "WASM"Repository: phodal/auto-dev
Length of output: 1229
🏁 Script executed:
# Check if there's any actual usage of xiuper-fs WASM in the codebase
rg "xiuper-fs.*wasmJs" --type=kotlinRepository: phodal/auto-dev
Length of output: 41
🏁 Script executed:
# Check when the WebWorkerDriver dependencies were added and if there's a migration in progress
git log --oneline xiuper-fs/build.gradle.kts | head -10Repository: phodal/auto-dev
Length of output: 142
🏁 Script executed:
# See if there's any TODO or FIXME comments related to WASM database
rg "TODO|FIXME|XXX" xiuper-fs/src/wasmJsMain/ -A 2Repository: phodal/auto-dev
Length of output: 423
🏁 Script executed:
# Verify the exact commit and what was done for WASM support
git show e61846be --stat | head -30Repository: phodal/auto-dev
Length of output: 1736
🏁 Script executed:
# Check if the WebWorkerDriver API usage is documented in comments or in mpp-ui
grep -r "createDefaultWebWorkerDriver" xiuper-fs/Repository: phodal/auto-dev
Length of output: 41
🏁 Script executed:
# Final check: Search for any platform conditionals that might prevent WASM from using database
rg "expect|actual" xiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/db/ -B 2 -A 2Repository: phodal/auto-dev
Length of output: 855
Update the WASM DatabaseDriverFactory to use WebWorkerDriver or clearly document that database features are unsupported on WASM.
The current implementation returns NoopSqlDriver despite the build declaring sqldelight.webWorker and sqldelight.webWorker.wasmJs dependencies. Either:
- Use the proper driver pattern like mpp-ui does:
actual fun createDriver(): SqlDriver { return createDefaultWebWorkerDriver() }
- Or remove the unused WebWorkerDriver dependencies and update the README to clarify WASM database support is not yet implemented (not production-ready).
The NoopSqlDriver will throw UnsupportedOperationException if any code path accidentally invokes database operations, which is safer than silent failures but leaves the implementation incomplete relative to declared capabilities.
🤖 Prompt for AI Agents
In
xiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/db/DatabaseDriverFactory.kt
around lines 10 to 16, the WASM factory currently returns NoopSqlDriver while
the build includes sqldelight.webWorker and sqldelight.webWorker.wasmJs; update
this to either (A) return the actual WebWorker driver by calling the platform
helper used elsewhere (e.g., wire up and return createDefaultWebWorkerDriver()
or the equivalent WebWorker driver factory exported by your sqldelight setup) so
the declared dependencies are actually used, or (B) if you don’t intend to
support DB on WASM yet, remove the webWorker/webWorker.wasmJs dependencies and
update the README to state WASM DB is unsupported; ensure the chosen path avoids
silent mismatches (NoopSqlDriver currently throws UnsupportedOperationException
when invoked) and update imports and docs accordingly.
| class WasmMcpBackend( | ||
| override val mcpClient: Client | ||
| ) : McpBackend { | ||
| override val isAvailable: Boolean = true |
There was a problem hiding this comment.
Misleading isAvailable = true when factory always returns null.
WasmMcpBackend sets isAvailable = true, but createMcpBackend always returns null, meaning no backend is ever created. This is contradictory. Also, the try/catch in the factory is dead code since there's nothing that can throw before the null return.
🔎 Suggested fix
actual suspend fun createMcpBackend(serverConfig: McpServerConfig): McpBackend? {
- return try {
- // TODO: Implement actual MCP client creation for WASM
- null // Placeholder for now
- } catch (e: Exception) {
- null
- }
+ // TODO: Implement actual MCP client creation for WASM
+ return null
}Also applies to: 61-68
🤖 Prompt for AI Agents
In
xiuper-fs/src/wasmJsMain/kotlin/cc/unitmesh/xiuper/fs/mcp/McpBackend.wasmJs.kt
around lines 21 (and similarly 61-68), the class marks isAvailable = true while
the factory/createMcpBackend always returns null and contains a dead try/catch;
change the contract to be consistent by either implementing createMcpBackend to
return a real backend (and keep isAvailable = true) or, if no backend is
provided on this platform, set isAvailable = false and remove the unreachable
try/catch and null-return plumbing; update both occurrences (line 21 and lines
61-68) so availability and factory behavior match.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpp-core/build.gradle.ktsmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.ktmpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/CodingAgentPage.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/CodingAgentPage.kt
- mpp-core/build.gradle.kts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualpattern for platform-specific code (for example:Platformimplementations)
Use concrete classes (not interfaces) andPromise(notFlow) for @JsExport annotations
Avoid emoji and UTF-8 characters in WASM code
When modifyingCodingAgentRenderer, update ALL implementations:DefaultCodingAgentRenderer,ComposeRenderer,JewelRenderer,ServerSideRenderer, andJsRendererAdapterin Kotlin
When modifyingCodingAgentRenderer, update JVM CLI implementations:CodingCliRendererandConsoleRenderer
UseAutoDevColorsorMaterialTheme.colorSchemefor Compose styling
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt
**/*.{kt,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never use hardcoded colors - always use design tokens from the design system
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt
🧠 Learnings (4)
📚 Learning: 2025-12-07T08:40:10.859Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.859Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update ALL implementations: `DefaultCodingAgentRenderer`, `ComposeRenderer`, `JewelRenderer`, `ServerSideRenderer`, and `JsRendererAdapter` in Kotlin
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt
📚 Learning: 2025-12-07T08:40:10.859Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.859Z
Learning: Applies to **/*.kt : When modifying `CodingAgentRenderer`, update JVM CLI implementations: `CodingCliRenderer` and `ConsoleRenderer`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt
📚 Learning: 2025-12-07T08:40:10.859Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.859Z
Learning: Applies to **/*.kt : Use `AutoDevColors` or `MaterialTheme.colorScheme` for Compose styling
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.kt
📚 Learning: 2025-12-07T08:40:10.859Z
Learnt from: CR
Repo: phodal/auto-dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T08:40:10.859Z
Learning: Applies to mpp-vscode/src/**/*.ts : When modifying `CodingAgentRenderer`, update VSCode implementations: `mpp-vscode/src/bridge/mpp-core.ts` and `mpp-vscode/src/providers/chat-view.ts`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test mpp-core (KMP)
- GitHub Check: Build
🔇 Additional comments (2)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/components/header/IdeaAgentTabsHeader.kt (2)
263-263: LGTM: Color mapping follows design token guidelines.The ARTIFACT type is mapped to
IdeaAutoDevColors.Indigo.c400, correctly using the design system. Note that ARTIFACT is not included in the visible tabs list (Line 92), similar to LOCAL_CHAT, suggesting it's accessed through a different mechanism (likely file opening for .unit files).
277-277: Verify that ARTIFACT is intentionally excluded from the segmented tabs.The icon mapping is appropriate (Description icon for artifacts). However, ARTIFACT is not included in the visible tabs list at Line 92, while receiving color and icon mappings. Given the PR's focus on file open handling for .unit files, this appears intentional—ARTIFACT may be accessed via file opening rather than tab selection.
Please confirm this design decision aligns with the intended UX for artifact handling.
| // Main content area with resizable split pane for chat-based modes | ||
| when (currentAgentType) { | ||
| AgentType.CODING, AgentType.LOCAL_CHAT -> { | ||
| AgentType.CODING, AgentType.LOCAL_CHAT, AgentType.ARTIFACT -> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify artifact UI components exist and check how they're integrated
# Search for artifact-specific UI components
echo "=== Artifact UI Components ==="
rg -n --type=kt "class.*Artifact(Page|PreviewPanel|Content|Renderer)" -C 2
# Check if ArtifactAgent or ArtifactViewModel exists
echo -e "\n=== Artifact ViewModels/Agents ==="
rg -n --type=kt "class.*Artifact(Agent|ViewModel)" -C 2
# Check timeline rendering for artifact-specific handling
echo -e "\n=== Timeline Artifact Rendering ==="
rg -n --type=kt "AgentType\.ARTIFACT" -C 3Repository: phodal/auto-dev
Length of output: 260
🏁 Script executed:
#!/bin/bash
# Find kotlin files and search for artifact-related components
echo "=== Locating Kotlin files with artifact references ==="
find . -type f -name "*.kt" | head -20
echo -e "\n=== Finding IdeaAgentApp.kt ==="
find . -type f -name "IdeaAgentApp.kt"
echo -e "\n=== Searching for artifact components in .kt files ==="
rg "class.*Artifact(Page|PreviewPanel|Content|Renderer)" --type-list | grep -i kotlin
echo -e "\n=== Alternative search for Artifact classes ==="
rg "class Artifact" --glob="*.kt" -C 2
echo -e "\n=== Search for AgentType.ARTIFACT references ==="
rg "AgentType\.ARTIFACT" --glob="*.kt" -C 3Repository: phodal/auto-dev
Length of output: 21738
🏁 Script executed:
#!/bin/bash
# Read IdeaAgentApp.kt around line 211 to understand context
echo "=== IdeaAgentApp.kt around line 211 ==="
sed -n '200,230p' ./mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt
echo -e "\n=== Full when block structure ==="
sed -n '210,400p' ./mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt
echo -e "\n=== Check if other dedicated-component agents appear in CODING group ==="
grep -n "CODE_REVIEW\|KNOWLEDGE\|REMOTE\|WEB_EDIT" ./mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt | head -20Repository: phodal/auto-dev
Length of output: 13046
🏁 Script executed:
#!/bin/bash
# Search for IDEA-specific artifact components
echo "=== Search for IdeaArtifact* classes ==="
find ./mpp-idea -type f -name "*Artifact*.kt" | head -20
echo -e "\n=== Search for artifact references in mpp-idea ==="
rg -n "Artifact" ./mpp-idea/src --glob="*.kt" | head -30
echo -e "\n=== Search for ArtifactAgentViewModel usage ==="
rg -n "ArtifactAgentViewModel" --glob="*.kt"
echo -e "\n=== Check if ArtifactPage is cross-platform or IDEA-specific ==="
rg -n "fun ArtifactPage\|class ArtifactPage" --glob="*.kt" -C 1Repository: phodal/auto-dev
Length of output: 1223
🏁 Script executed:
#!/bin/bash
# Find ArtifactPage definition to understand its full scope
echo "=== ArtifactPage definition ==="
rg -n "fun ArtifactPage\|@Composable.*ArtifactPage" --glob="*.kt" -A 5
echo -e "\n=== Timeline content rendering in IDEA ==="
rg -n "IdeaTimelineContent" ./mpp-idea/src --glob="*.kt" -B 2 -A 10 | head -50
echo -e "\n=== Check how artifact content flows in timeline ==="
rg -n "ArtifactResult\|Artifact\(" ./mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose --glob="*.kt" | head -20Repository: phodal/auto-dev
Length of output: 6332
🏁 Script executed:
#!/bin/bash
# Find where ArtifactPage is defined
echo "=== Search for ArtifactPage file ==="
find . -type f -name "*ArtifactPage*" -o -type f -name "*.kt" | xargs grep -l "fun ArtifactPage\|@Composable.*ArtifactPage" 2>/dev/null
echo -e "\n=== Check ArtifactPage.kt imports and structure ==="
head -100 ./mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/artifact/ArtifactPage.kt
echo -e "\n=== Verify if ARTIFACT should be in dedicated case like CODE_REVIEW ==="
grep -A 5 "AgentType.CODE_REVIEW ->" ./mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt | head -15
echo -e "\n=== Check if artifact renderer handles timeline rendering ==="
rg -n "class.*ArtifactRenderer\|interface.*ArtifactRenderer" --glob="*.kt" -A 5 | head -30Repository: phodal/auto-dev
Length of output: 5111
Move ARTIFACT to a dedicated case with IDEA-specific content component.
ARTIFACT has dedicated infrastructure (ArtifactAgentViewModel, ArtifactPage in cross-platform code, ArtifactAgent) but is incorrectly grouped with CODING and LOCAL_CHAT in IdeaAgentApp, sharing the generic timeline UI. This violates the established pattern where dedicated agents (CODE_REVIEW, KNOWLEDGE, WEB_EDIT) have their own dedicated content components.
ARTIFACT should either:
- Get an IDEA-specific component (IdeaArtifactContent) following the same pattern as other dedicated agents, or
- Use a dedicated case that wraps ArtifactPage/similar cross-platform artifact UI for the IDEA plugin
The current routing is architecturally inconsistent and likely incomplete.
🤖 Prompt for AI Agents
In mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt
around line 211, AgentType.ARTIFACT is incorrectly grouped with CODING and
LOCAL_CHAT; create a dedicated case for ARTIFACT instead. Add a new when-branch
for AgentType.ARTIFACT that either instantiates an IdeaArtifactContent component
(implementing the IDEA-specific UI and wiring ArtifactAgentViewModel) or wraps
the cross-platform ArtifactPage (passing the ArtifactAgentViewModel/state into
the composable), update imports, and remove ARTIFACT from the CODING/LOCAL_CHAT
branch so the artifact agent uses its dedicated UI and follows the established
pattern for CODE_REVIEW/KNOWLEDGE/WEB_EDIT.
Pull request overview
This PR implements cross-platform file open handling for
.unitartifact bundle files, enabling macOS double-click support through the expect/actual pattern. It introduces a comprehensive artifact generation system inspired by Claude's Artifacts, including artifact bundling, WebView preview, and Load-Back support.Key Changes:
FileOpenHandlerusing expect/actual pattern for OS-level file association handlingArtifactAgent,ArtifactBundle,ArtifactBundlePackerfor reversible artifact generation