feat(vscode): implement plan functionality for mpp-vscode #40
Conversation
|
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. WalkthroughExposes PlanStateService from CodingAgent, ensures PlanStateService re-emits StateFlow after mutations, adds JS observePlanState export and JS-friendly plan summaries, wires plan updates into IDE and VSCode UIs, adds file-change tracking UI/dialogs, and small logging/build tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant IDE as IDE (Idea/VSCode)
participant Agent as CodingAgent
participant Service as PlanStateService
participant JS as JS Bridge / Renderer
participant UI as Extension/Webview/UI
IDE->>Agent: initializeCodingAgent()
Agent->>Service: getPlanStateService()
Agent-->>IDE: PlanStateService? (exposed)
IDE->>Service: observe currentPlan (callback)
Service->>Service: mutate plan (updateTaskStatus / completeStep / updateStepStatus)
Service->>Service: assign _currentPlan.value = updatedPlan (emit)
Service-->>IDE: callback(JsPlanSummaryData) / callback(null)
IDE->>JS: renderPlanSummary / setPlan
JS->>UI: post message (planUpdate / planCleared)
UI->>UI: update timeline / input / widgets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Introduce new classes for tracking, summarizing, and displaying file changes in the IDEA toolwindow, and update input area to integrate with these features.
Integrate PlanStateService observation to update renderer with current plan. Add logging for plan updates and summary rendering.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (13)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/plan/IdeaPlanSummaryBar.kt (2)
55-56: Consider using debug-level logging for Composable recomposition tracking.Since
IdeaPlanSummaryBaris a Composable function that may recompose frequently during UI updates, using.info()level could generate excessive log volume in production. Consider changing to.debug()level for operational visibility without cluttering production logs.Apply this diff:
- planSummaryLogger.info("IdeaPlanSummaryBar called: plan=${plan != null}, tasks=${plan?.tasks?.size ?: 0}") + planSummaryLogger.debug("IdeaPlanSummaryBar called: plan=${plan != null}, tasks=${plan?.tasks?.size ?: 0}")
60-60: Consider using debug-level logging for consistency.For the same reason as the previous log statement, consider changing this to
.debug()level to avoid log noise during frequent recompositions.Apply this diff:
- planSummaryLogger.info("IdeaPlanSummaryBar: not rendering (plan is null or empty)") + planSummaryLogger.debug("IdeaPlanSummaryBar: not rendering (plan is null or empty)")mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt (1)
151-152: Consider DEBUG level for verbose tool call logging.These log statements fire on every tool call, which could create significant noise in INFO logs. Tool-level trace logging typically belongs at DEBUG or TRACE level to keep INFO logs focused on significant events.
Apply this diff to move to DEBUG level:
- jewelRendererLogger.info("renderToolCall: toolName=$toolName, paramsStr length=${paramsStr.length}") + if (jewelRendererLogger.isDebugEnabled) { + jewelRendererLogger.debug("renderToolCall: toolName=$toolName, paramsStr length=${paramsStr.length}") + } val toolInfo = formatToolCallDisplay(toolName, paramsStr) val params = parseParamsString(paramsStr) val toolType = toolName.toToolType() - jewelRendererLogger.info("renderToolCall: parsed params keys=${params.keys}") + if (jewelRendererLogger.isDebugEnabled) { + jewelRendererLogger.debug("renderToolCall: parsed params keys=${params.keys}") + }Also applies to: 157-158
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgent.kt (1)
88-94: LGTM!The method correctly exposes the
PlanStateServicethrough delegation, maintaining proper encapsulation. Using a concrete class as return type aligns with Kotlin/JS export guidelines.Consider using an import for
cc.unitmesh.agent.plan.PlanStateServiceinstead of the fully qualified name to improve readability, though this is a minor style preference.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.kt (1)
108-110: Consider extracting the timestamp update pattern.The
plan.copy(updatedAt = kotlinx.datetime.Clock.System.now().toEpochMilliseconds())pattern is repeated in three methods (updateTaskStatus,completeStep,updateStepStatus). Consider extracting a helper function for consistency and maintainability:private fun emitPlanUpdate(plan: AgentPlan) { _currentPlan.value = plan.copy(updatedAt = kotlinx.datetime.Clock.System.now().toEpochMilliseconds()) }mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt (1)
265-268: Consider avoiding the non-null assertion operator.The
codingAgent!!assertion at line 267 is safe in this context since the agent was just created, but the pattern could be cleaner by capturing the local reference:- agentInitialized = true - - // Start observing PlanStateService and sync to renderer - startPlanStateObserver(codingAgent!!) + agentInitialized = true + val agent = codingAgent!! + + // Start observing PlanStateService and sync to renderer + startPlanStateObserver(agent) } - return codingAgent!! + return codingAgent ?: throw IllegalStateException("Agent initialization failed")Alternatively, restructure to assign
codingAgentand return in one flow to eliminate the assertion entirely.mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.kt (1)
186-213: Good implementation using callback pattern for JS interop.The callback-based approach correctly avoids exposing Kotlin
Flowdirectly to JavaScript, which aligns with the coding guidelines. The null-safety checks forplanStateServiceare properly handled.One minor simplification - the
jobvariable declaration on line 198 can be combined with the assignment:- var job: kotlinx.coroutines.Job? = null - job = GlobalScope.launch { + val job = GlobalScope.launch { planStateService.currentPlan.collect { plan ->mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/RendererExports.kt (1)
154-156: Metadata parameter is not forwarded to the JS renderer.The
renderToolResultadapter receives ametadata: Map<String, String>parameter but doesn't forward it toJsCodingAgentRenderer.renderToolResult(), which lacks metadata in its signature. This is a stable design decision since JS tools typically initialize metadata as empty maps anyway.If metadata becomes essential for the JS renderer in the future, extend
JsCodingAgentRenderer.renderToolResult()to accept it. Note: Some executors (e.g., CodeReviewAgentExecutor) still use the older 4-parameter signature without metadata.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.kt (1)
219-261: Use IntelliJ logger instead ofprintln.Line 257 uses
printlnfor error logging. In IntelliJ plugins, use the platform's logging mechanism for proper log management and visibility.+import com.intellij.openapi.diagnostic.Logger + +private val logger = Logger.getInstance("IdeaFileChangeSummary") + private fun undoChange(project: Project, change: FileChange) { ApplicationManager.getApplication().invokeLater { runWriteAction { try { // ... existing code ... } catch (e: Exception) { - println("Failed to undo change for ${change.filePath}: ${e.message}") + logger.error("Failed to undo change for ${change.filePath}", e) } } } }mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeTracker.kt (3)
39-39: Consider thread-safe collection fororiginalContents.VFS events can be dispatched from various threads. Using a plain
mutableMapOffororiginalContentsmay lead to race conditions when concurrent events access or modify the map.- private val originalContents = mutableMapOf<String, String?>() + private val originalContents = java.util.concurrent.ConcurrentHashMap<String, String?>()
184-185: ContentRevision.content can throw exceptions.
beforeRevision?.contentandafterRevision?.contentmay throwVcsException. Consider wrapping content retrieval in a try-catch to handle potential failures gracefully.- val originalContent = change.beforeRevision?.content - val newContent = change.afterRevision?.content + val originalContent = try { change.beforeRevision?.content } catch (e: Exception) { null } + val newContent = try { change.afterRevision?.content } catch (e: Exception) { null }
12-12: Unused import:ChangeListListener.
ChangeListListeneris imported but never used. Consider removing it unless planned for future implementation.-import com.intellij.openapi.vcs.changes.ChangeListListenermpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.kt (1)
38-54: TheIdeaFileChangeDiffDialogcomposable has a side-effect issue and appears unused.This
@Composablefunction directly callsshow()which displays a modal dialog as a side effect. This would trigger on every recomposition, which is not the intended behavior for composables.Looking at
IdeaFileChangeSummary.kt, it correctly usesIdeaFileChangeDiffDialogWrapper.show()directly viainvokeLater. Consider removing this unused function to avoid confusion.-@Composable -fun IdeaFileChangeDiffDialog( - project: Project, - change: FileChange, - onDismiss: () -> Unit, - onUndo: () -> Unit, - onKeep: () -> Unit -) { - // Show the dialog using DialogWrapper - IdeaFileChangeDiffDialogWrapper.show( - project = project, - change = change, - onUndo = onUndo, - onKeep = onKeep, - onDismiss = onDismiss - ) -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgent.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.kt(3 hunks)mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.kt(2 hunks)mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/RendererExports.kt(3 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt(4 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/sketch/IdeaTerminalRenderer.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.kt(2 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeTracker.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/plan/IdeaPlanSummaryBar.kt(1 hunks)mpp-vscode/package.json(1 hunks)mpp-vscode/src/providers/chat-view.ts(3 hunks)mpp-vscode/webview/src/App.tsx(4 hunks)mpp-vscode/webview/src/hooks/useVSCode.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualfor platform-specific code (e.g., file I/O on JVM/JS/Wasm) in Kotlin Multiplatform projects
Check export first if some functions are not working well with CLI (TypeScript)
In Kotlin/JS @JsExport: AvoidFlow, usePromiseinstead
In Kotlin/JS @JsExport: Use concrete classes as return types and parameter types; avoid interface types
For WASM platform, avoid using emoji and UTF-8 in code
Files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgent.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/plan/IdeaPlanSummaryBar.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/sketch/IdeaTerminalRenderer.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeTracker.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/agent/RendererExports.kt
**/idea/**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/idea/**/*.kt: For SwingPanel z-index issues with Compose popups, enable Jewel's custom popup renderer:JewelFlags.useCustomPopupRenderer = trueinIdeaAgentToolWindowFactory
For popup/dropdown menus in IntelliJ plugins, use Jewel'sPopupMenuinstead ofandroidx.compose.ui.window.Popup
For dialogs in IntelliJ plugins, use IntelliJ'sDialogWrapperwithorg.jetbrains.jewel.bridge.composeinstead ofandroidx.compose.ui.window.Dialog
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/plan/IdeaPlanSummaryBar.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/sketch/IdeaTerminalRenderer.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeTracker.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.kt
🧠 Learnings (8)
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/*.kt : In Kotlin/JS JsExport: Avoid `Flow`, use `Promise` instead
Applied to files:
mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/idea/**/*.kt : For dialogs in IntelliJ plugins, use IntelliJ's `DialogWrapper` with `org.jetbrains.jewel.bridge.compose` instead of `androidx.compose.ui.window.Dialog`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/idea/**/*.kt : For popup/dropdown menus in IntelliJ plugins, use Jewel's `PopupMenu` instead of `androidx.compose.ui.window.Popup`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/idea/**/*.kt : For SwingPanel z-index issues with Compose popups, enable Jewel's custom popup renderer: `JewelFlags.useCustomPopupRenderer = true` in `IdeaAgentToolWindowFactory`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/plan/IdeaPlanSummaryBar.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Note that `IdeaAgentViewModelTest` requires IntelliJ Platform Test Framework; `JewelRendererTest` can run standalone with JUnit 5
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: For `mpp-idea` plugin tests, run with: `cd mpp-idea && ../gradlew test --tests "cc.unitmesh.devins.idea.renderer.JewelRendererTest"`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/compose/**/*.{kt,kts} : In Compose (Desktop/Android), use `AutoDevColors` from `cc.unitmesh.devins.ui.compose.theme` or `MaterialTheme.colorScheme`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/*.kt : In Kotlin/JS JsExport: Use concrete classes as return types and parameter types; avoid interface types
Applied to files:
mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/RendererExports.kt
🧬 Code graph analysis (4)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.kt (1)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt (1)
IdeaFileChangeItem(28-171)
mpp-vscode/src/providers/chat-view.ts (1)
mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.kt (1)
step(11-32)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt (2)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/compose/agent/ComposeRenderer.kt (3)
formatToolCallDisplay(636-638)parseParamsString(644-646)updateTaskFromToolCall(199-232)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/render/RendererUtils.kt (2)
formatToolCallDisplay(15-50)parseParamsString(92-103)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeTracker.kt (1)
mpp-ui/src/jsMain/typescript/utils/outputFormatter.ts (1)
FileChange(10-15)
🔇 Additional comments (18)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/plan/IdeaPlanSummaryBar.kt (1)
46-47: LGTM: Logger declaration follows IntelliJ conventions.The logger is properly initialized using IntelliJ's Logger API with an appropriate name.
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt (3)
16-16: LGTM! Proper IntelliJ logger setup.The logger import and instantiation follow IntelliJ plugin best practices, using the standard
Logger.getInstance()pattern.Also applies to: 22-23
89-96: LGTM! Clean public API addition.The
setPlan()method is well-documented and properly syncs plan state with appropriate logging. The StateFlow update pattern is correct.
166-167: LGTM! Appropriate logging for plan tool detection.This targeted log statement for plan tool detection is appropriate at INFO level since plan operations are significant state changes.
mpp-vscode/package.json (1)
141-141: LGTM!Externalizing
wsis appropriate for Node.js native modules in VSCode extensions. This follows the same pattern as the existing--external:vscodeand resolves WebSocket bundling issues for MCP protocol support.mpp-vscode/webview/src/hooks/useVSCode.ts (1)
51-53: LGTM!The new
planUpdateandplanClearedmessage types follow the established naming convention and are properly categorized with a comment. These align with the plan state observation feature being implemented.mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.kt (1)
97-99: The shallow copy shares mutable collections; mutations persist across references.The
plan.copy(updatedAt = ...)creates a shallow copy ofAgentPlan, but since bothAgentPlan.tasks(MutableList) and each task'ssteps(MutableList) are mutable, the new copy shares the same list and step objects as the original. Whentask.updateStatus()orplan.completeStep()mutates these objects before the copy is made, the copied plan retains references to the same mutated objects. If code stores plan references fromcurrentPlanorgetPlan(), future mutations will be visible in those stored references.Refactor to either:
- Use immutable data structures (ImmutableList) instead of MutableList, or
- Create complete deep copies when mutations occur, before assigning to StateFlow
mpp-vscode/src/providers/chat-view.ts (2)
344-371: Plan state observer implementation looks good.The observer properly cancels any existing subscription before creating a new one, guards against missing
codingAgent, and handles both plan updates and cleared states appropriately. Error handling with try-catch and logging is also in place.
433-440: LGTM!The
renderPlanSummarycallback correctly usesconvertPlanSummaryto transform the plan data before posting to the webview, maintaining consistency with the observer path.mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/RendererExports.kt (1)
9-53: Well-structured JS-friendly data classes.The
JsStepSummaryandJsTaskSummaryclasses correctly follow the Kotlin/JS export guidelines by using concrete classes with@JsExport, and properly convert enum values to strings for JavaScript consumption. Based on learnings, this approach of using concrete classes as return/parameter types is the recommended pattern.mpp-vscode/webview/src/App.tsx (1)
411-416: No action needed. The Timeline component is correctly implemented without acurrentPlanprop. TheTimelinePropsinterface (Timeline.tsx lines 13-17) defines only three props:items,currentStreamingContent, andonAction. The Timeline component focuses on rendering timeline events and does not display plan information, which is appropriate for its design. The code at App.tsx lines 411-416 correctly passes all necessary props.Likely an incorrect or invalid review comment.
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.kt (1)
115-119: LGTM!The
IdeaFileChangeSummarycomponent is correctly integrated into the layout, positioned appropriately after the plan summary bar and before the top toolbar. Theprojectparameter is properly passed through.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt (1)
28-35: LGTM!The component is well-structured with proper use of Jewel theming and AutoDevColors. Icon and color mappings appropriately represent each change type semantically.
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.kt (2)
224-231: Verify: Undo for CREATE clears content but doesn't delete the file.The comment mentions "delete or clear the content" but only clearing is implemented. This leaves an empty file after undoing a create operation. Is this the intended behavior, or should the file be deleted entirely?
41-53: LGTM!The component correctly observes
FileChangeTracker.changesstate and only renders when there are changes. The dialog display pattern usingLaunchedEffectwithselectedChangeis appropriately implemented.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeTracker.kt (1)
32-33: LGTM!The service is correctly annotated as project-scoped and implements
Disposablefor proper cleanup. The VFS event subscription pattern is appropriate for real-time file change tracking.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.kt (2)
252-288: LGTM!The diff line rendering correctly handles unified diff format: file headers (
+++/---) are excluded from change styling, hunk headers (@@) get distinct styling, and regular additions/deletions are color-coded appropriately.
59-121: LGTM!The
DialogWrapperimplementation correctly follows IntelliJ plugin guidelines:
- Uses
org.jetbrains.jewel.bridge.composefor Compose content- Overrides
createSouthPanel()to return null for custom action buttons- Properly handles cancel action with callback
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt
Outdated
Show resolved
Hide resolved
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/sketch/IdeaTerminalRenderer.kt
Show resolved
Hide resolved
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt
Outdated
Show resolved
Hide resolved
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR implements plan functionality for the mpp-vscode extension and adds file change tracking UI components to the mpp-idea IntelliJ plugin. The implementation mirrors the plan display feature from mpp-idea, allowing VSCode users to see plan progress updates in real-time through the webview interface.
Key Changes:
- Adds
observePlanStatemethod toJsCodingAgentfor reactive plan state monitoring via StateFlow - Extends JS exports with detailed task and step information (
JsTaskSummary,JsStepSummary) - Implements plan state observer in VSCode's
ChatViewProviderwith real-time updates to the webview - Adds comprehensive file change tracking UI components to mpp-idea (not mentioned in PR description)
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
mpp-vscode/webview/src/hooks/useVSCode.ts |
Adds planUpdate and planCleared message types for plan state communication |
mpp-vscode/webview/src/App.tsx |
Adds plan state management and message handlers for plan updates |
mpp-vscode/src/providers/chat-view.ts |
Implements plan state observer with callback-based updates and data conversion |
mpp-vscode/package.json |
Adds --external:ws to esbuild config to resolve WebSocket bundling issues |
mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.kt |
Adds observePlanState method using StateFlow collection with GlobalScope |
mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/RendererExports.kt |
Extends JS exports with JsTaskSummary and JsStepSummary for complete plan data |
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.kt |
Adds StateFlow update triggers using plan.copy() to notify observers of mutations |
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgent.kt |
Adds getPlanStateService() accessor method for plan state observation |
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/plan/IdeaPlanSummaryBar.kt |
Adds debug logging for plan rendering |
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt |
Implements plan state observer using coroutine job with proper cancellation |
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt |
Adds setPlan() method and additional debug logging for plan updates |
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/sketch/IdeaTerminalRenderer.kt |
Comments out output display code without explanation |
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.kt |
Integrates file change summary component into input area |
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeTracker.kt |
New file: Implements VFS and VCS file change tracking service |
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.kt |
New file: Implements collapsible file change summary UI component |
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt |
New file: Implements individual file change display item with actions |
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.kt |
New file: Implements diff dialog for file change review |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| println("Failed to undo change for ${change.filePath}: ${e.message}") |
There was a problem hiding this comment.
Error message should be logged properly using a logger instead of println. The IntelliJ plugin codebase uses Logger.getInstance() for logging (as seen elsewhere in the codebase). Replace println() with proper logging:
private val logger = Logger.getInstance(IdeaFileChangeSummary::class.java)
// ... then use:
logger.error("Failed to undo change for ${change.filePath}", e)| } | ||
|
|
||
| var job: kotlinx.coroutines.Job? = null | ||
| job = GlobalScope.launch { |
There was a problem hiding this comment.
Using GlobalScope.launch is generally discouraged as it creates a coroutine that lives for the entire application lifetime and can lead to memory leaks if not properly managed. While the returned cancellation function helps, a better approach would be to use a structured concurrency scope tied to the agent's lifecycle. Consider using a scope that can be properly cancelled when the agent is disposed, or document why GlobalScope is necessary here.
| package cc.unitmesh.devins.idea.toolwindow.changes | ||
|
|
||
| import cc.unitmesh.agent.diff.ChangeType | ||
| import cc.unitmesh.agent.diff.FileChange | ||
| import cc.unitmesh.agent.diff.FileChangeTracker | ||
| import com.intellij.openapi.Disposable | ||
| import com.intellij.openapi.application.ApplicationManager | ||
| import com.intellij.openapi.components.Service | ||
| import com.intellij.openapi.fileEditor.FileDocumentManager | ||
| import com.intellij.openapi.project.Project | ||
| import com.intellij.openapi.vcs.changes.Change | ||
| import com.intellij.openapi.vcs.changes.ChangeListListener | ||
| import com.intellij.openapi.vcs.changes.ChangeListManager | ||
| import com.intellij.openapi.vfs.VirtualFile | ||
| import com.intellij.openapi.vfs.VirtualFileManager | ||
| import com.intellij.openapi.vfs.newvfs.BulkFileListener | ||
| import com.intellij.openapi.vfs.newvfs.events.VFileContentChangeEvent | ||
| import com.intellij.openapi.vfs.newvfs.events.VFileCreateEvent | ||
| import com.intellij.openapi.vfs.newvfs.events.VFileDeleteEvent | ||
| import com.intellij.openapi.vfs.newvfs.events.VFileEvent | ||
|
|
||
| /** | ||
| * Bridge between IntelliJ VCS/VFS and the cross-platform FileChangeTracker. | ||
| * | ||
| * This service monitors file changes in the project and syncs them to the | ||
| * FileChangeTracker for display in the IdeaFileChangeSummary component. | ||
| * | ||
| * It can track changes from: | ||
| * 1. VCS ChangeListManager (for VCS-tracked changes) | ||
| * 2. VFS BulkFileListener (for real-time file modifications) | ||
| */ | ||
| @Service(Service.Level.PROJECT) | ||
| class IdeaFileChangeTracker(private val project: Project) : Disposable { | ||
|
|
||
| private val changeListManager = ChangeListManager.getInstance(project) | ||
| private var isTracking = false | ||
|
|
||
| // Cache of original file contents before changes | ||
| private val originalContents = mutableMapOf<String, String?>() | ||
|
|
||
| init { | ||
| // Subscribe to VFS events for real-time file change tracking | ||
| project.messageBus.connect(this).subscribe( | ||
| VirtualFileManager.VFS_CHANGES, | ||
| object : BulkFileListener { | ||
| override fun before(events: List<VFileEvent>) { | ||
| if (!isTracking) return | ||
|
|
||
| events.forEach { event -> | ||
| when (event) { | ||
| is VFileContentChangeEvent -> { | ||
| // Cache original content before change | ||
| val file = event.file | ||
| if (isProjectFile(file)) { | ||
| val document = FileDocumentManager.getInstance().getDocument(file) | ||
| originalContents[file.path] = document?.text | ||
| } | ||
| } | ||
| is VFileDeleteEvent -> { | ||
| // Cache content before deletion | ||
| val file = event.file | ||
| if (isProjectFile(file)) { | ||
| val document = FileDocumentManager.getInstance().getDocument(file) | ||
| originalContents[file.path] = document?.text | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| override fun after(events: List<VFileEvent>) { | ||
| if (!isTracking) return | ||
|
|
||
| events.forEach { event -> | ||
| when (event) { | ||
| is VFileContentChangeEvent -> { | ||
| val file = event.file | ||
| if (isProjectFile(file)) { | ||
| val document = FileDocumentManager.getInstance().getDocument(file) | ||
| val newContent = document?.text | ||
| val originalContent = originalContents.remove(file.path) | ||
|
|
||
| if (originalContent != newContent) { | ||
| recordChange( | ||
| filePath = file.path, | ||
| changeType = ChangeType.EDIT, | ||
| originalContent = originalContent, | ||
| newContent = newContent | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| is VFileCreateEvent -> { | ||
| val file = event.file | ||
| if (file != null && isProjectFile(file)) { | ||
| val document = FileDocumentManager.getInstance().getDocument(file) | ||
| recordChange( | ||
| filePath = file.path, | ||
| changeType = ChangeType.CREATE, | ||
| originalContent = null, | ||
| newContent = document?.text | ||
| ) | ||
| } | ||
| } | ||
| is VFileDeleteEvent -> { | ||
| val file = event.file | ||
| if (isProjectFile(file)) { | ||
| val originalContent = originalContents.remove(file.path) | ||
| recordChange( | ||
| filePath = file.path, | ||
| changeType = ChangeType.DELETE, | ||
| originalContent = originalContent, | ||
| newContent = null | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Start tracking file changes | ||
| */ | ||
| fun startTracking() { | ||
| isTracking = true | ||
| } | ||
|
|
||
| /** | ||
| * Stop tracking file changes | ||
| */ | ||
| fun stopTracking() { | ||
| isTracking = false | ||
| originalContents.clear() | ||
| } | ||
|
|
||
| /** | ||
| * Record a file change to the FileChangeTracker | ||
| */ | ||
| fun recordChange( | ||
| filePath: String, | ||
| changeType: ChangeType, | ||
| originalContent: String?, | ||
| newContent: String? | ||
| ) { | ||
| val change = FileChange( | ||
| filePath = filePath, | ||
| changeType = changeType, | ||
| originalContent = originalContent, | ||
| newContent = newContent | ||
| ) | ||
| FileChangeTracker.recordChange(change) | ||
| } | ||
|
|
||
| /** | ||
| * Sync VCS changes to FileChangeTracker | ||
| */ | ||
| fun syncVcsChanges() { | ||
| val changes = changeListManager.defaultChangeList.changes | ||
| changes.forEach { change -> | ||
| val fileChange = convertVcsChange(change) | ||
| if (fileChange != null) { | ||
| FileChangeTracker.recordChange(fileChange) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Convert IntelliJ VCS Change to FileChange | ||
| */ | ||
| private fun convertVcsChange(change: Change): FileChange? { | ||
| val virtualFile = change.virtualFile ?: return null | ||
| val filePath = virtualFile.path | ||
|
|
||
| val changeType = when (change.type) { | ||
| Change.Type.NEW -> ChangeType.CREATE | ||
| Change.Type.DELETED -> ChangeType.DELETE | ||
| Change.Type.MOVED -> ChangeType.RENAME | ||
| else -> ChangeType.EDIT | ||
| } | ||
|
|
||
| val originalContent = change.beforeRevision?.content | ||
| val newContent = change.afterRevision?.content | ||
|
|
||
| return FileChange( | ||
| filePath = filePath, | ||
| changeType = changeType, | ||
| originalContent = originalContent, | ||
| newContent = newContent | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Check if a file is within the project | ||
| */ | ||
| private fun isProjectFile(file: VirtualFile): Boolean { | ||
| val projectBasePath = project.basePath ?: return false | ||
| return file.path.startsWith(projectBasePath) | ||
| } | ||
|
|
||
| override fun dispose() { | ||
| stopTracking() | ||
| } | ||
|
|
||
| companion object { | ||
| fun getInstance(project: Project): IdeaFileChangeTracker { | ||
| return project.getService(IdeaFileChangeTracker::class.java) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This file introduces file change tracking functionality for the IntelliJ plugin, but this feature is not mentioned in the PR title or description. The PR description states it's about "implement plan functionality for mpp-vscode", but includes significant new file change tracking UI components for mpp-idea (IdeaFileChangeTracker, IdeaFileChangeSummary, IdeaFileChangeItem, IdeaFileChangeDiffDialog). These should either be split into a separate PR or the PR description should be updated to reflect the full scope of changes.
| // if (showOutput && executionResult != null) { | ||
| // OutputDisplay(result = executionResult!!) | ||
| // } |
There was a problem hiding this comment.
The commented-out code for output display should either be removed or properly re-enabled with a clear explanation. Commented-out code reduces code clarity and maintainability. If this functionality is intentionally disabled, consider adding a TODO comment explaining why and when it should be restored.
| // if (showOutput && executionResult != null) { | |
| // OutputDisplay(result = executionResult!!) | |
| // } | |
| if (showOutput && executionResult != null) { | |
| OutputDisplay(result = executionResult!!) | |
| } |
| private isExecuting = false; | ||
| private messages: Array<{ role: string; content: string }> = []; | ||
| private editorChangeDisposable: vscode.Disposable | undefined; | ||
| private planStateUnsubscribe: (() => void) | null = null; |
There was a problem hiding this comment.
Missing disposal of planStateUnsubscribe callback. The ChatViewProvider class should implement a disposal mechanism to call planStateUnsubscribe() when the provider is disposed. Without this, there's a potential memory leak as the observer will continue to run even after the provider is no longer needed. Consider adding a dispose() method or cleaning up in the existing editorChangeDisposable pattern.
| // Trigger StateFlow update by creating a new plan instance with updated timestamp | ||
| _currentPlan.value = plan.copy(updatedAt = kotlinx.datetime.Clock.System.now().toEpochMilliseconds()) |
There was a problem hiding this comment.
Potential bug with StateFlow update strategy. Using plan.copy() to trigger StateFlow updates is problematic because AgentPlan.tasks is a MutableList<PlanTask>, meaning the copy will share the same mutable list reference as the original. This could lead to unexpected behavior and doesn't properly trigger StateFlow collectors.
A better approach would be to directly call _currentPlan.value = _currentPlan.value after the mutation (which triggers StateFlow), or change the approach to use immutable data structures. Alternatively, since AgentPlan.completeStep() already calls the internal touch() method to update the timestamp, simply reassigning the same object should be sufficient: _currentPlan.value = plan.
| // Trigger StateFlow update by creating a new plan instance with updated timestamp | |
| _currentPlan.value = plan.copy(updatedAt = kotlinx.datetime.Clock.System.now().toEpochMilliseconds()) | |
| // Trigger StateFlow update by reassigning the same plan object | |
| _currentPlan.value = plan |
| // Trigger StateFlow update by creating a new plan instance with updated timestamp | ||
| _currentPlan.value = plan.copy(updatedAt = kotlinx.datetime.Clock.System.now().toEpochMilliseconds()) |
There was a problem hiding this comment.
Potential bug with StateFlow update strategy. Using plan.copy() to trigger StateFlow updates is problematic because AgentPlan.tasks is a MutableList<PlanTask>, meaning the copy will share the same mutable list reference as the original. This could lead to unexpected behavior and doesn't properly trigger StateFlow collectors.
A better approach would be to directly reassign the same object after mutation: _currentPlan.value = plan. Since task.updateStepStatus() is called on the task (which mutates it), reassigning the plan object should trigger StateFlow collectors.
| // Trigger StateFlow update by creating a new plan instance with updated timestamp | ||
| _currentPlan.value = plan.copy(updatedAt = kotlinx.datetime.Clock.System.now().toEpochMilliseconds()) |
There was a problem hiding this comment.
Potential bug with StateFlow update strategy. Using plan.copy() to trigger StateFlow updates is problematic because AgentPlan.tasks is a MutableList<PlanTask>, meaning the copy will share the same mutable list reference as the original. This could lead to unexpected behavior and doesn't properly trigger StateFlow collectors.
A better approach would be to directly reassign the same object after mutation: _currentPlan.value = plan. Since task.updateStatus() is called on the task (which mutates it), reassigning the plan object should trigger StateFlow collectors.
Update the plan instance with a new timestamp to ensure StateFlow emits changes when task or step status is updated.
- Add observePlanState method to JsCodingAgent for observing PlanStateService state changes - Extend JsPlanSummaryData with JsTaskSummary and JsStepSummary for complete task/step info - Add plan state observer in ChatViewProvider to sync plan updates to webview - Add planUpdate/planCleared message handling in App.tsx - Pass currentPlan prop to ChatInput component for PlanSummaryBar display - Add ws as external dependency in esbuild config
phodal
force-pushed
the
feat/vscode-plan-functionality
branch
from
c7dbd34 to
2a5de66
Compare
|
augment review |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/RendererExports.kt (1)
154-156: Metadata parameter is silently dropped.The
renderToolResultadapter accepts ametadata: Map<String, String>parameter but does not forward it to the JS renderer. This could cause loss of important tool execution context.Consider extending
JsCodingAgentRenderer.renderToolResultto accept metadata:// In JsCodingAgentRenderer interface - fun renderToolResult(toolName: String, success: Boolean, output: String?, fullOutput: String?) + fun renderToolResult(toolName: String, success: Boolean, output: String?, fullOutput: String?, metadata: dynamic = null) // In JsRendererAdapter override fun renderToolResult(toolName: String, success: Boolean, output: String?, fullOutput: String?, metadata: Map<String, String>) { - jsRenderer.renderToolResult(toolName, success, output, fullOutput) + jsRenderer.renderToolResult(toolName, success, output, fullOutput, metadata.toJsObject()) }Alternatively, if metadata is not needed for JS consumers, document this intentional omission.
♻️ Duplicate comments (4)
mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.kt (1)
198-208: GlobalScope usage concern already flagged.The use of
GlobalScope.launchcreates a coroutine with application-lifetime scope, which can lead to memory leaks if not cancelled. The returned cancel function mitigates this, but consider tying the coroutine to the agent's lifecycle scope for better structured concurrency.mpp-vscode/webview/src/App.tsx (1)
260-268: Unsafe double cast concern already flagged.The
as unknown as PlanDatacast bypasses TypeScript's type checking and can hide runtime issues. Consider adding a type guard or minimal validation as suggested in the previous review.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.kt (1)
256-258: Replaceprintlnwith proper logging.This was already flagged in a previous review.
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt (1)
82-104: Edge case: separator shown even when parent path is missing.The middle dot separator (lines 82-89) is always rendered, but the parent path Text (lines 94-104) is conditionally rendered. For root-level files like
README.md, this results in a dangling separator.Additionally, the parent path logic for root-level files was flagged in a previous review.
Wrap both the separator and parent path in the same condition:
- // Path separator - Text( - text = "\u00B7", // Middle dot - style = JewelTheme.defaultTextStyle.copy( - fontSize = 11.sp, - color = AutoDevColors.Neutral.c500 - ) - ) - // Parent path - val parentPath = change.filePath.substringBeforeLast('/') - if (parentPath.isNotEmpty()) { + val parentPath = change.filePath.substringBeforeLast('/', "") + if (parentPath.isNotEmpty() && parentPath != change.filePath) { + // Path separator + Text( + text = "\u00B7", // Middle dot + style = JewelTheme.defaultTextStyle.copy( + fontSize = 11.sp, + color = AutoDevColors.Neutral.c500 + ) + ) + Text( text = parentPath.substringAfterLast('/'), ... ) }
🧹 Nitpick comments (5)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/plan/IdeaPlanSummaryBar.kt (1)
55-60: Consider using DEBUG level for recomposition-frequent logs.These log statements execute on every recomposition of the Composable, which can be very frequent during UI updates. Using INFO level will flood logs and may impact performance in production.
- planSummaryLogger.info("IdeaPlanSummaryBar called: plan=${plan != null}, tasks=${plan?.tasks?.size ?: 0}") + planSummaryLogger.debug("IdeaPlanSummaryBar called: plan=${plan != null}, tasks=${plan?.tasks?.size ?: 0}") ... - planSummaryLogger.info("IdeaPlanSummaryBar: not rendering (plan is null or empty)") + planSummaryLogger.debug("IdeaPlanSummaryBar: not rendering (plan is null or empty)")mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.kt (1)
97-98: StateFlow emission fix looks correct.Using
copy(updatedAt = ...)with a new timestamp ensures structural inequality, which correctly triggers StateFlow emissions. This addresses the core issue of notifying observers after in-place mutations.Consider extracting the repeated timestamp logic:
+private fun currentTimeMillis() = kotlinx.datetime.Clock.System.now().toEpochMilliseconds() + fun updateTaskStatus(taskId: String, status: TaskStatus) { val plan = _currentPlan.value ?: return val task = plan.getTask(taskId) ?: return task.updateStatus(status) - _currentPlan.value = plan.copy(updatedAt = kotlinx.datetime.Clock.System.now().toEpochMilliseconds()) + _currentPlan.value = plan.copy(updatedAt = currentTimeMillis()) notifyTaskUpdated(task) }mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.kt (1)
56-73: Consider usingDisposableEffector direct invocation instead ofLaunchedEffectfor dialog triggering.Using
LaunchedEffectto trigger a modal dialog is unconventional. TheselectedChangeis immediately set tonullafter showing the dialog, which could cause issues if the dialog takes time to initialize. Consider invoking the dialog directly from the click handler inIdeaFileChangeIteminstead of routing through state.- // Handle diff dialog display when a file is selected - LaunchedEffect(selectedChange) { - selectedChange?.let { change -> - ApplicationManager.getApplication().invokeLater { - IdeaFileChangeDiffDialogWrapper.show( - project = project, - change = change, - onUndo = { - undoChange(project, change) - FileChangeTracker.removeChange(change) - }, - onKeep = { - FileChangeTracker.removeChange(change) - }, - onDismiss = {} - ) - } - selectedChange = null - } - }Move the dialog invocation directly to the click handler:
onClick = { ApplicationManager.getApplication().invokeLater { IdeaFileChangeDiffDialogWrapper.show( project = project, change = change, onUndo = { undoChange(project, change); FileChangeTracker.removeChange(change) }, onKeep = { FileChangeTracker.removeChange(change) }, onDismiss = {} ) } }mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.kt (2)
38-54: Redundant composable wrapper function.
IdeaFileChangeDiffDialogcomposable just delegates toIdeaFileChangeDiffDialogWrapper.show(). Since dialogs in IntelliJ should useDialogWrapper(as per coding guidelines), callers should invoke the wrapper directly. This composable adds no value and may confuse developers about when to use Compose dialogs.Consider removing this function and having callers use
IdeaFileChangeDiffDialogWrapper.show()directly, which is already being done inIdeaFileChangeSummary.kt.
157-166: ConsiderLazyColumnfor large diffs.Using
ColumnwithforEachloads all diff lines at once. For large diffs (thousands of lines), this could cause performance issues. Consider usingLazyColumnfor virtualized rendering.- Column( - modifier = Modifier - .fillMaxSize() - .verticalScroll(scrollState) - ) { - diffContent.lines().forEach { line -> - DiffLine(line = line) - } - } + val lines = diffContent.lines() + LazyColumn( + modifier = Modifier.fillMaxSize() + ) { + items(lines.size) { index -> + DiffLine(line = lines[index]) + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgent.kt(1 hunks)mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.kt(3 hunks)mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.kt(2 hunks)mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/RendererExports.kt(3 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt(4 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/sketch/IdeaTerminalRenderer.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.kt(2 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeTracker.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/plan/IdeaPlanSummaryBar.kt(1 hunks)mpp-vscode/package.json(1 hunks)mpp-vscode/src/providers/chat-view.ts(3 hunks)mpp-vscode/webview/src/App.tsx(4 hunks)mpp-vscode/webview/src/hooks/useVSCode.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- mpp-vscode/package.json
- mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt
- mpp-vscode/webview/src/hooks/useVSCode.ts
- mpp-vscode/src/providers/chat-view.ts
- mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgent.kt
- mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt
- mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/sketch/IdeaTerminalRenderer.kt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualfor platform-specific code (e.g., file I/O on JVM/JS/Wasm) in Kotlin Multiplatform projects
Check export first if some functions are not working well with CLI (TypeScript)
In Kotlin/JS @JsExport: AvoidFlow, usePromiseinstead
In Kotlin/JS @JsExport: Use concrete classes as return types and parameter types; avoid interface types
For WASM platform, avoid using emoji and UTF-8 in code
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeTracker.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/plan/IdeaPlanSummaryBar.ktmpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/agent/RendererExports.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt
**/idea/**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/idea/**/*.kt: For SwingPanel z-index issues with Compose popups, enable Jewel's custom popup renderer:JewelFlags.useCustomPopupRenderer = trueinIdeaAgentToolWindowFactory
For popup/dropdown menus in IntelliJ plugins, use Jewel'sPopupMenuinstead ofandroidx.compose.ui.window.Popup
For dialogs in IntelliJ plugins, use IntelliJ'sDialogWrapperwithorg.jetbrains.jewel.bridge.composeinstead ofandroidx.compose.ui.window.Dialog
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeTracker.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/plan/IdeaPlanSummaryBar.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt
🧠 Learnings (6)
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/idea/**/*.kt : For SwingPanel z-index issues with Compose popups, enable Jewel's custom popup renderer: `JewelFlags.useCustomPopupRenderer = true` in `IdeaAgentToolWindowFactory`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/plan/IdeaPlanSummaryBar.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/idea/**/*.kt : For dialogs in IntelliJ plugins, use IntelliJ's `DialogWrapper` with `org.jetbrains.jewel.bridge.compose` instead of `androidx.compose.ui.window.Dialog`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/idea/**/*.kt : For popup/dropdown menus in IntelliJ plugins, use Jewel's `PopupMenu` instead of `androidx.compose.ui.window.Popup`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Note that `IdeaAgentViewModelTest` requires IntelliJ Platform Test Framework; `JewelRendererTest` can run standalone with JUnit 5
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/*.kt : In Kotlin/JS JsExport: Use concrete classes as return types and parameter types; avoid interface types
Applied to files:
mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/RendererExports.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/*.kt : In Kotlin/JS JsExport: Avoid `Flow`, use `Promise` instead
Applied to files:
mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/RendererExports.ktmpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.kt
🧬 Code graph analysis (3)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.kt (1)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.kt (1)
IdeaFileChangeSummary(41-138)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeTracker.kt (1)
mpp-ui/src/jsMain/typescript/utils/outputFormatter.ts (1)
FileChange(10-15)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.kt (1)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt (1)
IdeaFileChangeItem(28-171)
⏰ 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). (3)
- GitHub Check: Test mpp-core (KMP)
- GitHub Check: Build (241)
- GitHub Check: Build (223)
🔇 Additional comments (8)
mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.kt (1)
192-213: Implementation pattern is appropriate for JS interop.The callback-based API with a returned cancel function is a suitable pattern for JS consumers. The internal use of Flow collection is acceptable since it's not directly exported. Based on learnings, avoiding Flow in the public API is correct.
mpp-vscode/webview/src/App.tsx (1)
57-58: Plan state wiring looks correct.The
currentPlanstate initialization,planClearedhandler, and prop propagation toChatInputare properly implemented.Also applies to: 270-273, 455-455
mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/RendererExports.kt (1)
9-53: Well-structured JS-friendly data classes.
JsStepSummaryandJsTaskSummaryfollow the coding guidelines by using concrete classes with companion object converters. The@JsExportannotation and conversion pattern are appropriate for Kotlin/JS interop.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaDevInInputArea.kt (1)
115-119: LGTM!Clean integration of
IdeaFileChangeSummary. The placement follows the existing pattern used forIdeaPlanSummaryBar, and the component correctly receives the project context and modifier.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt (1)
28-45: LGTM!The composable structure with Row layout, click handling, and modifiers follows Compose best practices for IntelliJ plugins using Jewel.
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeTracker.kt (2)
41-122: VFS listener implementation looks correct.The before/after pattern for caching and recording changes is appropriate. The content caching in
before()and consumption inafter()handles the file modification lifecycle correctly.
160-168: This concern is not applicable.FileChangeTracker.recordChangealready handles duplicate entries by merging changes for the same file path (checking byfilePathand keeping the original content from the first change while using the latest new content). No duplicate files will appear in the UI, and no additional deduplication logic is needed insyncVcsChanges().Likely an incorrect or invalid review comment.
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeDiffDialog.kt (1)
59-121: Good use ofDialogWrapperfor IntelliJ dialogs.The implementation correctly follows the coding guidelines by using IntelliJ's
DialogWrapperwithorg.jetbrains.jewel.bridge.composefor the content panel. The custom action buttons in the Compose content instead of the default south panel is a valid design choice.
| onUndoAll = { | ||
| changes.forEach { change -> | ||
| undoChange(project, change) | ||
| } | ||
| FileChangeTracker.clearChanges() |
There was a problem hiding this comment.
Partial failure in "Undo All" leaves system in inconsistent state.
If undoChange fails for one file, subsequent files are not undone, but clearChanges() is still called, losing track of all changes including those that weren't undone.
Consider wrapping each undo in a try-catch and only removing successfully undone changes:
onUndoAll = {
- changes.forEach { change ->
- undoChange(project, change)
+ changes.forEach { change ->
+ try {
+ undoChange(project, change)
+ FileChangeTracker.removeChange(change)
+ } catch (e: Exception) {
+ // Log error, keep change in tracker for retry
+ }
}
- FileChangeTracker.clearChanges()
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.kt
around lines 90–94, the onUndoAll handler currently calls undoChange for each
change and then unconditionally calls FileChangeTracker.clearChanges(), which
loses tracking if any undo fails. Modify this to iterate changes, wrap each
undoChange(project, change) in a try-catch, record which changes succeeded,
log/report failures, and after the loop remove only the successfully undone
changes from the tracker (or call clearChanges() only if all succeeded); do not
clear the tracker unconditionally so remaining failed changes are preserved for
retry or inspection.
| ChangeType.CREATE -> { | ||
| // For created files, delete or clear the content | ||
| val virtualFile = LocalFileSystem.getInstance().findFileByPath(change.filePath) | ||
| virtualFile?.let { vf -> | ||
| val document = FileDocumentManager.getInstance().getDocument(vf) | ||
| document?.setText("") | ||
| } |
There was a problem hiding this comment.
Undo for CREATE should delete the file, not clear its content.
Clearing the content leaves an empty file in the filesystem, which is different from "undoing" a file creation. Users expect the file to be removed entirely.
ChangeType.CREATE -> {
- // For created files, delete or clear the content
+ // For created files, delete the file
val virtualFile = LocalFileSystem.getInstance().findFileByPath(change.filePath)
virtualFile?.let { vf ->
- val document = FileDocumentManager.getInstance().getDocument(vf)
- document?.setText("")
+ vf.delete(project)
}
}📝 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.
| ChangeType.CREATE -> { | |
| // For created files, delete or clear the content | |
| val virtualFile = LocalFileSystem.getInstance().findFileByPath(change.filePath) | |
| virtualFile?.let { vf -> | |
| val document = FileDocumentManager.getInstance().getDocument(vf) | |
| document?.setText("") | |
| } | |
| ChangeType.CREATE -> { | |
| // For created files, delete the file | |
| val virtualFile = LocalFileSystem.getInstance().findFileByPath(change.filePath) | |
| virtualFile?.let { vf -> | |
| WriteCommandAction.runWriteCommandAction(project) { | |
| vf.delete(this@IdeaFileChangeSummary) | |
| } | |
| } | |
| } |
| private var isTracking = false | ||
|
|
||
| // Cache of original file contents before changes | ||
| private val originalContents = mutableMapOf<String, String?>() |
There was a problem hiding this comment.
Thread safety: originalContents map accessed from multiple threads.
VFS events can be delivered on background threads, but mutableMapOf is not thread-safe. Concurrent access could cause ConcurrentModificationException or data corruption.
Use a thread-safe map:
- private val originalContents = mutableMapOf<String, String?>()
+ private val originalContents = java.util.concurrent.ConcurrentHashMap<String, String?>()📝 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 originalContents = mutableMapOf<String, String?>() | |
| private val originalContents = java.util.concurrent.ConcurrentHashMap<String, String?>() |
🤖 Prompt for AI Agents
In
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeTracker.kt
around line 39, the mutableMapOf originalContents is not thread-safe and can be
accessed from VFS/background threads; replace it with a thread-safe map (e.g.,
java.util.concurrent.ConcurrentHashMap or Collections.synchronizedMap) and
update usages to avoid compound read-modify-write races (use atomic operations
provided by ConcurrentHashMap or synchronize around multi-step accesses) so
concurrent VFS events cannot cause data races or
ConcurrentModificationExceptions.
- Add error handling for plan markdown parsing in JewelRenderer - Restore OutputDisplay in IdeaTerminalRenderer (was commented out) - Fix root-level files edge case in IdeaFileChangeItem - Cancel planStateObserverJob in IdeaAgentViewModel.dispose() - Add dispose() method to ChatViewProvider for cleanup - Add type guard validation for PlanData in App.tsx - Replace println with Logger in IdeaFileChangeSummary - Simplify StateFlow update in PlanStateService (use direct reassignment)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mpp-vscode/src/providers/chat-view.ts (1)
24-44: RegisterChatViewProviderinstance withcontext.subscriptionsfor proper cleanup.The
ChatViewProviderimplementsDisposableand has a correctdispose()method, but the current registration inextension.ts(lines 69-72) only pushes the return value ofregisterWebviewViewProvider()to subscriptions, not the provider instance itself. This meansChatViewProvider.dispose()will never be called on extension deactivation, leaving resources uncleaned.Fix by adding:
context.subscriptions.push(chatViewProvider);after the registration, so the provider's cleanup logic runs when the extension deactivates.
♻️ Duplicate comments (1)
mpp-vscode/webview/src/App.tsx (1)
276-289: LGTM! Plan message handlers are well-structured.The
planUpdatehandler correctly uses the type guard before updating state, with a safe fallback tonullfor invalid data. TheplanClearedhandler is appropriately simple.
🧹 Nitpick comments (2)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt (1)
82-104: Move separator inside parent path conditional.The middle dot separator is always rendered (lines 82–89), even when no parent path follows (e.g., for root-level files like
README.md). This results in the separator appearing between the file name and diff stats, which may not match the intended visual hierarchy.Consider moving the separator rendering into the conditional block so it only appears when a parent path is displayed:
- // Path separator - Text( - text = "\u00B7", // Middle dot - style = JewelTheme.defaultTextStyle.copy( - fontSize = 11.sp, - color = AutoDevColors.Neutral.c500 - ) - ) - // Parent path val parentPath = change.filePath.substringBeforeLast('/', "") if (parentPath.isNotEmpty() && parentPath != change.filePath) { + // Path separator + Text( + text = "\u00B7", // Middle dot + style = JewelTheme.defaultTextStyle.copy( + fontSize = 11.sp, + color = AutoDevColors.Neutral.c500 + ) + ) + Text( text = parentPath.substringAfterLast('/'), style = JewelTheme.defaultTextStyle.copy(mpp-vscode/src/providers/chat-view.ts (1)
458-483: Consider defensive checks for potentially undefined nested properties.The conversion correctly uses
Array.from()for Kotlin/JS collections. However, ifsummary.tasksortask.stepsareundefined(not justnull),Array.from(undefined)will throw. The current|| []fallback only handles falsy values.- tasks: Array.from(summary.tasks || []).map((task: any) => ({ + tasks: Array.from(summary.tasks ?? []).map((task: any) => ({ id: task.id, title: task.title, status: task.status, completedSteps: task.completedSteps, totalSteps: task.totalSteps, - steps: Array.from(task.steps || []).map((step: any) => ({ + steps: Array.from(task.steps ?? []).map((step: any) => ({This is functionally equivalent for most cases, but using
??is more explicit about intent (handlingnull/undefinedspecifically).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt(4 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/sketch/IdeaTerminalRenderer.kt(0 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt(2 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.kt(1 hunks)mpp-vscode/src/providers/chat-view.ts(4 hunks)mpp-vscode/webview/src/App.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/sketch/IdeaTerminalRenderer.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeSummary.kt
- mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/renderer/JewelRenderer.kt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/*.kt: Useexpect/actualfor platform-specific code (e.g., file I/O on JVM/JS/Wasm) in Kotlin Multiplatform projects
Check export first if some functions are not working well with CLI (TypeScript)
In Kotlin/JS @JsExport: AvoidFlow, usePromiseinstead
In Kotlin/JS @JsExport: Use concrete classes as return types and parameter types; avoid interface types
For WASM platform, avoid using emoji and UTF-8 in code
Files:
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt
**/idea/**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
**/idea/**/*.kt: For SwingPanel z-index issues with Compose popups, enable Jewel's custom popup renderer:JewelFlags.useCustomPopupRenderer = trueinIdeaAgentToolWindowFactory
For popup/dropdown menus in IntelliJ plugins, use Jewel'sPopupMenuinstead ofandroidx.compose.ui.window.Popup
For dialogs in IntelliJ plugins, use IntelliJ'sDialogWrapperwithorg.jetbrains.jewel.bridge.composeinstead ofandroidx.compose.ui.window.Dialog
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt
🧠 Learnings (4)
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Note that `IdeaAgentViewModelTest` requires IntelliJ Platform Test Framework; `JewelRendererTest` can run standalone with JUnit 5
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/idea/**/*.kt : For popup/dropdown menus in IntelliJ plugins, use Jewel's `PopupMenu` instead of `androidx.compose.ui.window.Popup`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/idea/**/*.kt : For dialogs in IntelliJ plugins, use IntelliJ's `DialogWrapper` with `org.jetbrains.jewel.bridge.compose` instead of `androidx.compose.ui.window.Dialog`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt
📚 Learning: 2025-12-02T00:20:34.480Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Applies to **/idea/**/*.kt : For SwingPanel z-index issues with Compose popups, enable Jewel's custom popup renderer: `JewelFlags.useCustomPopupRenderer = true` in `IdeaAgentToolWindowFactory`
Applied to files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/changes/IdeaFileChangeItem.kt
🧬 Code graph analysis (2)
mpp-vscode/src/providers/chat-view.ts (1)
mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.kt (1)
step(11-32)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt (1)
mpp-vscode/src/providers/chat-view.ts (1)
startPlanStateObserver(355-382)
🔇 Additional comments (12)
mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/plan/PlanStateService.kt (3)
93-100: LGTM! StateFlow update strategy correctly implemented.The direct reassignment of
_currentPlan.value = planafter mutation properly triggers StateFlow collectors, addressing the past review concern about usingplan.copy()with shared mutable lists.
105-111: LGTM! Consistent StateFlow update pattern.The implementation correctly triggers StateFlow updates by reassigning the plan object after mutation, consistent with the pattern in
updateTaskStatus.
116-123: LGTM! All mutation methods now follow the correct pattern.The StateFlow update strategy is consistently applied across all three mutation methods (
updateTaskStatus,completeStep, andupdateStepStatus), ensuring proper propagation of plan state changes to observers.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentViewModel.kt (3)
265-267: LGTM! Observer initialized at the correct time.The plan state observer is started after the CodingAgent is fully initialized, ensuring the PlanStateService is available before observation begins.
272-289: LGTM! Plan state observer implementation is correct.The observer implementation correctly:
- Cancels any existing observer before starting a new one (prevents leaks)
- Handles missing PlanStateService gracefully with early return
- Collects from StateFlow and forwards updates to the renderer
- Uses the ViewModel's coroutineScope for proper lifecycle management
The implementation mirrors the VSCode pattern shown in relevant code snippets, ensuring cross-platform consistency.
577-581: LGTM! Observer cleanup properly implemented.The plan state observer job is now correctly cancelled in
dispose(), addressing the past review concern about potential coroutine leaks. The cleanup order is appropriate, with individual jobs cancelled before the coroutine scope.mpp-vscode/webview/src/App.tsx (3)
19-33: Well-implemented type guard addressing the previous concern.The
isPlanDatatype guard properly validates the structure before setting state, replacing the unsafe double cast. The validation checks essential fields with appropriate type checks.One minor consideration: if
PlanDatainterface evolves (e.g., optional fields), ensure this guard stays synchronized.
428-432: Verify ifTimelineshould receivecurrentPlanprop.The AI summary indicates
currentPlanshould be passed to bothTimelineandChatInput, but onlyChatInputreceives it. IfTimelineis intended to display plan-related information, the prop should be passed here as well.
73-74: LGTM!The plan state initialization follows React conventions with appropriate typing.
mpp-vscode/src/providers/chat-view.ts (3)
351-382: Well-structured observer lifecycle management.The
startPlanStateObservermethod correctly:
- Cleans up any existing subscription before creating a new one
- Guards against missing
codingAgent- Handles both plan data and
nullcases appropriately- Includes error handling with logging
343-348: LGTM!The plan state observer is appropriately started after the
CodingAgentis initialized, ensuring the observer has a valid agent instance to subscribe to.
444-451: Consider consolidating or differentiatingrenderPlanSummaryandobservePlanStateto avoid duplicateplanUpdatemessages.Both mechanisms send
planUpdatemessages to the webview.renderPlanSummaryis called by the executor after successful plan tool execution (CodingAgentExecutor.kt:297), whileobservePlanStatesubscribes to general plan state changes. This can result in duplicate updates when both are triggered for the same plan change event.Either consolidate to a single mechanism or implement filtering to prevent redundant messages reaching the webview.
Summary
Implement plan functionality for mpp-vscode extension, mirroring the plan display feature from mpp-idea.
Changes
mpp-core (Kotlin)
observePlanStatemethod toJsCodingAgentfor observingPlanStateServicestate changes via callbackJsPlanSummaryDatawithJsTaskSummaryandJsStepSummaryclasses for complete task/step informationmpp-vscode Extension (TypeScript)
planStateUnsubscribefield for cleanupstartPlanStateObserver()method to observe plan state changesconvertPlanSummary()helper for data format conversionrenderPlanSummaryto renderer objectmpp-vscode Webview (React)
currentPlanstate andplanUpdate/planClearedmessage handlingplanUpdateandplanClearedtoExtensionMessagetypeBuild Configuration
--external:wsto esbuild config to resolve WebSocket dependency issueTesting
./gradlew :mpp-core:assembleJsPackagecd mpp-vscode && npm run buildSummary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.