feat(mpp-idea): implement remote agent support for IntelliJ IDEA plugin#19
feat(mpp-idea): implement remote agent support for IntelliJ IDEA plugin#19
Conversation
- Add IdeaRemoteAgentClient for HTTP/SSE communication with mpp-server - Add IdeaRemoteAgentViewModel for state management and event handling - Add IdeaRemoteAgentContent UI with server configuration panel - Update IdeaAgentApp to integrate REMOTE agent type - Add comprehensive tests for ViewModel and renderer interactions Referenced from mpp-ui's RemoteAgentChatInterface implementation.
|
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. WalkthroughAdds a REMOTE agent: new SSE-based remote client, a remote ViewModel managing server connection and streaming execution, Compose UI for remote configuration and timeline, IdeaAgentApp wiring for lazy lifecycle and dedicated remote input state, and unit tests for viewmodel behavior and project ID derivation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as IdeaAgentApp (UI)
participant VM as IdeaRemoteAgentViewModel
participant Client as IdeaRemoteAgentClient
participant Server as Remote Server
participant Renderer as JewelRenderer
User->>UI: Switch to REMOTE tab
UI->>VM: create/mount remoteAgentViewModel
User->>UI: Enter server URL & Connect
UI->>VM: checkConnection()
VM->>Client: healthCheck()
Client->>Server: GET /health
Server-->>Client: HealthResponse
Client-->>VM: health result
VM->>Client: getProjects()
Client->>Server: GET /api/projects
Server-->>Client: ProjectListResponse
Client-->>VM: projects
VM-->>UI: update isConnected / availableProjects
User->>UI: Submit task (projectId/gitUrl + task)
UI->>VM: executeTask(...)
VM->>Client: executeStream(RemoteAgentRequest) [SSE]
Client->>Server: POST /api/agent/stream (SSE)
loop streaming events
Server-->>Client: RemoteAgentEvent (LLMChunk / ToolCall / ToolResult / etc.)
Client->>VM: emit RemoteAgentEvent
VM->>Renderer: handleRemoteEvent() -> render timeline item
Renderer-->>UI: timeline updated
end
Server-->>Client: RemoteAgentEvent.Complete
Client->>VM: stream closed
VM-->>UI: isExecuting = false
User->>UI: Leave REMOTE tab
UI->>VM: dispose()
VM->>Client: close()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus on:
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 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| */ | ||
| fun getEffectiveProjectId(projectId: String, gitUrl: String): String { | ||
| return if (gitUrl.isNotBlank()) { | ||
| gitUrl.split('/').last().removeSuffix(".git") |
There was a problem hiding this comment.
gitUrl.split('/').last() will return an empty string for URLs with a trailing slash (e.g., .../repo/), producing a blank project id; consider handling trailing slashes or selecting the last non-blank segment to avoid rejecting valid inputs.
🤖 Was this useful? React with 👍 or 👎
| ): RemoteAgentRequest { | ||
| return if (gitUrl.isNotBlank()) { | ||
| RemoteAgentRequest( | ||
| projectId = gitUrl.split('/').lastOrNull()?.removeSuffix(".git") ?: "temp-project", |
There was a problem hiding this comment.
In buildRequest(), deriving projectId via split('/').lastOrNull()?.removeSuffix(".git") can yield an empty string when the URL ends with /; consider using the last non-blank segment or trimming trailing slashes before splitting (also applies to the similar branch below).
🤖 Was this useful? React with 👍 or 👎
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt (1)
117-133: Restructure DisposableEffect to dispose only the ViewModel for the tab being left, not all ViewModels except the current tab.The current logic disposes ViewModels based on what
currentAgentTypeis NOT, which is semantically inverted. When switching from CODE_REVIEW to CODING,onDisposeruns withcurrentAgentType = CODING, and the conditions dispose all ViewModels that don't match the current tab.While this works functionally, it's counterintuitive and harder to maintain. The pattern contradicts other DisposableEffect usages in the codebase (CodeReviewPage.kt, CodeReviewDemo.kt) which track and dispose the specific resource that changed.
Consider tracking the previous
agentTypeor using separate DisposableEffects per ViewModel to make the cleanup intent explicit: only dispose the ViewModel for the tab being left.
🧹 Nitpick comments (10)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentContent.kt (1)
44-62: Potential state sync loop between parent and child.The parent
IdeaRemoteAgentContentusesLaunchedEffectto propagateprojectIdandgitUrlchanges to callbacks (lines 57-62), whileRemoteConfigPanelalso syncs these values bidirectionally (lines 128-139 and 147-156). When the user types inRemoteConfigPanel, the flow is:
projectIdStateupdates →snapshotFlowfires →onProjectIdChangecalled (line 131)- Parent receives callback → updates local
projectIdstate (line 72)- Parent's
LaunchedEffect(projectId)fires → callsonProjectIdChangeagain (line 58)This creates redundant callback invocations. Consider removing the parent-level
LaunchedEffectblocks since the child already handles propagation.- // Propagate changes to parent - LaunchedEffect(projectId) { - onProjectIdChange(projectId) - } - LaunchedEffect(gitUrl) { - onGitUrlChange(gitUrl) - }mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt (1)
108-114: Consider making the default server URL configurable.The hardcoded
"http://localhost:8080"is fine for local development, but users may need to configure different server endpoints. The UI already allows URL input, so the initial default could be loaded from persisted settings.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.kt (2)
94-97: Replace printStackTrace with proper logging.Using
e.printStackTrace()outputs to stderr without context. Consider using IntelliJ's logging infrastructure or removing debug output in favor of the thrown exception message.} catch (e: Exception) { - e.printStackTrace() + // Consider: com.intellij.openapi.diagnostic.Logger.getInstance(IdeaRemoteAgentClient::class.java).warn("Stream connection failed", e) throw RemoteAgentException("Stream connection failed: ${e.message}", e) }
108-125: Sensitive fields in serializable data classes may be logged.
RemoteAgentRequestcontainspasswordandLLMConfigcontainsapiKey. Since these are@Serializable, they could appear in logs if the objects are printed. Consider implementing customtoString()to mask sensitive fields.@Serializable data class RemoteAgentRequest( val projectId: String, val task: String, val llmConfig: LLMConfig? = null, val gitUrl: String? = null, val branch: String? = null, val username: String? = null, val password: String? = null -) +) { + override fun toString(): String = "RemoteAgentRequest(projectId=$projectId, task=$task, gitUrl=$gitUrl, branch=$branch, username=$username, password=***)" +} @Serializable data class LLMConfig( val provider: String, val modelName: String, val apiKey: String, val baseUrl: String? = null -) +) { + override fun toString(): String = "LLMConfig(provider=$provider, modelName=$modelName, apiKey=***, baseUrl=$baseUrl)" +}mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt (3)
29-42: Test scope created but not used; tests use runBlocking instead.The
testScopeis initialized in@BeforeEachbut never used. All tests userunBlockingdirectly. Either use the test scope for proper structured concurrency testing, or remove the unused setup.- private lateinit var testScope: CoroutineScope - - @BeforeEach - fun setUp() { - testScope = CoroutineScope(SupervisorJob() + Dispatchers.Default) - } - - @AfterEach - fun tearDown() { - testScope.cancel() - } + // Remove if not using test scope, or consider using kotlinx-coroutines-test's runTest
43-59: Test class name suggests ViewModel testing but primarily tests JewelRenderer.The comment at lines 45-47 correctly notes that full ViewModel testing requires IntelliJ Platform. Consider renaming the test class to
JewelRendererRemoteAgentTestor adding separate integration tests when Platform is available.Based on learnings, these standalone JUnit 5 tests can be run with
cd mpp-idea && ../gradlew test --tests "cc.unitmesh.devins.idea.toolwindow.remote.IdeaRemoteAgentViewModelTest".
253-271: Test duplicates getEffectiveProjectId logic.Line 259 manually implements the same logic as
getEffectiveProjectIdinIdeaRemoteAgentContent.kt. Consider importing and using the helper to ensure tests align with production behavior.+import cc.unitmesh.devins.idea.toolwindow.remote.getEffectiveProjectId + @Test fun testRemoteAgentRequestWithGitUrl() { val gitUrl = "https://github.com/user/repo.git" val task = "Fix the bug" - val projectId = gitUrl.split('/').lastOrNull()?.removeSuffix(".git") ?: "temp-project" + val projectId = getEffectiveProjectId("", gitUrl) val request = RemoteAgentRequest( projectId = projectId,mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModel.kt (3)
148-181: Duplicated Git URL detection logic across codebase.The
buildRequestfunction reimplements Git URL detection (lines 162-164) that's conceptually similar togetEffectiveProjectId. Consider consolidating this logic into a shared utility to ensure consistent behavior and reduce maintenance burden.
250-256: Simplify cancelTask by removing redundant null check.The
cancel()call on aJob?can be simplified. Kotlin's safe call operator handles null gracefully, and cancelling an already-completed job is a no-op.fun cancelTask() { - if (_isExecuting.value && currentExecutionJob != null) { - currentExecutionJob?.cancel("Task cancelled by user") - currentExecutionJob = null - _isExecuting.value = false - } + currentExecutionJob?.cancel("Task cancelled by user") + currentExecutionJob = null + _isExecuting.value = false }
87-91: Replace println with IntelliJ logger.Using
printlnfor logging is not appropriate in an IntelliJ plugin. Use IntelliJ's logging infrastructure for proper log level control and IDE integration.+import com.intellij.openapi.diagnostic.Logger + +private val LOG = Logger.getInstance(IdeaRemoteAgentViewModel::class.java) + fun executeTask(projectId: String, task: String, gitUrl: String = "") { if (_isExecuting.value) { - println("Agent is already executing") + LOG.info("Agent is already executing") return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt(7 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentContent.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModel.kt(1 hunks)mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
Use
expect/actualfor platform-specific code in KMP projects (e.g., file I/O on JVM/JS/Wasm)
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentContent.ktmpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModel.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt
🧠 Learnings (1)
📚 Learning: 2025-11-30T02:30:49.805Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-30T02:30:49.805Z
Learning: Use `cd mpp-idea && ../gradlew test --tests "cc.unitmesh.devins.idea.renderer.JewelRendererTest"` for running standalone JUnit 5 tests that do not require IntelliJ Platform
Applied to files:
mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt
🧬 Code graph analysis (3)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentContent.kt (1)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/timeline/IdeaTimelineContent.kt (1)
IdeaTimelineContent(20-48)
mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (5)
assertTrue(29-33)assertFalse(38-40)assertNull(64-68)assertEquals(45-49)assertNotNull(54-59)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt (2)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/timeline/IdeaTimelineContent.kt (2)
IdeaTimelineContent(20-48)IdeaEmptyStateMessage(80-94)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentContent.kt (2)
IdeaRemoteAgentContent(30-95)getEffectiveProjectId(311-317)
⏰ 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: Build (241)
- GitHub Check: Build (223)
🔇 Additional comments (7)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentContent.kt (2)
101-156: TextFieldState synchronization pattern is well-implemented.The bidirectional sync between
TextFieldStateand external String parameters usingsnapshotFlowwithdistinctUntilChangedis the correct pattern for Jewel TextField integration. The external-to-internal sync (lines 142-156) properly checks for differences before updating to avoid loops.
266-306: Connection status indicator is well-designed.The animated color transition and clear status messaging provide good user feedback. The conditional logic for displaying connection state, error messages, or "Not connected" is appropriately prioritized.
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/IdeaAgentApp.kt (1)
215-246: Remote input area implementation is correct.The validation logic properly checks for a valid project identifier before executing tasks, and uses the
getEffectiveProjectIdhelper to derive the project ID from either the direct input or Git URL. Error feedback via the renderer provides appropriate user guidance.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.kt (1)
26-44: HTTP client configuration is well-tuned for IDE plugin usage.The SSE reconnection settings (30s interval, 3 attempts) and connection pool configuration are appropriate for a background agent client. The
expectSuccess = falseallows custom error handling but requires explicit status checks in API methods (noted separately).mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt (1)
273-307: Data class sanity tests are appropriate.These tests validate proper instantiation and field access for
LLMConfig,HealthResponse, andProjectInfo. While simple, they help catch potential serialization or constructor issues early.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModel.kt (2)
186-245: Event handling implementation is comprehensive and well-structured.The
handleRemoteEventfunction properly maps allRemoteAgentEventtypes to appropriate renderer calls. State transitions forisProcessingare correctly managed (ending LLM response before tool calls, etc.).
273-276: Resource cleanup is properly implemented.The
dispose()method correctly cancels any pending job and closes the HTTP client, preventing resource leaks when the ViewModel is disposed.
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.kt
Show resolved
Hide resolved
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentContent.kt
Show resolved
Hide resolved
| } catch (e: CancellationException) { | ||
| renderer.forceStop() | ||
| renderer.renderError("Task cancelled by user") | ||
| _isExecuting.value = false | ||
| currentExecutionJob = null | ||
| } catch (e: Exception) { | ||
| renderer.renderError(e.message ?: "Unknown error") | ||
| _isExecuting.value = false | ||
| currentExecutionJob = null | ||
| } |
There was a problem hiding this comment.
Catching CancellationException prevents proper coroutine cancellation propagation.
CancellationException is special in Kotlin coroutines and should generally be rethrown to allow proper cancellation propagation. The current handling logs an error message but may interfere with structured concurrency.
- } catch (e: CancellationException) {
- renderer.forceStop()
- renderer.renderError("Task cancelled by user")
- _isExecuting.value = false
- currentExecutionJob = null
+ } catch (e: CancellationException) {
+ renderer.forceStop()
+ // Don't render cancellation as an error - it's user-initiated
+ _isExecuting.value = false
+ currentExecutionJob = null
+ throw e // Re-throw to maintain structured concurrency
} catch (e: Exception) {Alternatively, if you want to handle cancellation gracefully without re-throwing, use a finally block for cleanup:
try {
// ... streaming logic
} finally {
_isExecuting.value = false
currentExecutionJob = null
}🤖 Prompt for AI Agents
In
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModel.kt
around lines 135-144, the code explicitly catches CancellationException which
prevents proper coroutine cancellation propagation; remove the dedicated catch
for CancellationException (or if you must catch it, immediately rethrow it) and
move cleanup into a finally block so that _isExecuting.value = false and
currentExecutionJob = null always run, and keep renderer.forceStop() and
renderer.renderError(...) only for non-cancellation exceptions (i.e., catch
Exception for error reporting, rethrow CancellationException or avoid catching
it).
There was a problem hiding this comment.
Pull request overview
This PR implements remote agent support for the IntelliJ IDEA plugin (mpp-idea) by creating a new HTTP-based client that connects to mpp-server. The implementation closely mirrors the existing RemoteAgentChatInterface from mpp-ui, adapting it for IntelliJ's plugin architecture with Jewel UI components and StateFlow-based state management instead of Compose's mutableStateOf.
Key Changes:
- New HTTP/SSE client for streaming agent execution from remote server
- ViewModel with StateFlow-based state management following IntelliJ plugin patterns
- Jewel-based Compose UI with server configuration panel and connection status display
- Integration with existing agent infrastructure through the REMOTE tab in IdeaAgentApp
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
IdeaRemoteAgentClient.kt |
HTTP client using Ktor/SSE for streaming events from mpp-server; includes data classes for request/response serialization |
IdeaRemoteAgentViewModel.kt |
ViewModel managing connection state, task execution, and event routing to JewelRenderer; follows IntelliJ Disposable pattern |
IdeaRemoteAgentContent.kt |
Compose UI with server configuration inputs, connection status indicator, and timeline display; uses Jewel TextField components |
IdeaRemoteAgentViewModelTest.kt |
Unit tests for renderer behavior and data classes; tests run without IntelliJ Platform dependencies |
IdeaAgentApp.kt |
Integration of remote agent into main UI; adds lazy ViewModel creation, disposal logic, and dedicated input area for remote mode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fun getEffectiveProjectId(projectId: String, gitUrl: String): String { | ||
| return if (gitUrl.isNotBlank()) { | ||
| gitUrl.split('/').last().removeSuffix(".git") | ||
| } else { | ||
| projectId | ||
| } | ||
| } |
There was a problem hiding this comment.
The function getEffectiveProjectId duplicates the logic from IdeaRemoteAgentViewModel.buildRequest() for extracting the project name from a Git URL. The implementation here (gitUrl.split('/').last().removeSuffix(".git")) is slightly different from the ViewModel's version which uses .lastOrNull() and provides a fallback. This inconsistency could lead to bugs. Consider either: (1) moving this function to a shared utility class, or (2) removing it entirely and using the ViewModel's logic directly, or (3) ensuring both implementations handle edge cases identically (e.g., what if gitUrl is empty or doesn't contain a '/'?).
| remoteAgentViewModel = IdeaRemoteAgentViewModel( | ||
| project = project, | ||
| coroutineScope = coroutineScope, | ||
| serverUrl = "http://localhost:8080" |
There was a problem hiding this comment.
The server URL is hardcoded to "http://localhost:8080". This should be configurable, either through user preferences, environment variables, or a configuration file. Consider loading this from ConfigManager or providing a UI setting to allow users to change the server URL without modifying code.
| onSend = { task -> | ||
| val effectiveProjectId = getEffectiveProjectId(remoteProjectId, remoteGitUrl) | ||
| if (effectiveProjectId.isNotBlank()) { | ||
| remoteVm.executeTask(effectiveProjectId, task, remoteGitUrl) | ||
| } else { | ||
| remoteVm.renderer.renderError("Please provide a project or Git URL") | ||
| } |
There was a problem hiding this comment.
The validation logic here only checks if effectiveProjectId is not blank, but doesn't validate whether it's a valid project ID or Git URL format. Additionally, the getEffectiveProjectId function can throw an exception (as noted in another comment). Consider adding more robust validation before calling executeTask, such as checking if the project exists in the available projects list, or validating the Git URL format with a regex pattern.
| // Sync server URL state to callback | ||
| LaunchedEffect(Unit) { | ||
| snapshotFlow { serverUrlState.text.toString() } | ||
| .distinctUntilChanged() | ||
| .collect { onServerUrlChange(it) } | ||
| } | ||
|
|
||
| // Sync project ID state to callback | ||
| LaunchedEffect(Unit) { | ||
| snapshotFlow { projectIdState.text.toString() } | ||
| .distinctUntilChanged() | ||
| .collect { onProjectIdChange(it) } | ||
| } | ||
|
|
||
| // Sync git URL state to callback | ||
| LaunchedEffect(Unit) { | ||
| snapshotFlow { gitUrlState.text.toString() } | ||
| .distinctUntilChanged() | ||
| .collect { onGitUrlChange(it) } | ||
| } | ||
|
|
||
| // Sync external changes to text field states | ||
| LaunchedEffect(serverUrl) { | ||
| if (serverUrlState.text.toString() != serverUrl) { | ||
| serverUrlState.setTextAndPlaceCursorAtEnd(serverUrl) | ||
| } | ||
| } | ||
| LaunchedEffect(projectId) { | ||
| if (projectIdState.text.toString() != projectId) { | ||
| projectIdState.setTextAndPlaceCursorAtEnd(projectId) | ||
| } | ||
| } | ||
| LaunchedEffect(gitUrl) { | ||
| if (gitUrlState.text.toString() != gitUrl) { | ||
| gitUrlState.setTextAndPlaceCursorAtEnd(gitUrl) | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential infinite loop or update cycle: Multiple LaunchedEffect blocks are creating bidirectional synchronization between the TextFieldState and the parent state. When a user types in the TextField, it triggers the snapshotFlow collectors (lines 121-138) which update the parent state via callbacks. The parent state changes then trigger the LaunchedEffect blocks (lines 142-156) which try to update the TextFieldState back. This could cause update cycles. While the if checks help prevent some updates, this pattern is fragile. Consider using a unidirectional data flow pattern or using rememberUpdatedState to avoid stale closures.
| fun testLLMConfigSerialization() { | ||
| val config = LLMConfig( | ||
| provider = "OpenAI", | ||
| modelName = "gpt-4", | ||
| apiKey = "test-key", | ||
| baseUrl = "https://api.openai.com" | ||
| ) |
There was a problem hiding this comment.
Security concern: The API key is being logged or stored in plaintext in test code (line 278). While this is a test file with a dummy key, it sets a poor precedent. In production code, API keys from LLMConfig are transmitted to the remote server (see IdeaRemoteAgentViewModel.kt line 114-119). Ensure that: (1) API keys are transmitted over HTTPS only, (2) They're not logged in production code, and (3) Consider adding a comment in tests that no real API keys should be used.
| if (event is RemoteAgentEvent.Complete) { | ||
| _isExecuting.value = false | ||
| currentExecutionJob = null | ||
| } | ||
| } | ||
|
|
||
| } catch (e: CancellationException) { | ||
| renderer.forceStop() | ||
| renderer.renderError("Task cancelled by user") | ||
| _isExecuting.value = false | ||
| currentExecutionJob = null | ||
| } catch (e: Exception) { | ||
| renderer.renderError(e.message ?: "Unknown error") |
There was a problem hiding this comment.
Potential race condition: The _isExecuting flag is set to false inside the collect block when a Complete event is received (line 130), but it's also set in the catch blocks (lines 138, 142). If an exception occurs after the Complete event but before the coroutine finishes, _isExecuting will be set to false twice. More importantly, if the job is cancelled between checking _isExecuting and setting it, this could lead to inconsistent state. Consider using a mutex or atomic operations, or restructure the code to ensure _isExecuting is always managed in a single place (e.g., using try-finally).
| if (event is RemoteAgentEvent.Complete) { | |
| _isExecuting.value = false | |
| currentExecutionJob = null | |
| } | |
| } | |
| } catch (e: CancellationException) { | |
| renderer.forceStop() | |
| renderer.renderError("Task cancelled by user") | |
| _isExecuting.value = false | |
| currentExecutionJob = null | |
| } catch (e: Exception) { | |
| renderer.renderError(e.message ?: "Unknown error") | |
| // No need to set _isExecuting or currentExecutionJob here; handled in finally | |
| // if (event is RemoteAgentEvent.Complete) { | |
| // _isExecuting.value = false | |
| // currentExecutionJob = null | |
| // } | |
| } catch (e: CancellationException) { | |
| renderer.forceStop() | |
| renderer.renderError("Task cancelled by user") | |
| // _isExecuting.value = false | |
| // currentExecutionJob = null | |
| } catch (e: Exception) { | |
| renderer.renderError(e.message ?: "Unknown error") | |
| // _isExecuting.value = false | |
| // currentExecutionJob = null | |
| } finally { |
| private fun buildRequest( | ||
| projectId: String, | ||
| task: String, | ||
| gitUrl: String, | ||
| llmConfig: LLMConfig? | ||
| ): RemoteAgentRequest { | ||
| return if (gitUrl.isNotBlank()) { | ||
| RemoteAgentRequest( | ||
| projectId = gitUrl.split('/').lastOrNull()?.removeSuffix(".git") ?: "temp-project", | ||
| task = task, | ||
| llmConfig = llmConfig, | ||
| gitUrl = gitUrl | ||
| ) | ||
| } else { | ||
| val isGitUrl = projectId.startsWith("http://") || | ||
| projectId.startsWith("https://") || | ||
| projectId.startsWith("git@") | ||
|
|
||
| if (isGitUrl) { | ||
| RemoteAgentRequest( | ||
| projectId = projectId.split('/').lastOrNull()?.removeSuffix(".git") ?: "temp-project", | ||
| task = task, | ||
| llmConfig = llmConfig, | ||
| gitUrl = projectId | ||
| ) |
There was a problem hiding this comment.
Duplicated logic: The code for extracting project ID from a git URL (gitUrl.split('/').lastOrNull()?.removeSuffix(".git") ?: "temp-project") is duplicated in lines 156 and 168. This logic should be extracted into a private helper function to improve maintainability and reduce the risk of inconsistencies if this logic needs to be changed in the future.
| */ | ||
| fun getEffectiveProjectId(projectId: String, gitUrl: String): String { | ||
| return if (gitUrl.isNotBlank()) { | ||
| gitUrl.split('/').last().removeSuffix(".git") |
There was a problem hiding this comment.
Potential crash: The function uses .last() instead of .lastOrNull(), which will throw a NoSuchElementException if the gitUrl doesn't contain a '/'. This can happen if the user enters an invalid Git URL. Change to: gitUrl.split('/').lastOrNull()?.removeSuffix(".git") ?: projectId to safely handle edge cases.
| gitUrl.split('/').last().removeSuffix(".git") | |
| gitUrl.split('/').lastOrNull()?.removeSuffix(".git") ?: projectId |
| class IdeaRemoteAgentViewModelTest { | ||
|
|
||
| private lateinit var testScope: CoroutineScope | ||
|
|
||
| @BeforeEach | ||
| fun setUp() { | ||
| testScope = CoroutineScope(SupervisorJob() + Dispatchers.Default) | ||
| } | ||
|
|
||
| @AfterEach | ||
| fun tearDown() { | ||
| testScope.cancel() | ||
| } | ||
|
|
||
| @Test | ||
| fun testInitialState() = runBlocking { | ||
| // Create a mock project-free test by testing the renderer and state directly | ||
| // We can't easily test the full ViewModel without IntelliJ Platform, | ||
| // but we can test the renderer and state management | ||
| val renderer = JewelRenderer() | ||
|
|
||
| // Verify initial renderer state | ||
| val timeline = renderer.timeline.first() | ||
| assertTrue(timeline.isEmpty()) | ||
|
|
||
| val isProcessing = renderer.isProcessing.first() | ||
| assertFalse(isProcessing) | ||
|
|
||
| val errorMessage = renderer.errorMessage.first() | ||
| assertNull(errorMessage) | ||
| } | ||
|
|
||
| @Test | ||
| fun testRendererHandlesIterationEvent() = runBlocking { | ||
| val renderer = JewelRenderer() | ||
|
|
||
| // Simulate handling iteration event | ||
| renderer.renderIterationHeader(3, 10) | ||
|
|
||
| val currentIteration = renderer.currentIteration.first() | ||
| assertEquals(3, currentIteration) | ||
|
|
||
| val maxIterations = renderer.maxIterations.first() | ||
| assertEquals(10, maxIterations) | ||
| } | ||
|
|
||
| @Test | ||
| fun testRendererHandlesLLMChunkEvent() = runBlocking { | ||
| val renderer = JewelRenderer() | ||
|
|
||
| // Simulate LLM streaming | ||
| renderer.renderLLMResponseStart() | ||
| assertTrue(renderer.isProcessing.first()) | ||
|
|
||
| renderer.renderLLMResponseChunk("Hello ") | ||
| renderer.renderLLMResponseChunk("world!") | ||
|
|
||
| val streamingOutput = renderer.currentStreamingOutput.first() | ||
| assertTrue(streamingOutput.contains("Hello")) | ||
| assertTrue(streamingOutput.contains("world")) | ||
|
|
||
| renderer.renderLLMResponseEnd() | ||
| assertFalse(renderer.isProcessing.first()) | ||
| } | ||
|
|
||
| @Test | ||
| fun testRendererHandlesToolCallEvent() = runBlocking { | ||
| val renderer = JewelRenderer() | ||
|
|
||
| // Simulate tool call | ||
| renderer.renderToolCall("read-file", "path=\"/test/file.txt\"") | ||
|
|
||
| val currentToolCall = renderer.currentToolCall.first() | ||
| assertNotNull(currentToolCall) | ||
|
|
||
| val timeline = renderer.timeline.first() | ||
| assertEquals(1, timeline.size) | ||
| assertTrue(timeline.first() is JewelRenderer.TimelineItem.ToolCallItem) | ||
| } | ||
|
|
||
| @Test | ||
| fun testRendererHandlesToolResultEvent() = runBlocking { | ||
| val renderer = JewelRenderer() | ||
|
|
||
| // Simulate tool call and result | ||
| renderer.renderToolCall("read-file", "path=\"/test/file.txt\"") | ||
| renderer.renderToolResult( | ||
| toolName = "read-file", | ||
| success = true, | ||
| output = "File content", | ||
| fullOutput = "Full file content", | ||
| metadata = emptyMap() | ||
| ) | ||
|
|
||
| val currentToolCall = renderer.currentToolCall.first() | ||
| assertNull(currentToolCall) | ||
|
|
||
| val timeline = renderer.timeline.first() | ||
| assertEquals(1, timeline.size) | ||
|
|
||
| val toolItem = timeline.first() as JewelRenderer.TimelineItem.ToolCallItem | ||
| assertEquals(true, toolItem.success) | ||
| } | ||
|
|
||
| @Test | ||
| fun testRendererHandlesErrorEvent() = runBlocking { | ||
| val renderer = JewelRenderer() | ||
|
|
||
| renderer.renderError("Connection failed") | ||
|
|
||
| val errorMessage = renderer.errorMessage.first() | ||
| assertEquals("Connection failed", errorMessage) | ||
|
|
||
| val timeline = renderer.timeline.first() | ||
| assertEquals(1, timeline.size) | ||
| assertTrue(timeline.first() is JewelRenderer.TimelineItem.ErrorItem) | ||
| } | ||
|
|
||
| @Test | ||
| fun testRendererHandlesCompleteEvent() = runBlocking { | ||
| val renderer = JewelRenderer() | ||
|
|
||
| renderer.renderFinalResult(true, "Task completed", 5) | ||
|
|
||
| val timeline = renderer.timeline.first() | ||
| assertEquals(1, timeline.size) | ||
|
|
||
| val item = timeline.first() as JewelRenderer.TimelineItem.TaskCompleteItem | ||
| assertTrue(item.success) | ||
| assertEquals("Task completed", item.message) | ||
| assertEquals(5, item.iterations) | ||
| } | ||
|
|
||
| @Test | ||
| fun testRendererClearTimeline() = runBlocking { | ||
| val renderer = JewelRenderer() | ||
|
|
||
| // Add some items | ||
| renderer.addUserMessage("User message") | ||
| renderer.renderError("An error") | ||
|
|
||
| var timeline = renderer.timeline.first() | ||
| assertEquals(2, timeline.size) | ||
|
|
||
| // Clear timeline | ||
| renderer.clearTimeline() | ||
|
|
||
| timeline = renderer.timeline.first() | ||
| assertTrue(timeline.isEmpty()) | ||
|
|
||
| val errorMessage = renderer.errorMessage.first() | ||
| assertNull(errorMessage) | ||
| } | ||
|
|
||
| @Test | ||
| fun testRendererForceStop() = runBlocking { | ||
| val renderer = JewelRenderer() | ||
|
|
||
| // Start streaming | ||
| renderer.renderLLMResponseStart() | ||
| renderer.renderLLMResponseChunk("Partial output") | ||
|
|
||
| assertTrue(renderer.isProcessing.first()) | ||
|
|
||
| // Force stop | ||
| renderer.forceStop() | ||
|
|
||
| assertFalse(renderer.isProcessing.first()) | ||
|
|
||
| // Verify interrupted message was added | ||
| val timeline = renderer.timeline.first() | ||
| assertTrue(timeline.isNotEmpty()) | ||
| val lastItem = timeline.last() | ||
| assertTrue(lastItem is JewelRenderer.TimelineItem.MessageItem) | ||
| assertTrue((lastItem as JewelRenderer.TimelineItem.MessageItem).content.contains("[Interrupted]")) | ||
| } | ||
|
|
||
| @Test | ||
| fun testRendererClearError() = runBlocking { | ||
| val renderer = JewelRenderer() | ||
|
|
||
| // Set error | ||
| renderer.renderError("Test error") | ||
| assertEquals("Test error", renderer.errorMessage.first()) | ||
|
|
||
| // Clear error | ||
| renderer.clearError() | ||
| assertNull(renderer.errorMessage.first()) | ||
| } | ||
|
|
||
| @Test | ||
| fun testRendererAddUserMessage() = runBlocking { | ||
| val renderer = JewelRenderer() | ||
|
|
||
| renderer.addUserMessage("Hello from user") | ||
|
|
||
| val timeline = renderer.timeline.first() | ||
| assertEquals(1, timeline.size) | ||
|
|
||
| val item = timeline.first() as JewelRenderer.TimelineItem.MessageItem | ||
| assertEquals(JewelRenderer.MessageRole.USER, item.role) | ||
| assertEquals("Hello from user", item.content) | ||
| } | ||
|
|
||
| @Test | ||
| fun testRemoteAgentRequestBuilder() { | ||
| // Test the request building logic | ||
| val projectId = "test-project" | ||
| val task = "Fix the bug" | ||
| val gitUrl = "" | ||
|
|
||
| // When gitUrl is empty, should use projectId | ||
| val request = RemoteAgentRequest( | ||
| projectId = projectId, | ||
| task = task, | ||
| llmConfig = null, | ||
| gitUrl = if (gitUrl.isNotBlank()) gitUrl else null | ||
| ) | ||
|
|
||
| assertEquals("test-project", request.projectId) | ||
| assertEquals("Fix the bug", request.task) | ||
| assertNull(request.gitUrl) | ||
| } | ||
|
|
||
| @Test | ||
| fun testRemoteAgentRequestWithGitUrl() { | ||
| // Test the request building logic with git URL | ||
| val gitUrl = "https://github.com/user/repo.git" | ||
| val task = "Fix the bug" | ||
|
|
||
| val projectId = gitUrl.split('/').lastOrNull()?.removeSuffix(".git") ?: "temp-project" | ||
|
|
||
| val request = RemoteAgentRequest( | ||
| projectId = projectId, | ||
| task = task, | ||
| llmConfig = null, | ||
| gitUrl = gitUrl | ||
| ) | ||
|
|
||
| assertEquals("repo", request.projectId) | ||
| assertEquals("Fix the bug", request.task) | ||
| assertEquals(gitUrl, request.gitUrl) | ||
| } | ||
|
|
||
| @Test | ||
| fun testLLMConfigSerialization() { | ||
| val config = LLMConfig( | ||
| provider = "OpenAI", | ||
| modelName = "gpt-4", | ||
| apiKey = "test-key", | ||
| baseUrl = "https://api.openai.com" | ||
| ) | ||
|
|
||
| assertEquals("OpenAI", config.provider) | ||
| assertEquals("gpt-4", config.modelName) | ||
| assertEquals("test-key", config.apiKey) | ||
| assertEquals("https://api.openai.com", config.baseUrl) | ||
| } | ||
|
|
||
| @Test | ||
| fun testHealthResponseParsing() { | ||
| val response = HealthResponse(status = "ok") | ||
| assertEquals("ok", response.status) | ||
| } | ||
|
|
||
| @Test | ||
| fun testProjectInfoParsing() { | ||
| val project = ProjectInfo( | ||
| id = "proj-1", | ||
| name = "My Project", | ||
| path = "/path/to/project", | ||
| description = "A test project" | ||
| ) | ||
|
|
||
| assertEquals("proj-1", project.id) | ||
| assertEquals("My Project", project.name) | ||
| assertEquals("/path/to/project", project.path) | ||
| assertEquals("A test project", project.description) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing test coverage for the IdeaRemoteAgentViewModel class itself. The test file only tests the JewelRenderer and data classes, but doesn't test the actual ViewModel methods like updateServerUrl(), checkConnection(), executeTask(), cancelTask(), etc. While these methods may require mocking the HTTP client, at minimum you should test the state management logic (e.g., verify that updateServerUrl properly updates the server URL, resets connection state, and recreates the client).
| maxReconnectionAttempts = 3 | ||
| } | ||
|
|
||
| expectSuccess = false |
There was a problem hiding this comment.
The expectSuccess = false setting in the HTTP client configuration is concerning. This means that HTTP error responses (4xx, 5xx) won't throw exceptions automatically, which could lead to silent failures. Consider removing this line to enable proper error handling, or add explicit error checking in the response handling code. If there's a specific reason to keep this, it should be documented with a comment explaining why.
| expectSuccess = false |
- Handle trailing slashes in Git URLs when extracting project ID - Add HTTP response status validation in healthCheck() and getProjects() - Add comment explaining expectSuccess = false for SSE connections - Add comprehensive tests for getEffectiveProjectId edge cases
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt (1)
29-232: Tests focus on JewelRenderer rather than ViewModel behavior.This duplicates a previous review comment. The tests validate
JewelRendererfunctionality but don't testIdeaRemoteAgentViewModelmethods likeupdateServerUrl(),checkConnection(),cancelTask(), or state transitions. While full integration tests require mocking the HTTP client, you could add unit tests for:
updateServerUrlresets connection statecancelTasksetsisExecutingto false- Initial state of
isConnected,connectionError, etc.Consider adding a separate test class with a mocked
IdeaRemoteAgentClientfor more comprehensive coverage.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.kt (1)
103-103: ReplaceprintStackTrace()with IntelliJ's logging framework.This duplicates a previous review comment. Use
com.intellij.openapi.diagnostic.Loggerfor consistent error tracking in IntelliJ plugins.+ private val LOG = com.intellij.openapi.diagnostic.Logger.getInstance(IdeaRemoteAgentClient::class.java) + fun executeStream(request: RemoteAgentRequest): Flow<RemoteAgentEvent> = flow { try { // ... } catch (e: Exception) { - e.printStackTrace() + LOG.error("Stream connection failed", e) throw RemoteAgentException("Stream connection failed: ${e.message}", e) } }mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModel.kt (2)
88-91: Replaceprintlnwith IntelliJ's logging framework.This duplicates a previous review comment. Use
Logger.getInstance()for consistent logging.+ private val LOG = com.intellij.openapi.diagnostic.Logger.getInstance(IdeaRemoteAgentViewModel::class.java) + fun executeTask(projectId: String, task: String, gitUrl: String = "") { if (_isExecuting.value) { - println("Agent is already executing") + LOG.warn("Agent is already executing") return }
135-144: CatchingCancellationExceptionprevents proper coroutine cancellation propagation.This duplicates a previous review comment.
CancellationExceptionshould be re-thrown to maintain structured concurrency. Consider using afinallyblock for cleanup:client.executeStream(request).collect { event -> handleRemoteEvent(event) - - if (event is RemoteAgentEvent.Complete) { - _isExecuting.value = false - currentExecutionJob = null - } } - - } catch (e: CancellationException) { - renderer.forceStop() - renderer.renderError("Task cancelled by user") - _isExecuting.value = false - currentExecutionJob = null } catch (e: Exception) { + if (e is CancellationException) { + renderer.forceStop() + throw e // Re-throw to maintain structured concurrency + } renderer.renderError(e.message ?: "Unknown error") + } finally { _isExecuting.value = false currentExecutionJob = null }
🧹 Nitpick comments (3)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.kt (1)
116-125: Consider adding@SerialNameannotations for API stability.If the server API uses different JSON field names (e.g.,
git_urlvsgitUrl), deserialization will fail silently withignoreUnknownKeys = true. Adding explicit@SerialNameannotations documents the contract and prevents issues if field naming conventions differ.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentContent.kt (1)
48-54: Connection check triggered on every server URL change may cause excessive API calls.The
LaunchedEffect(serverUrl)block callsviewModel.checkConnection()wheneverserverUrlchanges, including during typing. This could result in many failed health check requests as the user types the URL character by character.Consider debouncing the connection check or only triggering it via the "Connect" button:
- // Check connection on initial load and when server URL changes - LaunchedEffect(serverUrl) { - if (serverUrl.isNotBlank()) { - viewModel.updateServerUrl(serverUrl) - viewModel.checkConnection() - } - } + // Only update the server URL; let user trigger connection via Connect button + LaunchedEffect(serverUrl) { + if (serverUrl.isNotBlank()) { + viewModel.updateServerUrl(serverUrl) + } + }Alternatively, add debouncing if auto-connect is desired.
mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt (1)
253-271: Test duplicates request building logic instead of testing the ViewModel'sbuildRequestmethod.The test manually constructs the project ID extraction logic (
gitUrl.split('/').lastOrNull()?.removeSuffix(".git")) rather than testing the actualbuildRequestmethod inIdeaRemoteAgentViewModel. This means the test could pass even if the ViewModel's logic differs.Since
buildRequestis private, consider either:
- Making it
internalfor testing, or- Testing through the public
executeTaskAPI with a mocked client
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentContent.kt(1 hunks)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModel.kt(1 hunks)mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
Use
expect/actualfor platform-specific code in KMP projects (e.g., file I/O on JVM/JS/Wasm)
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModel.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.ktmpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentContent.ktmpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt
🧠 Learnings (1)
📚 Learning: 2025-11-30T02:30:49.805Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-30T02:30:49.805Z
Learning: Use `cd mpp-idea && ../gradlew test --tests "cc.unitmesh.devins.idea.renderer.JewelRendererTest"` for running standalone JUnit 5 tests that do not require IntelliJ Platform
Applied to files:
mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt
🧬 Code graph analysis (2)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentContent.kt (1)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/timeline/IdeaTimelineContent.kt (1)
IdeaTimelineContent(20-48)
mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt (2)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/test/DevInsTestCase.kt (5)
assertTrue(29-33)assertFalse(38-40)assertNull(64-68)assertEquals(45-49)assertNotNull(54-59)mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentContent.kt (1)
getEffectiveProjectId(312-323)
⏰ 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: Build (223)
- GitHub Check: Build (241)
🔇 Additional comments (4)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentContent.kt (1)
312-323: LGTM! Edge cases for Git URL parsing are now properly handled.The implementation correctly handles trailing slashes, empty segments, and falls back to
projectIdwhen extraction fails. The tests inIdeaRemoteAgentViewModelTest.ktprovide good coverage for these edge cases.mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModel.kt (2)
186-192: Good refactor: Centralized URL parsing logic.The
extractProjectIdFromUrlhelper properly handles trailing slashes and empty segments, addressing the previous code duplication concerns. This is now consistent withgetEffectiveProjectIdinIdeaRemoteAgentContent.kt.
284-287: The dispose method correctly handles cleanup for this ViewModel.The
coroutineScopeis passed as an external parameter from the caller (IdeaAgentApp) and is not owned by the ViewModel. Cancelling it indispose()would be inappropriate since the scope lifecycle is managed externally and may be used by other components. The current cleanup—cancellingcurrentExecutionJoband closingclient—is correct.However, note that
checkConnection()at line 67 launches a coroutine that is not tracked in a variable. If this becomes a concern for resource cleanup, consider tracking it similarly tocurrentExecutionJob.Likely an incorrect or invalid review comment.
mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt (1)
311-359: Good coverage ofgetEffectiveProjectIdedge cases.The tests thoroughly cover various edge cases including trailing slashes, multiple slashes, empty URLs, blank URLs, and
.gitsuffix handling. This provides confidence that the URL parsing logic is robust.
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.kt
Show resolved
Hide resolved
- Add HTTP status validation in executeStream before processing SSE events - Add comprehensive tests for data classes and state management - Test RemoteAgentRequest with all fields and default values - Test LLMConfig with null baseUrl - Test ProjectListResponse with empty and multiple projects - Test RemoteAgentException with and without cause - Test getEffectiveProjectId with SSH URLs and deep paths
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt (2)
274-286: Document that test API keys should never be real secrets.While using hardcoded test keys like
"test-key"is acceptable in tests, consider adding a comment to establish best practices: (1) never use real API keys in tests, (2) ensure production code transmits API keys over HTTPS only, (3) verify API keys aren't logged in production.Add a comment at the top of the test class:
/** * Unit tests for IdeaRemoteAgentViewModel. * + * Note: All API keys in these tests are dummy values. + * Production code must never log API keys and should only transmit them over HTTPS. + * * Tests the ViewModel's functionality including:Also applies to: 382-387, 409-421
15-28: Test class doesn't match its name and documentation.The class is named
IdeaRemoteAgentViewModelTestand documented as "Unit tests for IdeaRemoteAgentViewModel," but it only testsJewelRendererand data classes—not the actual ViewModel methods likeupdateServerUrl(),checkConnection(),executeTask(), etc.Consider either:
- Renaming to reflect actual scope:
IdeaRemoteAgentComponentsTestorJewelRendererAndDataClassesTest- Adding actual ViewModel tests (may require mocking the HTTP client)
Misleading test class names create maintenance burden and false confidence in test coverage.
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.kt (1)
107-109: ReplaceprintStackTrace()with IntelliJ Platform Logger.Using
printStackTrace()writes directly to stderr and isn't integrated with the IDE's logging infrastructure. IntelliJ plugins should usecom.intellij.openapi.diagnostic.Loggerfor consistent error tracking and debugging.Apply this diff:
+import com.intellij.openapi.diagnostic.Logger + class IdeaRemoteAgentClient( private val baseUrl: String = "http://localhost:8080" ) { + companion object { + private val LOG = Logger.getInstance(IdeaRemoteAgentClient::class.java) + } + private val httpClient: HttpClient = HttpClient(CIO) {} catch (e: Exception) { - e.printStackTrace() + LOG.error("Stream connection failed", e) throw RemoteAgentException("Stream connection failed: ${e.message}", e) }
🧹 Nitpick comments (1)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.kt (1)
23-46: Consider implementingCloseablefor idiomatic resource management.The
HttpClientis created in the constructor and has aclose()method, but the class doesn't implementCloseableorAutoCloseable. ImplementingCloseablewould enable use of Kotlin's.use {}pattern and make resource cleanup more explicit.Apply this diff:
-class IdeaRemoteAgentClient( +class IdeaRemoteAgentClient( private val baseUrl: String = "http://localhost:8080" -) { +) : Closeable {Then the close method becomes an override:
- fun close() { + override fun close() { httpClient.close() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.kt(1 hunks)mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
📄 CodeRabbit inference engine (AGENTS.md)
Use
expect/actualfor platform-specific code in KMP projects (e.g., file I/O on JVM/JS/Wasm)
Files:
mpp-idea/src/main/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentClient.ktmpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.kt
🧠 Learnings (1)
📚 Learning: 2025-11-30T02:30:49.805Z
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-30T02:30:49.805Z
Learning: Use `cd mpp-idea && ../gradlew test --tests "cc.unitmesh.devins.idea.renderer.JewelRendererTest"` for running standalone JUnit 5 tests that do not require IntelliJ Platform
Applied to files:
mpp-idea/src/test/kotlin/cc/unitmesh/devins/idea/toolwindow/remote/IdeaRemoteAgentViewModelTest.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: Build (223)
- GitHub Check: Build (241)
1 participant
Summary
This PR implements a Remote Agent for mpp-idea by referencing the existing
RemoteAgentChatInterfaceimplementation in mpp-ui.Changes
New Files Created:
IdeaRemoteAgentClient.ktRemoteAgentRequest,LLMConfig,HealthResponse,ProjectInfo, etc.)IdeaRemoteAgentViewModel.ktJewelRenderer(instead ofComposeRendererlike mpp-ui)RemoteAgentEventto renderer calls for UI displayIdeaRemoteAgentContent.ktIdeaRemoteAgentViewModelTest.ktModified Files:
IdeaAgentApp.ktremoteAgentViewModelstate managementIdeaRemoteAgentContentfor REMOTE agent typeArchitecture
The implementation follows the same pattern as mpp-ui's
RemoteAgentChatInterface:Key differences from mpp-ui:
JewelRendererinstead ofComposeRendererfor native IntelliJ themingrememberTextFieldState()for Jewel TextField componentsTesting
Pull Request opened by Augment Code with guidance from the PR author
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.