feat(mpp-vscode): init vscode extension project structure #32
Conversation
- Create project directory structure - Add package.json with VSCode extension configuration - Add tsconfig.json for TypeScript - Add README.md with TODO list and architecture overview Closes #31
|
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 new mpp-vscode VS Code extension: project scaffold, TypeScript configs, a Kotlin/JS bridge to mpp-core, extension activation and webview chat UI (React), diff management, an MCP-style local IDE HTTP server, DevIns language support, status/logger utilities, and tests/mocks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as External client/tool
participant IDE as IDEServer (Express)
participant Ext as VSCode Extension
participant Diff as DiffManager
participant FS as VSCode Workspace FS
Client->>IDE: POST /diff/open {filePath, content} (Bearer token)
IDE->>Ext: call diffManager.showDiff(filePath, content)
Ext->>Diff: showDiff(filePath, content)
Diff->>Ext: register right-doc URI & set content via DiffContentProvider
Ext-->>IDE: 200 OK (opened)
Note over Ext,Diff: User reviews diff in VSCode UI
User->>Ext: accept via command/UI
Ext->>Diff: acceptDiff(rightDocUri)
Diff->>FS: workspace.fs.writeFile(originalFilePath, newContent)
FS-->>Diff: write success
Diff-->>Ext: emit accepted event
Ext-->>IDE: optionally notify client of acceptance
IDE-->>Client: 200 OK (accepted)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| "default": "gpt-4", | ||
| "description": "Model name" | ||
| }, | ||
| "autodev.apiKey": { |
There was a problem hiding this comment.
Defining autodev.apiKey as a plain-text setting stores keys in settings.json (and potentially sync), which is insecure; consider using VS Code SecretStorage and collecting the key via a command instead (and remove this setting). (Guideline: no_hardcoded_credentials)
🤖 Was this useful? React with 👍 or 👎
| "@vscode/vsce": "^2.22.0" | ||
| }, | ||
| "dependencies": { | ||
| "@autodev/mpp-core": "file:../mpp-core/build/packages/js", |
There was a problem hiding this comment.
Local file: dependency @autodev/mpp-core points outside the extension folder, which typically breaks vsce package/publishing and makes installs non-portable; consider using a published package or bundling the JS output into the extension.
🤖 Was this useful? React with 👍 or 👎
| { | ||
| "id": "autodev-sidebar", | ||
| "title": "AutoDev", | ||
| "icon": "resources/icon.svg" |
There was a problem hiding this comment.
resources/icon.svg is referenced for the activity bar container but the file isn’t included in this PR; VS Code will show a missing/default icon and vsce may warn, so consider adding the asset or removing the reference until available.
🤖 Was this useful? React with 👍 or 👎
mpp-vscode/package.json
Outdated
| "license": "Apache-2.0", | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/phodal/auto-dev" |
There was a problem hiding this comment.
repository.url points to phodal/auto-dev but this project appears to live in phodal/auto-dev-sketch; correcting it will avoid broken links in Marketplace/npm metadata.
🤖 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
🧹 Nitpick comments (1)
mpp-vscode/README.md (1)
1-128: Consider English documentation for accessibility.The README is written entirely in Chinese, which limits discoverability and contribution from international developers. Consider providing an English version or translating key sections (architecture, setup, tech stack).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mpp-vscode/README.md(1 hunks)mpp-vscode/package.json(1 hunks)mpp-vscode/tsconfig.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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 mpp-ui/src/jsMain/typescript/**/*.{ts,tsx} : In CLI/TUI (TypeScript), use `mpp-ui/src/jsMain/typescript/design-system/` and import `semanticInk` / `semanticChalk`
Applied to files:
mpp-vscode/tsconfig.json
⏰ 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 (5)
mpp-vscode/tsconfig.json (1)
1-20: TypeScript configuration is well-suited for the VSCode extension.The configuration appropriately targets ES2022, uses CommonJS for Node.js runtime, enables strict type checking, and generates declaration files and source maps. The exclusion of
webviewaligns with the separate React UI project mentioned in the README.mpp-vscode/package.json (3)
8-11: Verify repository URL.The repository URL points to
https://github.com/phodal/auto-dev, but based on the PR context, it should likely referencehttps://github.com/phodal/auto-dev-sketch. Confirm which is correct.
106-112: Build and packaging configuration is well-structured.The esbuild command correctly bundles the extension with
--platform=node,--format=cjs, and excludesvscode(external). The scripts properly separate build (npm run build) and watch modes (npm run watch), and thevscode:prepublishhook ensures the extension is built before publishing.
29-29: Extension entry point will be implemented in Phase 2.Line 29 references
./dist/extension.jsas the main entry, butsrc/extension.tsis not included in this PR (Phase 1 infra only). Ensureextension.tsis prioritized in Phase 2 implementation.This is not a blocker for Phase 1, but document in Phase 2 planning that
src/extension.tsis the critical entry point.mpp-vscode/README.md (1)
95-100: Replace hardcoded absolute path with a reproducible command.Line 98 contains a machine-specific absolute path (
/Volumes/source/ai/autocrud) that is not portable across development environments. Provide a relative path or generic instructions.For example, replace with a path-agnostic instruction or assume the user is in the repository root:
### 构建 mpp-core ```bash - cd /Volumes/source/ai/autocrud + cd ../mpp-core ./gradlew :mpp-core:assembleJsPackageAlternatively, document where `mpp-core` is expected to be located relative to this project. <details> <summary>⛔ Skipped due to learnings</summary>Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Build MPP Core with:cd /Volumes/source/ai/autocrud && ./gradlew :mpp-core:assembleJsPackageLearnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Formpp-ideaplugin development, usecd mpp-idea && ../gradlew compileKotlininstead of./gradlew :mpp-idea:compileKotlinLearnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Build and run MPP CLI with:cd mpp-ui && npm run build && npm run startLearnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Do not run./gradlew clean. Clean only the specific module, e.g.,./gradlew :mpp-core:cleanLearnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Use ./gradlew :mpp-ui:generateI18n4kFiles for i18n generationLearnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: For release: modify version ingradle.properties, then publish cli version withcd mpp-ui && npm publish:remoteLearnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Formpp-ideaplugin tests, run with:cd mpp-idea && ../gradlew test --tests "cc.unitmesh.devins.idea.renderer.JewelRendererTest"</details> </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| "viewsContainers": { | ||
| "activitybar": [ | ||
| { | ||
| "id": "autodev-sidebar", | ||
| "title": "AutoDev", | ||
| "icon": "resources/icon.svg" | ||
| } | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for icon.svg in the entire mpp-vscode directory and check common locations
echo "=== Searching for icon.svg in mpp-vscode directory ==="
fd -t f "icon.svg" mpp-vscode/
echo -e "\n=== Listing resources directory contents if it exists ==="
if [ -d "mpp-vscode/resources" ]; then
ls -la mpp-vscode/resources/
else
echo "mpp-vscode/resources directory does not exist"
fi
echo -e "\n=== Checking file structure of mpp-vscode ==="
ls -la mpp-vscode/ | head -20Repository: phodal/auto-dev-sketch
Length of output: 573
Add the missing icon asset or update the path reference.
Line 54 references resources/icon.svg for the sidebar icon, but neither the resources directory nor the icon.svg file exists in the mpp-vscode directory. The VSCode extension will fail to render the icon at runtime. Either add the asset file or update the reference to point to an existing icon location.
🤖 Prompt for AI Agents
mpp-vscode/package.json lines 49-56: the extension references
"resources/icon.svg" for the activitybar view container but that file/path does
not exist; either add the missing resources/icon.svg asset into the mpp-vscode
repository or update the package.json to point to an existing icon (e.g., adjust
the path to the actual icon location or replace with a bundled icon filename).
Ensure the selected path is committed to the repo and that the icon file is a
valid SVG (or supported image) so VSCode can load it at runtime.
| "dependencies": { | ||
| "@autodev/mpp-core": "file:../mpp-core/build/packages/js", | ||
| "@modelcontextprotocol/sdk": "^1.0.0", | ||
| "cors": "^2.8.5", | ||
| "express": "^4.18.2", | ||
| "zod": "^3.22.4" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Explore repository structure
git ls-files | head -20Repository: phodal/auto-dev-sketch
Length of output: 681
🏁 Script executed:
# Check if mpp-core build configuration exists
fd -e gradle -e gradle.kts | head -20Repository: phodal/auto-dev-sketch
Length of output: 326
🏁 Script executed:
# Look for package.json files to understand monorepo structure
fd -n package.jsonRepository: phodal/auto-dev-sketch
Length of output: 296
🏁 Script executed:
# Check root package.json for workspace configuration
cat -n package.json 2>/dev/null || echo "No root package.json found"Repository: phodal/auto-dev-sketch
Length of output: 92
🏁 Script executed:
# Look for CI/CD workflow files
fd -e yml -e yaml | grep -i -E '(workflow|action|ci|cd)' | head -10Repository: phodal/auto-dev-sketch
Length of output: 48
🏁 Script executed:
# Examine mpp-core build configuration to find JS package output path
head -100 mpp-core/build.gradle.ktsRepository: phodal/auto-dev-sketch
Length of output: 2640
🏁 Script executed:
# Search for assembleJsPackage task in mpp-core
rg -A 10 -B 5 'assembleJsPackage' mpp-core/Repository: phodal/auto-dev-sketch
Length of output: 48
🏁 Script executed:
# Examine mpp-vscode package.json to understand full context
cat -n mpp-vscode/package.jsonRepository: phodal/auto-dev-sketch
Length of output: 4299
🏁 Script executed:
# Check CI/CD workflows to see how this path is used
cat -n .github/workflows/build.ymlRepository: phodal/auto-dev-sketch
Length of output: 9516
🏁 Script executed:
# Check for other workflow files that might handle mpp-vscode or publishing
ls -la .github/workflows/ | grep -v totalRepository: phodal/auto-dev-sketch
Length of output: 563
🏁 Script executed:
# Check mpp-core build.gradle.kts for npm publish plugin (lines 100-200)
sed -n '100,200p' mpp-core/build.gradle.ktsRepository: phodal/auto-dev-sketch
Length of output: 4008
🏁 Script executed:
# Look for npm publish configuration in mpp-core
rg -i 'npm|publish' mpp-core/build.gradle.ktsRepository: phodal/auto-dev-sketch
Length of output: 403
🏁 Script executed:
# Check release.yml workflow
head -100 .github/workflows/release.ymlRepository: phodal/auto-dev-sketch
Length of output: 2590
🏁 Script executed:
# Check compose-release.yml to see if it handles npm publishing
head -150 .github/workflows/compose-release.ymlRepository: phodal/auto-dev-sketch
Length of output: 4191
🏁 Script executed:
# Search for npmPublish configuration in mpp-core build.gradle.kts
rg -A 20 'npmPublish' mpp-core/build.gradle.ktsRepository: phodal/auto-dev-sketch
Length of output: 813
🏁 Script executed:
# Check mpp-ui package.json for publish scripts (mentioned in learning)
cat -n mpp-ui/package.json 2>/dev/null | head -50Repository: phodal/auto-dev-sketch
Length of output: 2737
🏁 Script executed:
# Check the publish-remote.js script to understand npm publishing
cat -n mpp-ui/scripts/publish-remote.js 2>/dev/null | head -50Repository: phodal/auto-dev-sketch
Length of output: 1981
🏁 Script executed:
# Check AGENTS.md and README for documentation on this approach
cat -n AGENTS.md 2>/dev/null | head -80Repository: phodal/auto-dev-sketch
Length of output: 4092
🏁 Script executed:
# Search for any documentation about the build/publish process
rg -i 'build.*mpp-core|publish.*npm' --type md | head -20Repository: phodal/auto-dev-sketch
Length of output: 236
🏁 Script executed:
# Check if mpp-vscode has any build scripts or CI configuration
cat mpp-vscode/package.json | jq '.scripts' 2>/dev/nullRepository: phodal/auto-dev-sketch
Length of output: 334
🏁 Script executed:
# Search for any references to mpp-vscode in CI/CD workflows
rg 'mpp-vscode' .github/workflows/Repository: phodal/auto-dev-sketch
Length of output: 48
🏁 Script executed:
# Check if there's a build.gradle.kts for mpp-vscode (unlikely but check)
fd build.gradle.kts mpp-vscode/Repository: phodal/auto-dev-sketch
Length of output: 48
Review dependency path stability.
Line 126 uses a relative file path (file:../mpp-core/build/packages/js) to reference @autodev/mpp-core. While this is an intentional monorepo development pattern (the codebase uses local build outputs during development), it creates brittleness for mpp-vscode specifically:
- The build output location (
build/packages/js) must exist after running./gradlew :mpp-core:assembleJsPackage - mpp-vscode has no CI/CD automation; the dependency path is only workable via manual local setup
- Unlike mpp-ui (which has
npm publish:remoteautomation to convertfile:to npm versions), mpp-vscode lacks this workflow integration - Future restructuring of mpp-core's build output will break this reference unless the automation is updated
Consider either integrating mpp-vscode into the npm publishing workflow (similar to mpp-ui), using a workspace configuration, or publishing @autodev/mpp-core to npm so external projects can consume it reliably.
🤖 Prompt for AI Agents
In mpp-vscode/package.json around lines 125 to 131, the dependency on
"@autodev/mpp-core" uses a brittle local file path
("file:../mpp-core/build/packages/js"); replace this with a robust solution:
either (A) point the dependency to a published npm version (e.g. "^x.y.z") and
add publishing steps for mpp-core, (B) adopt a workspace-based reference (e.g.
convert to a workspace monorepo and use "workspace:*" or a package name that
resolves via workspaces) and ensure root package.json includes the workspace
pattern, or (C) add CI steps to build and publish mpp-core and to
transform/remove the file: reference during CI (or run the assemble step and
assert the build path exists before install); update package.json accordingly
and add CI automation to keep the path resolution reliable.
Phase 2 - Core Services: - Add mpp-core bridge layer with LLMService, CodingAgent, ToolRegistry, CompletionManager, and DevInsCompiler wrappers - Implement extension.ts entry point with activation/deactivation Phase 3 - IDE Integration: - Implement IDE Server with Express HTTP endpoints - Add DiffManager for file diff operations - Support MCP protocol communication Phase 4 - Chat Interface: - Implement ChatViewProvider with webview - Add streaming LLM response support - Create embedded HTML chat UI Phase 5 - Commands: - Register autodev.chat, autodev.acceptDiff, autodev.cancelDiff, autodev.runAgent - Add keyboard shortcut (Cmd+Shift+A) Testing: - Add vitest configuration - Add unit tests for mpp-core bridge - Add unit tests for DiffManager - Add unit tests for IDEServer Refs #31
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
mpp-vscode/package.json (4)
10-10: Repository URL mismatch.The repository URL points to
phodal/auto-devbut this project is inphodal/auto-dev-sketch. This will cause broken links in VSCode Marketplace and npm metadata.Apply this diff:
- "url": "https://github.com/phodal/auto-dev" + "url": "https://github.com/phodal/auto-dev-sketch"
54-54: Missing icon asset.The extension references
resources/icon.svgbut this file is not included in the PR. VSCode will display a missing/default icon.
81-85: API key stored as plain-text setting is a security risk.Storing
autodev.apiKeyas a plain-text configuration exposes API keys insettings.jsonand potentially syncs them via Settings Sync. Use VSCode'sSecretStorageAPI instead, collecting the key via a command and removing this configuration property.
129-129: Brittle local file dependency path.The dependency uses
file:../mpp-core/build/packages/jswhich is fragile and breaks portability. This has been extensively discussed in past reviews.
🧹 Nitpick comments (16)
mpp-vscode/test/mocks/vscode.ts (1)
20-28: EventEmitter listener disposal has a subtle mutation issue.Line 24 mutates the
listenersarray during disposal by filtering. Ifdispose()is called while the event is firing (line 26), it could skip listeners. For test mocks this is likely fine, but consider using a more defensive pattern if tests become flaky.Consider storing a "disposed" flag instead:
export const EventEmitter = class { private listeners: Function[] = []; + private disposed = false; event = (listener: Function) => { this.listeners.push(listener); - return { dispose: () => { this.listeners = this.listeners.filter(l => l !== listener); } }; + return { + dispose: () => { + const idx = this.listeners.indexOf(listener); + if (idx !== -1) this.listeners.splice(idx, 1); + } + }; }; - fire = (data: any) => { this.listeners.forEach(l => l(data)); }; - dispose = () => { this.listeners = []; }; + fire = (data: any) => { + if (!this.disposed) { + this.listeners.slice().forEach(l => l(data)); + } + }; + dispose = () => { + this.disposed = true; + this.listeners = []; + }; };mpp-vscode/package.json (1)
26-28: Prefer more specific activation events.Using
onStartupFinishedactivates the extension on every VSCode startup, even when not needed. Consider more specific activation events likeonCommand:autodev.chatoronView:autodev-sidebarto improve startup performance and resource usage."activationEvents": [ - "onStartupFinished" + "onView:autodev-sidebar", + "onCommand:autodev.chat", + "onCommand:autodev.runAgent" ],mpp-vscode/test/services/diff-manager.test.ts (2)
8-47: Duplicate VSCode mock implementation.Lines 8-47 duplicate the VSCode API mock already defined in
test/mocks/vscode.ts. The vitest.config.ts already aliasesvscodeto the shared mock, so this inline mock is redundant and creates maintenance burden.Remove the inline mock and rely on the global alias:
-// Mock vscode module -vi.mock('vscode', () => ({ - Uri: { - file: (path: string) => ({ scheme: 'file', fsPath: path, path, toString: () => `file://${path}` }), - from: (components: { scheme: string; path: string; query?: string }) => ({ - scheme: components.scheme, - path: components.path, - query: components.query, - fsPath: components.path, - toString: () => `${components.scheme}://${components.path}${components.query ? '?' + components.query : ''}` - }), - parse: (value: string) => { - const url = new URL(value); - return { scheme: url.protocol.replace(':', ''), path: url.pathname, fsPath: url.pathname, toString: () => value }; - } - }, - EventEmitter: class { - private listeners: Function[] = []; - event = (listener: Function) => { - this.listeners.push(listener); - return { dispose: () => { this.listeners = this.listeners.filter(l => l !== listener); } }; - }; - fire = (data: any) => { this.listeners.forEach(l => l(data)); }; - dispose = () => { this.listeners = []; }; - }, - window: { - onDidChangeActiveTextEditor: () => ({ dispose: () => {} }), - activeTextEditor: undefined, - tabGroups: { all: [] } - }, - workspace: { - fs: { - stat: vi.fn().mockResolvedValue({}), - writeFile: vi.fn().mockResolvedValue(undefined) - }, - openTextDocument: vi.fn().mockResolvedValue({ getText: () => 'test content', uri: {} }) - }, - commands: { - executeCommand: vi.fn().mockResolvedValue(undefined) - } -})); - +// The vscode module is already mocked via vitest.config.ts alias
140-149: Test validates nothing about event emission.Lines 145-148 subscribe to the
onDidChangeevent but never trigger any action that would fire an event, soexpect(events).toHaveLength(0)always passes vacuously. This test provides no coverage of the event mechanism.Either enhance the test to trigger an event or remove it:
it('should emit events on diff changes', async () => { const { DiffManager, DiffContentProvider } = await import('../../src/services/diff-manager'); const contentProvider = new DiffContentProvider(); const manager = new DiffManager(mockLog, contentProvider); const events: any[] = []; manager.onDidChange((event) => events.push(event)); - expect(events).toHaveLength(0); + // TODO: Trigger an actual diff operation and verify event emission + // For now, just verify the subscription mechanism works + expect(events).toHaveLength(0); });mpp-vscode/test/services/ide-server.test.ts (2)
8-32: Duplicate mock implementations.Lines 8-32 duplicate VSCode and fs/promises mocks. Consider consolidating these into shared test fixtures in
test/mocks/to avoid maintenance overhead and drift.
90-124: Remove placeholder tests that provide no coverage.Lines 94-123 are placeholder tests that only check
IDEServeris defined. These tests provide no actual coverage of endpoint handlers or server behavior. Either implement meaningful tests or remove these placeholders to avoid false confidence in test coverage metrics.-describe('IDEServer API Endpoints', () => { - // These tests would require starting the actual server - // For unit tests, we verify the structure and methods exist - - it('should define health endpoint handler', async () => { - const { IDEServer } = await import('../../src/services/ide-server'); - expect(IDEServer).toBeDefined(); - // The actual endpoint testing would be done in integration tests - }); - - it('should define context endpoint handler', async () => { - const { IDEServer } = await import('../../src/services/ide-server'); - expect(IDEServer).toBeDefined(); - }); - - it('should define diff/open endpoint handler', async () => { - const { IDEServer } = await import('../../src/services/ide-server'); - expect(IDEServer).toBeDefined(); - }); - - it('should define diff/close endpoint handler', async () => { - const { IDEServer } = await import('../../src/services/ide-server'); - expect(IDEServer).toBeDefined(); - }); - - it('should define file/read endpoint handler', async () => { - const { IDEServer } = await import('../../src/services/ide-server'); - expect(IDEServer).toBeDefined(); - }); - - it('should define file/write endpoint handler', async () => { - const { IDEServer } = await import('../../src/services/ide-server'); - expect(IDEServer).toBeDefined(); - }); -}); +// TODO: Add integration tests for API endpoints in a separate test filempp-vscode/test/bridge/mpp-core.test.ts (1)
74-114: Consider adding behavior tests beyond structural checks.Lines 79-113 test that classes can be imported and instantiated, but don't verify any actual behavior (e.g., that
clearHistory()actually clears, that config values are used correctly). For a "Chill" review this is acceptable for initial scaffolding, but consider enhancing test coverage as the bridge matures.Example enhancement for LLMService:
it('should use provider configuration', async () => { const { LLMService } = await import('../../src/bridge/mpp-core'); const service = new LLMService({ provider: 'anthropic', model: 'claude-3', apiKey: 'test-key' }); // Verify the correct provider mapping is used expect(service).toBeDefined(); // Future: could verify internal koogService was created with correct args });mpp-vscode/vitest.config.ts (1)
7-7: Consider separating unit and integration test patterns.Line 7 includes both
src/**/*.test.tsandtest/**/*.test.ts. If you later add integration tests that need different setup (e.g., actually starting servers), consider using separate glob patterns or test directories to run them independently.// Future enhancement: include: ['src/**/*.test.ts', 'test/unit/**/*.test.ts'], // Then add separate config or script for integration testsmpp-vscode/src/extension.ts (2)
101-107: IDE server startup error handling could be more user-friendly.Lines 101-107 catch and log IDE server startup errors but don't inform the user. If the IDE server is critical for functionality, consider showing an error message to the user so they're aware of the failure.
try { await ideServer.start(context); log(`IDE Server started on port ${serverPort}`); } catch (err) { const message = err instanceof Error ? err.message : String(err); log(`Failed to start IDE server: ${message}`); + vscode.window.showErrorMessage(`AutoDev: Failed to start IDE server: ${message}`); }
117-123: globalState.update() should be awaited.Line 122 calls
context.globalState.update()without awaiting. While this probably won't cause issues in practice, it's better to await the operation to ensure the state is persisted before the activation completes.if (!context.globalState.get(welcomeShownKey)) { vscode.window.showInformationMessage( 'AutoDev extension installed successfully! Press Cmd+Shift+A to open chat.' ); - context.globalState.update(welcomeShownKey, true); + await context.globalState.update(welcomeShownKey, true); }mpp-vscode/src/providers/chat-view.ts (3)
36-45: Missing disposal for webview message handler subscription.The
onDidReceiveMessagehandler creates a subscription that should be disposed when the webview is disposed to prevent memory leaks.+ private disposables: vscode.Disposable[] = []; + resolveWebviewView( webviewView: vscode.WebviewView, _context: vscode.WebviewViewResolveContext, _token: vscode.CancellationToken ): void { this.webviewView = webviewView; webviewView.webview.options = { enableScripts: true, localResourceRoots: [this.context.extensionUri] }; webviewView.webview.html = this.getHtmlContent(); // Handle messages from webview - webviewView.webview.onDidReceiveMessage(async (message) => { + this.disposables.push( + webviewView.webview.onDidReceiveMessage(async (message) => { switch (message.type) { case 'sendMessage': await this.handleUserMessage(message.content); break; case 'clearHistory': this.clearHistory(); break; } - }); + }) + ); + + webviewView.onDidDispose(() => { + this.disposables.forEach(d => d.dispose()); + this.disposables = []; + }); // Initialize LLM service this.initializeLLMService(); }
69-95: Consider listening for configuration changes.The LLM service is initialized once but won't update if the user changes settings (e.g., API key, provider) without reloading the extension. Consider subscribing to
vscode.workspace.onDidChangeConfigurationto reinitialize when relevant settings change.
112-121: Minor redundancy:fullResponseis accumulated twice.
LLMService.streamMessagealready accumulates chunks and returns the full response. The localfullResponsevariable duplicates this accumulation. You can simplify by using the returned value directly.try { - let fullResponse = ''; - - await this.llmService.streamMessage(content, (chunk) => { - fullResponse += chunk; + const fullResponse = await this.llmService.streamMessage(content, (chunk) => { this.postMessage({ type: 'responseChunk', content: chunk }); }); this.messages.push({ role: 'assistant', content: fullResponse });mpp-vscode/src/services/diff-manager.ts (2)
14-38: DiffContentProvider should dispose its EventEmitter.
DiffContentProviderholds anEventEmitterthat should be disposed to avoid memory leaks. Consider adding adispose()method.export class DiffContentProvider implements vscode.TextDocumentContentProvider { private content = new Map<string, string>(); private onDidChangeEmitter = new vscode.EventEmitter<vscode.Uri>(); get onDidChange(): vscode.Event<vscode.Uri> { return this.onDidChangeEmitter.event; } + dispose(): void { + this.onDidChangeEmitter.dispose(); + } + provideTextDocumentContent(uri: vscode.Uri): string {
80-84: Dispose theonDidChangeEmitterinDiffManager.dispose().The
onDidChangeEmittershould be disposed alongside subscriptions.dispose(): void { for (const subscription of this.subscriptions) { subscription.dispose(); } + this.onDidChangeEmitter.dispose(); }mpp-vscode/src/bridge/mpp-core.ts (1)
330-331: Fragile access to internalkoogServicevia type assertion.Casting
llmService as anyto access the privatekoogServiceproperty is fragile. IfLLMServiceinternals change, this will fail silently at runtime.Consider exposing a getter in
LLMService:// In LLMService class: getInternalService(): any { return this.koogService; }Then use it in
CodingAgent:- const internalService = (llmService as any).koogService; + const internalService = llmService.getInternalService();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
mpp-vscode/README.md(1 hunks)mpp-vscode/package.json(1 hunks)mpp-vscode/src/bridge/mpp-core.ts(1 hunks)mpp-vscode/src/extension.ts(1 hunks)mpp-vscode/src/providers/chat-view.ts(1 hunks)mpp-vscode/src/services/diff-manager.ts(1 hunks)mpp-vscode/src/services/ide-server.ts(1 hunks)mpp-vscode/src/utils/logger.ts(1 hunks)mpp-vscode/test/bridge/mpp-core.test.ts(1 hunks)mpp-vscode/test/mocks/vscode.ts(1 hunks)mpp-vscode/test/services/diff-manager.test.ts(1 hunks)mpp-vscode/test/services/ide-server.test.ts(1 hunks)mpp-vscode/vitest.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mpp-vscode/README.md
🧰 Additional context used
🧠 Learnings (1)
📚 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: Build MPP Core with: `cd /Volumes/source/ai/autocrud && ./gradlew :mpp-core:assembleJsPackage`
Applied to files:
mpp-vscode/package.json
🧬 Code graph analysis (7)
mpp-vscode/test/services/ide-server.test.ts (1)
mpp-vscode/src/services/ide-server.ts (1)
IDEServer(23-250)
mpp-vscode/test/bridge/mpp-core.test.ts (1)
mpp-vscode/src/bridge/mpp-core.ts (8)
ProviderTypes(26-34)LLMService(59-142)CompletionManager(147-175)DevInsCompiler(189-215)ToolRegistry(227-307)CodingAgent(319-386)getAvailableModels(413-416)getAllProviders(421-423)
mpp-vscode/src/providers/chat-view.ts (1)
mpp-vscode/src/bridge/mpp-core.ts (2)
LLMService(59-142)ModelConfig(39-46)
mpp-vscode/src/extension.ts (4)
mpp-vscode/src/services/ide-server.ts (1)
IDEServer(23-250)mpp-vscode/src/utils/logger.ts (1)
createLogger(10-26)mpp-vscode/src/services/diff-manager.ts (2)
DiffContentProvider(14-38)DiffManager(61-261)mpp-vscode/src/providers/chat-view.ts (1)
ChatViewProvider(11-262)
mpp-vscode/src/services/ide-server.ts (1)
mpp-vscode/src/services/diff-manager.ts (1)
DiffManager(61-261)
mpp-vscode/src/services/diff-manager.ts (2)
mpp-vscode/test/mocks/vscode.ts (1)
Uri(5-18)mpp-vscode/src/extension.ts (1)
DIFF_SCHEME(13-13)
mpp-vscode/src/bridge/mpp-core.ts (1)
mpp-core/src/jsMain/kotlin/cc/unitmesh/llm/JsExports.kt (1)
providerName(168-197)
🔇 Additional comments (13)
mpp-vscode/src/utils/logger.ts (1)
10-26: LGTM!The logger implementation is clean and appropriate. It correctly checks the extension mode to conditionally log to the console in development, and formats messages with ISO timestamps for the output channel.
mpp-vscode/test/bridge/mpp-core.test.ts (1)
9-60: LGTM - comprehensive mock setup.The mock implementation for
@autodev/mpp-coreis well-structured and covers all the major exported classes and functions. This provides a solid foundation for testing the bridge layer.mpp-vscode/vitest.config.ts (1)
17-19: Verify vscode alias path resolves correctly.Line 18 uses a relative path
./test/mocks/vscode.tsfor the alias. Ensure this path is resolved correctly relative to the config file location (mpp-vscode/vitest.config.ts), which should make itmpp-vscode/test/mocks/vscode.ts. The path appears correct.mpp-vscode/src/extension.ts (4)
22-25: LGTM - proper logger initialization.The logger setup is clean: creates an OutputChannel, wraps it with the createLogger utility, and logs the activation message appropriately.
36-43: Verify intended behavior: cancel diff on document close.Lines 38-42 automatically cancel any diff when the corresponding document is closed. This means closing a diff tab discards changes without explicit user action. Ensure this UX is intentional—users might expect closing the document to preserve the diff state or prompt for confirmation.
Consider if this should be:
- Accepting the diff (apply changes)
- Prompting the user
- Just canceling (current behavior)
Based on typical diff UX patterns, you may want to prompt:
vscode.workspace.onDidCloseTextDocument(async (doc) => { if (doc.uri.scheme === DIFF_SCHEME) { const choice = await vscode.window.showWarningMessage( 'Diff closed. Apply changes?', 'Accept', 'Cancel' ); if (choice === 'Accept') { await diffManager.acceptDiff(doc.uri); } else { diffManager.cancelDiff(doc.uri); } } })
78-94: Good validation and user flow for run agent command.Lines 79-83 properly validate workspace folders before prompting, and line 91 forwards the input to the chat view. This is a clean integration between the command and the chat provider.
131-146: Excellent deactivation error handling.The deactivate function properly handles errors and ensures the logger is disposed in a finally block. This is robust and follows best practices.
mpp-vscode/test/services/ide-server.test.ts (1)
35-39: Fix mockDiffManager.onDidChange to match the actual DiffManager interface.The mock's
onDidChange: { dispose: () => {} }does not match the realDiffManager.onDidChange, which is avscode.Event<DiffEvent>. Avscode.Eventis a callable event stream, not a disposable object. Update the mock to reflect the actual event interface pattern so the mock accurately represents the interface, preventing issues if future code subscribes to this event.Likely an incorrect or invalid review comment.
mpp-vscode/src/services/ide-server.ts (1)
45-79: Security middleware looks solid.The combination of CORS (denying browser-origin requests), host validation (localhost only), and Bearer token authentication provides reasonable protection for this local server. The per-session random token written to a permissions-restricted file is a good pattern.
mpp-vscode/src/services/diff-manager.ts (2)
86-129: Well-structured diff view creation.Good handling of both existing and new files using the
untitledscheme fallback. The random query parameter for cache-busting is appropriate.
241-260: Tab closing logic is sound.The iteration through tab groups to find the diff tab by matching the modified URI is correct. Early return after closing prevents unnecessary iteration.
mpp-vscode/src/bridge/mpp-core.ts (2)
59-77: LLMService initialization and wrapper looks good.Clean wrapping of the Kotlin/JS
JsKoogLLMServicewith appropriate defaults for optional parameters. The provider name normalization logic is clear.
26-34: No action needed. The'custom-openai-base': 'CUSTOM_OPENAI_BASE'mapping in ProviderTypes is correct and fully supported by mpp-core. TheJsExports.ktexplicitly recognizes both"CUSTOM-OPENAI-BASE"and"CUSTOM_OPENAI_BASE"(line 49), and the Kotlin codebase has complete support including enum definition, executor factory handling, and test coverage.Likely an incorrect or invalid review comment.
| await this.koogService.streamPrompt( | ||
| message, | ||
| historyMessages, | ||
| (chunk: string) => { | ||
| fullResponse += chunk; | ||
| onChunk(chunk); | ||
| }, | ||
| (error: any) => { | ||
| throw new Error(`LLM Error: ${error.message || error}`); | ||
| }, | ||
| () => { /* complete */ } | ||
| ); | ||
|
|
||
| this.chatHistory.push({ role: 'assistant', content: fullResponse }); | ||
| return fullResponse; | ||
| } |
There was a problem hiding this comment.
Error thrown in callback may not propagate correctly.
Throwing an error inside the onError callback passed to streamPrompt may not reject the outer promise, depending on how the Kotlin/JS implementation handles callbacks. This could result in the error being swallowed.
Consider wrapping in a Promise and explicitly rejecting on error:
async streamMessage(
message: string,
onChunk: (chunk: string) => void
): Promise<string> {
this.chatHistory.push({ role: 'user', content: message });
const historyMessages = this.chatHistory.slice(0, -1).map(msg =>
new JsMessage(msg.role, msg.content)
);
let fullResponse = '';
+ let streamError: Error | null = null;
await this.koogService.streamPrompt(
message,
historyMessages,
(chunk: string) => {
fullResponse += chunk;
onChunk(chunk);
},
(error: any) => {
- throw new Error(`LLM Error: ${error.message || error}`);
+ streamError = new Error(`LLM Error: ${error.message || error}`);
},
() => { /* complete */ }
);
+ if (streamError) {
+ throw streamError;
+ }
+
this.chatHistory.push({ role: 'assistant', content: fullResponse });
return fullResponse;
}📝 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.
| await this.koogService.streamPrompt( | |
| message, | |
| historyMessages, | |
| (chunk: string) => { | |
| fullResponse += chunk; | |
| onChunk(chunk); | |
| }, | |
| (error: any) => { | |
| throw new Error(`LLM Error: ${error.message || error}`); | |
| }, | |
| () => { /* complete */ } | |
| ); | |
| this.chatHistory.push({ role: 'assistant', content: fullResponse }); | |
| return fullResponse; | |
| } | |
| async streamMessage( | |
| message: string, | |
| onChunk: (chunk: string) => void | |
| ): Promise<string> { | |
| this.chatHistory.push({ role: 'user', content: message }); | |
| const historyMessages = this.chatHistory.slice(0, -1).map(msg => | |
| new JsMessage(msg.role, msg.content) | |
| ); | |
| let fullResponse = ''; | |
| let streamError: Error | null = null; | |
| await this.koogService.streamPrompt( | |
| message, | |
| historyMessages, | |
| (chunk: string) => { | |
| fullResponse += chunk; | |
| onChunk(chunk); | |
| }, | |
| (error: any) => { | |
| streamError = new Error(`LLM Error: ${error.message || error}`); | |
| }, | |
| () => { /* complete */ } | |
| ); | |
| if (streamError) { | |
| throw streamError; | |
| } | |
| this.chatHistory.push({ role: 'assistant', content: fullResponse }); | |
| return fullResponse; | |
| } |
🤖 Prompt for AI Agents
mpp-vscode/src/bridge/mpp-core.ts around lines 93-108: the onError callback
passed to streamPrompt currently throws inside the callback which may not reject
the outer async flow; wrap the streamPrompt interaction in a new Promise that
rejects when the onError callback is invoked and resolves when the completion
callback runs, collect chunks into fullResponse within onChunk, await that
Promise, then push to chatHistory and return fullResponse; ensure the Promise
rejects with the error message (error.message || error) so callers receive a
proper rejection.
| // Read file endpoint | ||
| app.post('/file/read', async (req: Request, res: Response) => { | ||
| try { | ||
| const { filePath } = req.body; | ||
| if (!filePath) { | ||
| return res.status(400).json({ error: 'filePath is required' }); | ||
| } | ||
|
|
||
| const uri = vscode.Uri.file(filePath); | ||
| const content = await vscode.workspace.fs.readFile(uri); | ||
| res.json({ success: true, content: Buffer.from(content).toString('utf8') }); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| res.status(500).json({ error: message }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Path traversal risk: file read endpoint accepts arbitrary paths.
The /file/read endpoint accepts any filePath without validating it's within the workspace. An attacker with the auth token could read sensitive files outside the workspace (e.g., ~/.ssh/id_rsa). Consider validating that the resolved path is within workspace boundaries.
app.post('/file/read', async (req: Request, res: Response) => {
try {
const { filePath } = req.body;
if (!filePath) {
return res.status(400).json({ error: 'filePath is required' });
}
+
+ // Validate path is within workspace
+ const workspaceFolders = vscode.workspace.workspaceFolders;
+ if (!workspaceFolders || workspaceFolders.length === 0) {
+ return res.status(400).json({ error: 'No workspace open' });
+ }
+
+ const resolvedPath = path.resolve(filePath);
+ const isWithinWorkspace = workspaceFolders.some(folder =>
+ resolvedPath.startsWith(folder.uri.fsPath + path.sep) || resolvedPath === folder.uri.fsPath
+ );
+ if (!isWithinWorkspace) {
+ return res.status(403).json({ error: 'Path outside workspace' });
+ }
const uri = vscode.Uri.file(filePath);Committable suggestion skipped: line range outside the PR's diff.
| // Write file endpoint | ||
| app.post('/file/write', async (req: Request, res: Response) => { | ||
| try { | ||
| const { filePath, content } = req.body; | ||
| if (!filePath || content === undefined) { | ||
| return res.status(400).json({ error: 'filePath and content are required' }); | ||
| } | ||
|
|
||
| const uri = vscode.Uri.file(filePath); | ||
| await vscode.workspace.fs.writeFile(uri, Buffer.from(content, 'utf8')); | ||
| res.json({ success: true }); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| res.status(500).json({ error: message }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Path traversal risk: file write endpoint accepts arbitrary paths.
Same concern as /file/read — arbitrary file writes outside the workspace could overwrite system files or inject malicious content. Apply the same workspace boundary validation.
| /** | ||
| * Stop the IDE server | ||
| */ | ||
| async stop(): Promise<void> { | ||
| if (this.server) { | ||
| return new Promise((resolve) => { | ||
| this.server!.close(() => { | ||
| this.log('IDE Server stopped'); | ||
| resolve(); | ||
| }); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Port file not cleaned up on stop.
The port file (~/.autodev/ide-server.json) is written on start but not removed on stop. Stale port files could mislead external tools into connecting to a non-existent server. Consider deleting the file in stop().
async stop(): Promise<void> {
if (this.server) {
return new Promise((resolve) => {
- this.server!.close(() => {
+ this.server!.close(async () => {
+ // Clean up port file
+ if (this.portFile) {
+ try {
+ await fs.unlink(this.portFile);
+ this.log('Port file removed');
+ } catch {
+ // Ignore if file doesn't exist
+ }
+ }
this.log('IDE Server stopped');
resolve();
});
});
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mpp-vscode/src/services/ide-server.ts around lines 191 to 203, the stop()
method currently closes the server but does not remove the port file
(~/.autodev/ide-server.json), leaving stale files behind; update stop() to
delete that file when the server is successfully closed (use fs.promises.unlink
or fs.unlink with a callback), handle ENOENT gracefully if the file is already
missing, catch and log any unlink errors without preventing shutdown, and ensure
the file removal completes (or is attempted) before resolving the returned
Promise so external tools won't be misled by a stale port file.
| parse: (value: string) => { | ||
| const url = new URL(value); | ||
| return { scheme: url.protocol.replace(':', ''), path: url.pathname, fsPath: url.pathname, toString: () => value }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "\.(ts|js)$" | head -30Repository: phodal/auto-dev-sketch
Length of output: 1650
🏁 Script executed:
rg "Uri\.parse" --type ts -B 2 -A 2Repository: phodal/auto-dev-sketch
Length of output: 462
🏁 Script executed:
rg "autodev-diff|custom.*scheme" --type tsRepository: phodal/auto-dev-sketch
Length of output: 735
🏁 Script executed:
cat -n mpp-vscode/test/mocks/vscode.tsRepository: phodal/auto-dev-sketch
Length of output: 3238
🏁 Script executed:
rg "Uri\.parse.*autodev-diff|parse.*autodev" --type ts -B 3 -A 3Repository: phodal/auto-dev-sketch
Length of output: 48
🏁 Script executed:
rg "diffDocuments" mpp-vscode/src/services/diff-manager.ts -B 2 -A 2Repository: phodal/auto-dev-sketch
Length of output: 1866
🏁 Script executed:
cat -n mpp-vscode/src/services/diff-manager.ts | head -80Repository: phodal/auto-dev-sketch
Length of output: 2629
🏁 Script executed:
cat -n mpp-vscode/test/services/diff-manager.test.ts | head -150Repository: phodal/auto-dev-sketch
Length of output: 6484
🏁 Script executed:
rg "closeDiffEditor" mpp-vscode/test/services/diff-manager.test.ts -B 3 -A 5Repository: phodal/auto-dev-sketch
Length of output: 48
🏁 Script executed:
rg "closeDiffEditor|closeAllDiffs" mpp-vscode/test/services/diff-manager.test.tsRepository: phodal/auto-dev-sketch
Length of output: 48
🏁 Script executed:
cat -n mpp-vscode/src/services/diff-manager.ts | sed -n '95,120p'Repository: phodal/auto-dev-sketch
Length of output: 974
Handle custom URI schemes in Uri.parse mock.
Line 15 uses new URL(value) which throws for non-standard schemes like autodev-diff:// used in diff-manager tests. Add error handling to parse custom schemes correctly:
parse: (value: string) => {
try {
const url = new URL(value);
return { scheme: url.protocol.replace(':', ''), path: url.pathname, fsPath: url.pathname, toString: () => value };
} catch {
// Fallback for non-standard schemes
const match = value.match(/^([^:]+):\/\/(.+)$/);
const scheme = match?.[1] || 'file';
const path = match?.[2] || value;
return { scheme, path, fsPath: path, toString: () => value };
}
}This ensures the mock handles custom schemes used in diff-manager, which stores URIs with autodev-diff:// scheme and later calls Uri.parse() on their string representations.
📝 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.
| parse: (value: string) => { | |
| const url = new URL(value); | |
| return { scheme: url.protocol.replace(':', ''), path: url.pathname, fsPath: url.pathname, toString: () => value }; | |
| } | |
| parse: (value: string) => { | |
| try { | |
| const url = new URL(value); | |
| return { scheme: url.protocol.replace(':', ''), path: url.pathname, fsPath: url.pathname, toString: () => value }; | |
| } catch { | |
| // Fallback for non-standard schemes | |
| const match = value.match(/^([^:]+):\/\/(.+)$/); | |
| const scheme = match?.[1] || 'file'; | |
| const path = match?.[2] || value; | |
| return { scheme, path, fsPath: path, toString: () => value }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In mpp-vscode/test/mocks/vscode.ts around lines 14 to 17, the Uri.parse mock
currently uses new URL(value) which throws for custom schemes (e.g.
autodev-diff://); update the mock to catch URL construction errors and handle
custom schemes by parsing the string manually: detect a scheme prefix before
'://', extract scheme and path (everything after '://'), and return the same
shape { scheme, path, fsPath: path, toString: () => value } so tests using
non-standard URIs don't throw.
Phase 5 - Status Bar: - Add StatusBarManager with idle/thinking/streaming/error states - Animated status icons during LLM operations - Click to open chat command Phase 6 - DevIns Language Support: - Add TextMate grammar for syntax highlighting - Support commands (/), agents (@), variables ($) - Add language configuration (brackets, folding, etc.) - Implement DevInsCompletionProvider with built-in completions - Register completion triggers for /, @, $ characters Testing: - Add unit tests for StatusBarManager - Add unit tests for DevInsCompletionProvider - All 56 tests passing Refs #31
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mpp-vscode/src/extension.ts (1)
128-134: Consider platform-agnostic keybinding description in welcome message.The welcome message hardcodes "Cmd+Shift+A" which is Mac-specific. On Windows/Linux, this would be "Ctrl+Shift+A". Consider using a platform-agnostic description or detecting the platform.
Apply this diff for a platform-agnostic message:
vscode.window.showInformationMessage( - 'AutoDev extension installed successfully! Press Cmd+Shift+A to open chat.' + 'AutoDev extension installed successfully! Use the command palette to open chat.' );Or detect the platform:
+ const isMac = process.platform === 'darwin'; + const keyBinding = isMac ? 'Cmd+Shift+A' : 'Ctrl+Shift+A'; vscode.window.showInformationMessage( - 'AutoDev extension installed successfully! Press Cmd+Shift+A to open chat.' + `AutoDev extension installed successfully! Press ${keyBinding} to open chat.` );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mpp-vscode/resources/icon.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
mpp-vscode/README.md(1 hunks)mpp-vscode/package.json(1 hunks)mpp-vscode/src/extension.ts(1 hunks)mpp-vscode/src/providers/devins-completion.ts(1 hunks)mpp-vscode/src/services/status-bar.ts(1 hunks)mpp-vscode/syntaxes/DevIns.tmLanguage.json(1 hunks)mpp-vscode/syntaxes/language-configuration.json(1 hunks)mpp-vscode/test/providers/devins-completion.test.ts(1 hunks)mpp-vscode/test/services/status-bar.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mpp-vscode/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 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: Build MPP Core with: `cd /Volumes/source/ai/autocrud && ./gradlew :mpp-core:assembleJsPackage`
Applied to files:
mpp-vscode/README.md
🧬 Code graph analysis (3)
mpp-vscode/test/providers/devins-completion.test.ts (3)
core/src/main/kotlin/cc/unitmesh/devti/settings/locale/LanguageChangedCallback.kt (1)
label(36-42)mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/document/docql/DocQLParser.kt (1)
expect(260-265)mpp-vscode/src/providers/devins-completion.ts (2)
DevInsCompletionProvider(52-140)registerDevInsCompletionProvider(145-155)
mpp-vscode/src/providers/devins-completion.ts (2)
mpp-vscode/src/bridge/mpp-core.ts (2)
CompletionManager(147-175)CompletionItem(177-184)core/src/main/kotlin/cc/unitmesh/devti/a2a/AgentRequest.kt (1)
agent(8-12)
mpp-vscode/test/services/status-bar.test.ts (1)
mpp-vscode/src/services/status-bar.ts (1)
StatusBarManager(12-127)
🔇 Additional comments (10)
mpp-vscode/syntaxes/language-configuration.json (1)
1-42: LGTM!The language configuration is well-structured with appropriate settings for a markdown-style language including comment definitions, bracket pairs, auto-closing behavior, folding markers, and Unicode-aware word patterns.
mpp-vscode/syntaxes/DevIns.tmLanguage.json (1)
1-172: LGTM!The TextMate grammar comprehensively defines DevIns language syntax with proper support for YAML frontmatter, custom commands, agents, variables, and markdown constructs. The recursive includes for nested structures (blockquotes, lists) are correctly configured.
mpp-vscode/src/providers/devins-completion.ts (3)
90-105: Verify completion insertion behavior with partial prefixes.The
insertTextusessubstring(1)to remove the trigger character (/,@,$) from the completion. This works correctly when the user has only typed the trigger character. However, if a user has already typed a partial prefix (e.g.,/fi), the VSCode completion API should replace the entire word. Please verify that the completion behavior is correct in both scenarios:
- User types
/→ selects/file→ result should be/file:path- User types
/fi→ selects/file→ result should be/file:path(not/fifile:path)This same logic applies to lines 118 (agents) and 135 (variables).
Test the completion behavior manually to confirm the insertText replacement works as expected with partial prefixes.
55-61: LGTM!The graceful fallback when mpp-core is unavailable is well-designed. The provider can operate with built-in completions even if the Kotlin/JS bridge isn't available.
63-88: LGTM!The completion item provider correctly identifies trigger characters and delegates to specialized helper methods. The logic is clear and handles all three trigger types (commands, agents, variables).
mpp-vscode/test/providers/devins-completion.test.ts (1)
1-160: LGTM!The test suite provides solid coverage of the DevInsCompletionProvider functionality, including trigger character handling, mock setup, and registration behavior. The tests appropriately verify that completions are provided for trigger characters and empty for regular text.
mpp-vscode/src/services/status-bar.ts (1)
112-126: LGTM!Animation handling is correctly implemented with proper cleanup. The 500ms interval provides smooth visual feedback, and
stopAnimation()ensures no interval leaks.mpp-vscode/test/services/status-bar.test.ts (1)
1-99: LGTM!The test suite appropriately validates StatusBarManager functionality, including state transitions and proper disposal. The cleanup calls (lines 71, 78) demonstrate good practice by disposing animation intervals.
mpp-vscode/src/extension.ts (2)
108-114: LGTM!Error handling for IDE server initialization is properly implemented with clear error messages logged to the output channel.
142-157: LGTM!The deactivation function properly cleans up resources with appropriate error handling and ensures the logger is disposed in the finally block.
| ```bash | ||
| cd /Volumes/source/ai/autocrud | ||
| ./gradlew :mpp-core:assembleJsPackage | ||
| ``` |
There was a problem hiding this comment.
Replace hardcoded absolute path with relative or environment-based path.
The build command uses a hardcoded absolute path /Volumes/source/ai/autocrud that won't work for other contributors. Replace this with a relative path from the repository root or use an environment variable (e.g., $AUTODEV_ROOT or $PWD/..) to make the documentation portable.
Apply this diff to use a relative path:
-cd /Volumes/source/ai/autocrud
-./gradlew :mpp-core:assembleJsPackage
+cd ../.. # Navigate to repository root, or use: cd $(git rev-parse --show-toplevel)
+./gradlew :mpp-core:assembleJsPackageBased on learnings, the original path is correct for the current developer environment but should be generalized in documentation.
📝 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.
| ```bash | |
| cd /Volumes/source/ai/autocrud | |
| ./gradlew :mpp-core:assembleJsPackage | |
| ``` |
| ```bash | |
| cd /Volumes/source/ai/autocrud | |
| ./gradlew :mpp-core:assembleJsPackage | |
| ``` |
🤖 Prompt for AI Agents
mpp-vscode/README.md lines 104-107: the example build commands use a hardcoded
absolute path (/Volumes/source/ai/autocrud); change these to a relative
repo-root invocation or use an environment variable so other contributors can
run it. Update the commands to run from the repo root (e.g., cd to the
repository root using a relative path like . or .. as appropriate) or reference
an environment variable such as $AUTODEV_ROOT or $PWD to construct the path, and
ensure the README instructs users to set the env var if used.
Phase 6 - React Webview UI: - Create webview React project with Vite build - Add MessageList component with Markdown rendering - Add ChatInput component with auto-resize textarea - Add useVSCode hook for extension communication - Integrate VSCode theme variables for consistent styling - Support streaming response with animated indicators - Add fallback inline HTML when React bundle not available - Update ChatViewProvider to load React bundle with CSP Components: - App.tsx - Main chat application - MessageList.tsx - Message display with Markdown support - ChatInput.tsx - Input with keyboard shortcuts - useVSCode.ts - VSCode API communication hook Testing: - Add ChatViewProvider tests - All 63 tests passing Refs #31
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
mpp-vscode/package.json (4)
10-10: Repository URL points to wrong repository.Line 10 references
phodal/auto-devbut this project lives inphodal/auto-dev-sketch; the incorrect URL will surface broken metadata in the VSCode Marketplace and npm registry.- "url": "https://github.com/phodal/auto-dev" + "url": "https://github.com/phodal/auto-dev-sketch"
54-54: Add the missing icon asset or update the reference.Line 54 references
resources/icon.svgfor the activity bar icon, but the file does not exist in the mpp-vscode directory. VSCode will fail to render the icon andvsce packagewill warn or fail. Either add the asset or update the path to an existing icon.
81-85: Avoid storing API keys as plain-text settings.Line 81 defines
autodev.apiKeyas a standard string setting, which stores the value insettings.json(user-synced and not encrypted). Use VS Code's SecretStorage API instead, and collect the key via a command at runtime.Remove this setting and implement key retrieval via SecretStorage in the extension's activation logic (e.g., in
extension.ts).
150-150: Resolve brittle local file path dependency.Line 150 uses a relative file path (
file:../mpp-core/build/packages/js) to reference@autodev/mpp-core. This pattern is brittle for packaging and publishing withvsce:
- The build output path must exist after running
./gradlew :mpp-core:assembleJsPackage(manual step, not automated)- mpp-vscode has no CI/CD automation to manage this dependency
- Unlike mpp-ui (which has npm publish automation), mpp-vscode lacks workflow integration
- Future mpp-core build restructuring will break this reference
Consider either:
- Publishing
@autodev/mpp-coreto npm and using a version specifier (e.g.,^0.1.0)- Adding mpp-vscode to a npm/workspace-based publish workflow
- Integrating mpp-vscode into CI/CD to handle the build path transformation
| resolveWebviewView( | ||
| webviewView: vscode.WebviewView, | ||
| _context: vscode.WebviewViewResolveContext, | ||
| _token: vscode.CancellationToken | ||
| ): void { | ||
| this.webviewView = webviewView; | ||
|
|
||
| webviewView.webview.options = { | ||
| enableScripts: true, | ||
| localResourceRoots: [ | ||
| vscode.Uri.joinPath(this.context.extensionUri, 'dist', 'webview') | ||
| ] | ||
| }; | ||
|
|
||
| webviewView.webview.html = this.getHtmlContent(webviewView.webview); | ||
|
|
||
| // Handle messages from webview | ||
| webviewView.webview.onDidReceiveMessage(async (message) => { | ||
| switch (message.type) { | ||
| case 'sendMessage': | ||
| await this.handleUserMessage(message.content); | ||
| break; | ||
| case 'clearHistory': | ||
| this.clearHistory(); | ||
| break; | ||
| } | ||
| }); | ||
|
|
||
| // Initialize LLM service | ||
| this.initializeLLMService(); | ||
| } |
There was a problem hiding this comment.
Track disposables to prevent resource leaks.
The onDidReceiveMessage listener (line 39) returns a Disposable that should be tracked and disposed when the webview is destroyed. Without proper cleanup, event handlers can accumulate and cause memory leaks.
Consider adding disposal tracking:
export class ChatViewProvider implements vscode.WebviewViewProvider {
private webviewView: vscode.WebviewView | undefined;
private llmService: LLMService | undefined;
private messages: Array<{ role: string; content: string }> = [];
+ private disposables: vscode.Disposable[] = [];
resolveWebviewView(
webviewView: vscode.WebviewView,
_context: vscode.WebviewViewResolveContext,
_token: vscode.CancellationToken
): void {
this.webviewView = webviewView;
webviewView.webview.options = {
enableScripts: true,
localResourceRoots: [
vscode.Uri.joinPath(this.context.extensionUri, 'dist', 'webview')
]
};
webviewView.webview.html = this.getHtmlContent(webviewView.webview);
// Handle messages from webview
- webviewView.webview.onDidReceiveMessage(async (message) => {
+ this.disposables.push(webviewView.webview.onDidReceiveMessage(async (message) => {
switch (message.type) {
case 'sendMessage':
await this.handleUserMessage(message.content);
break;
case 'clearHistory':
this.clearHistory();
break;
}
- });
+ }));
+
+ // Clean up when webview is disposed
+ this.disposables.push(webviewView.onDidDispose(() => {
+ this.disposables.forEach(d => d.dispose());
+ this.disposables = [];
+ this.webviewView = undefined;
+ }));
// Initialize LLM service
this.initializeLLMService();
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
mpp-vscode/src/providers/chat-view.ts around lines 22 to 52: the
onDidReceiveMessage call returns a Disposable that isn’t tracked and thus leaks
when the webview is recreated; capture the returned Disposable, add it to the
class disposable collection (e.g. this.disposables or a webview-specific array)
and ensure you dispose those disposables when the webview is disposed (listen
for webviewView.onDidDispose and call dispose on tracked disposables or clear
the array). Ensure any existing dispose helper is used or created so every time
resolveWebviewView runs you register and later clean up the message listener.
| async sendMessage(content: string): Promise<void> { | ||
| if (this.webviewView) { | ||
| // Show the chat view | ||
| this.webviewView.show(true); | ||
| } | ||
| await this.handleUserMessage(content); | ||
| } |
There was a problem hiding this comment.
Guard against undefined webview before handling messages.
The method shows the webview if it exists (line 60), but then unconditionally calls handleUserMessage (line 62) which will attempt to post messages via this.webviewView?.webview.postMessage. If the webview hasn't been resolved yet, the message handling will silently fail.
Consider adding a guard:
async sendMessage(content: string): Promise<void> {
- if (this.webviewView) {
- // Show the chat view
- this.webviewView.show(true);
+ if (!this.webviewView) {
+ this.log('Cannot send message: webview not initialized');
+ return;
}
+
+ // Show the chat view
+ this.webviewView.show(true);
await this.handleUserMessage(content);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In mpp-vscode/src/providers/chat-view.ts around lines 57–63, sendMessage
currently calls handleUserMessage unconditionally even if this.webviewView or
its .webview is undefined; update sendMessage to guard that the webview is
present before calling handleUserMessage—check if this.webviewView?.webview
exists and if not call this.webviewView?.show(true) and then either return early
(so the caller can retry) or enqueue the message for delivery once the webview
is ready; ensure you do not attempt to postMessage when .webview is undefined.
| "dependencies": { | ||
| "react": "^18.2.0", | ||
| "react-dom": "^18.2.0", | ||
| "react-markdown": "^9.0.1", | ||
| "remark-gfm": "^4.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/react": "^18.2.0", | ||
| "@types/react-dom": "^18.2.0", | ||
| "@vitejs/plugin-react": "^4.2.0", | ||
| "typescript": "^5.3.0", | ||
| "vite": "^5.0.0" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest stable versions React TypeScript Vite December 2025
💡 Result:
As of Dec 2, 2025:
- React: 19.2 (released Oct 1, 2025). [1]
- TypeScript: 5.9 (stable release, announced Aug 2025). [2]
- Vite: 7.1.4 (latest npm published release). [3]
Sources: [1] react.dev blog; [2] TypeScript blog (Microsoft); [3] vite on npm.
Update dependencies to latest stable versions.
As of December 2025, several dependencies are outdated:
- React: update from ^18.2.0 to ^19.2.0 (released October 2025)
- TypeScript: update from ^5.3.0 to ^5.9 (available since August 2025)
- Vite: update from ^5.0.0 to ^7.1.4 (latest stable)
Updating to current versions provides access to bug fixes, security patches, and new features.
Extract MarkdownTextParser to a shared mpp-core module for reuse and improved testability across platforms. Update imports accordingly.
There was a problem hiding this comment.
Pull request overview
This PR initializes the mpp-vscode project, a VSCode extension that leverages Kotlin Multiplatform (KMP) technology by reusing the mpp-core library. The extension provides an AI-powered coding assistant with chat interface, diff management, DevIns language support, and an MCP protocol server for external tool integration.
Key Changes
- Complete VSCode extension infrastructure: package.json with commands, views, configuration settings, and keybindings; TypeScript configuration with separate configs for extension and webview; build tooling with esbuild and Vite; testing framework with Vitest and comprehensive mocks
- Core services and integrations: TypeScript bridge to mpp-core Kotlin/JS library providing LLM service, coding agent, and tool registry; IDE server implementing MCP protocol with REST endpoints for diff operations and file access; diff manager for showing/accepting/canceling code changes; status bar integration with animated states
- UI components: React-based webview chat interface with streaming responses and markdown rendering; DevIns language support including syntax highlighting and auto-completion for commands (@), agents ($), and variables (/); inline fallback HTML UI for development
Reviewed changes
Copilot reviewed 39 out of 40 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| mpp-vscode/package.json | VSCode extension manifest with commands, views, configuration, and dependencies |
| mpp-vscode/tsconfig.json | TypeScript configuration for extension code (CommonJS, Node target) |
| mpp-vscode/vitest.config.ts | Test framework configuration with vscode mock aliasing |
| mpp-vscode/src/extension.ts | Extension entry point with activation, service initialization, and command registration |
| mpp-vscode/src/bridge/mpp-core.ts | TypeScript wrapper for Kotlin/JS mpp-core module providing LLM, agent, and tool APIs |
| mpp-vscode/src/services/ide-server.ts | Express-based MCP server with auth, CORS, diff/file endpoints |
| mpp-vscode/src/services/diff-manager.ts | Manages diff view lifecycle with accept/cancel operations |
| mpp-vscode/src/services/status-bar.ts | Status bar manager with animated state indicators |
| mpp-vscode/src/providers/chat-view.ts | Webview provider for chat interface with LLM streaming |
| mpp-vscode/src/providers/devins-completion.ts | DevIns language completion provider for commands, agents, variables |
| mpp-vscode/src/utils/logger.ts | Logging utility for output channel and console |
| mpp-vscode/webview/package.json | Webview React app dependencies and build scripts |
| mpp-vscode/webview/tsconfig.json | TypeScript config for webview (ESNext, React JSX, DOM) |
| mpp-vscode/webview/vite.config.ts | Vite build configuration for webview React app |
| mpp-vscode/webview/src/main.tsx | React app entry point |
| mpp-vscode/webview/src/App.tsx | Main chat application component with message handling |
| mpp-vscode/webview/src/hooks/useVSCode.ts | React hook for VSCode API communication |
| mpp-vscode/webview/src/components/MessageList.tsx | Chat message list with markdown rendering |
| mpp-vscode/webview/src/components/ChatInput.tsx | Chat input component with auto-resize and keyboard shortcuts |
| mpp-vscode/webview/src/styles/index.css | Global styles with VSCode theme integration |
| mpp-vscode/syntaxes/DevIns.tmLanguage.json | TextMate grammar for DevIns language syntax highlighting |
| mpp-vscode/syntaxes/language-configuration.json | Language configuration for DevIns (brackets, comments, folding) |
| mpp-vscode/test/mocks/vscode.ts | Mock implementation of VSCode API for testing |
| mpp-vscode/test/bridge/mpp-core.test.ts | Unit tests for mpp-core bridge layer |
| mpp-vscode/test/services/*.test.ts | Unit tests for IDE server, diff manager, status bar |
| mpp-vscode/test/providers/*.test.ts | Unit tests for chat view and DevIns completion providers |
| mpp-vscode/resources/icon.svg | Extension icon for activity bar |
| mpp-vscode/README.md | Project documentation with architecture, TODO list, and development guide |
| mpp-core/src/commonMain/kotlin/cc/unitmesh/markdown/MarkdownTextParser.kt | Moved from mpp-idea package to common module for reusability |
| mpp-idea/src/main/kotlin/.../markdown/*.kt | Updated imports to use new MarkdownTextParser package |
Comments suppressed due to low confidence (2)
mpp-core/src/commonMain/kotlin/cc/unitmesh/markdown/MarkdownTextParser.kt:1
- The package declaration has been changed from
cc.unitmesh.devins.idea.renderer.markdowntocc.unitmesh.markdown. While this makes the class more reusable, the old package path suggests this was IntelliJ IDEA-specific code. Ensure that all references in mpp-idea have been updated to import from the new package (which appears to be done correctly in the provided diffs).
mpp-vscode/src/services/status-bar.ts:73 - Unused variable colors.
const colors: Record<StatusBarState, string | undefined> = {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <head> | ||
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <meta http-equiv="Content-Security-Policy" content="default-src 'none'; style-src ${webview.cspSource} 'unsafe-inline'; script-src 'nonce-${nonce}';"> |
There was a problem hiding this comment.
The Content Security Policy (CSP) includes 'unsafe-inline' for styles, which is a security concern. Consider using nonces or hashes for inline styles, or move all styles to external stylesheets referenced via the styleUri.
| private async writePortFile(): Promise<void> { | ||
| const autodevDir = path.join(os.homedir(), '.autodev'); | ||
| this.portFile = path.join(autodevDir, 'ide-server.json'); | ||
|
|
||
| const content = JSON.stringify({ | ||
| port: this.port, | ||
| authToken: this.authToken, | ||
| workspacePath: vscode.workspace.workspaceFolders?.[0]?.uri.fsPath ?? '', | ||
| pid: process.pid | ||
| }); | ||
|
|
||
| try { | ||
| await fs.mkdir(autodevDir, { recursive: true }); | ||
| await fs.writeFile(this.portFile, content); | ||
| await fs.chmod(this.portFile, 0o600); | ||
| this.log(`Port file written to: ${this.portFile}`); |
There was a problem hiding this comment.
The port file is written with mode 0o600 (read/write for owner only), but the auth token is stored in plaintext in this file at ~/.autodev/ide-server.json. Consider encrypting the token or using a more secure storage mechanism, especially since this file could be accessed by other processes running as the same user.
| "autodev.apiKey": { | ||
| "type": "string", | ||
| "default": "", | ||
| "description": "API key for the LLM provider" |
There was a problem hiding this comment.
The API key is stored in VSCode settings with default: "". This could lead to unclear error messages when users haven't configured it. Consider adding a validation check on extension activation to prompt users to configure the API key if it's empty, or provide clearer setup instructions in the welcome message.
| "description": "API key for the LLM provider" | |
| "description": "Required: Enter your API key for the selected LLM provider. The extension will not work without a valid API key.", | |
| "markdownDescription": "Enter your API key for the selected LLM provider. **You must set this value for AutoDev to function.**\n\nTo obtain an API key, visit your provider's website (e.g., [OpenAI](https://platform.openai.com/account/api-keys)).\n\nIf this field is left empty, you may encounter unclear error messages or failed requests." |
| "key": "ctrl+shift+a", | ||
| "mac": "cmd+shift+a" |
There was a problem hiding this comment.
[nitpick] The keybinding uses "ctrl+shift+a" for Windows/Linux, but "a" is a very common key that might conflict with other extensions or user workflows. Consider using a less common key combination to reduce the risk of conflicts.
| "key": "ctrl+shift+a", | |
| "mac": "cmd+shift+a" | |
| "key": "ctrl+shift+alt+c", | |
| "mac": "cmd+shift+alt+c" |
| const colors: Record<StatusBarState, string | undefined> = { | ||
| idle: undefined, | ||
| thinking: new vscode.ThemeColor('statusBarItem.warningForeground').toString(), | ||
| streaming: new vscode.ThemeColor('statusBarItem.prominentForeground').toString(), | ||
| error: new vscode.ThemeColor('statusBarItem.errorForeground').toString() |
There was a problem hiding this comment.
The colors object assigns ThemeColor.toString() values to the colors, but toString() on ThemeColor doesn't return a usable color value. The ThemeColor instances should be assigned directly without calling toString().
| const colors: Record<StatusBarState, string | undefined> = { | |
| idle: undefined, | |
| thinking: new vscode.ThemeColor('statusBarItem.warningForeground').toString(), | |
| streaming: new vscode.ThemeColor('statusBarItem.prominentForeground').toString(), | |
| error: new vscode.ThemeColor('statusBarItem.errorForeground').toString() | |
| const colors: Record<StatusBarState, vscode.ThemeColor | undefined> = { | |
| idle: undefined, | |
| thinking: new vscode.ThemeColor('statusBarItem.warningForeground'), | |
| streaming: new vscode.ThemeColor('statusBarItem.prominentForeground'), | |
| error: new vscode.ThemeColor('statusBarItem.errorForeground') |
| "vitest": "^1.0.0" | ||
| }, | ||
| "dependencies": { | ||
| "@autodev/mpp-core": "file:../mpp-core/build/packages/js", |
There was a problem hiding this comment.
The package dependency @autodev/mpp-core uses file:../mpp-core/build/packages/js, which is a local file path. This will not work when publishing the extension. Consider publishing mpp-core to npm or bundling it differently for distribution.
| "@autodev/mpp-core": "file:../mpp-core/build/packages/js", | |
| "@autodev/mpp-core": "^1.0.0", |
| "pattern": "(\\p{Alphabetic}|\\p{Number}|\\p{Nonspacing_Mark})(((\\p{Alphabetic}|\\p{Number}|\\p{Nonspacing_Mark})|[_])?(\\p{Alphabetic}|\\p{Number}|\\p{Nonspacing_Mark}))*", | ||
| "flags": "ug" |
There was a problem hiding this comment.
[nitpick] In the language configuration, the wordPattern uses Unicode property escapes with the 'u' flag, which is good. However, the pattern might be overly complex. Consider testing this pattern with various DevIns syntax to ensure it works as expected.
| "pattern": "(\\p{Alphabetic}|\\p{Number}|\\p{Nonspacing_Mark})(((\\p{Alphabetic}|\\p{Number}|\\p{Nonspacing_Mark})|[_])?(\\p{Alphabetic}|\\p{Number}|\\p{Nonspacing_Mark}))*", | |
| "flags": "ug" | |
| "pattern": "\\p{L}[\\p{L}\\p{N}_]*", | |
| "flags": "u" |
| const result = await this.registry.shell(command, workingDirectory, timeoutMs); | ||
| return this.toToolResult(result); |
There was a problem hiding this comment.
The shell command execution has a default timeout of 30 seconds (30000ms), but there's no way for callers to know if the command failed due to timeout vs other errors. Consider including timeout information in the error message when a command times out.
| const result = await this.registry.shell(command, workingDirectory, timeoutMs); | |
| return this.toToolResult(result); | |
| try { | |
| const result = await this.registry.shell(command, workingDirectory, timeoutMs); | |
| return this.toToolResult(result); | |
| } catch (err: any) { | |
| // Detect timeout error by message or code if possible | |
| let isTimeout = false; | |
| let message = ''; | |
| if (err && typeof err === 'object') { | |
| if (typeof err.message === 'string' && ( | |
| err.message.includes('timed out') || | |
| err.message.includes('timeout') || | |
| (err.code && (err.code === 'ETIMEDOUT' || err.code === 'ETIMEOUT')) | |
| )) { | |
| isTimeout = true; | |
| } | |
| } | |
| if (isTimeout) { | |
| message = `Shell command timed out after ${timeoutMs}ms: ${command}`; | |
| } else if (err && typeof err.message === 'string') { | |
| message = err.message; | |
| } else { | |
| message = 'Shell command failed with unknown error'; | |
| } | |
| return this.toToolResult({ | |
| success: false, | |
| error: message, | |
| code: isTimeout ? 'TIMEOUT' : (err && err.code ? err.code : undefined), | |
| output: '', | |
| }); | |
| } |
| JsToolRegistry, | ||
| JsCompressionConfig |
There was a problem hiding this comment.
Unused variable JsCompressionConfig.
| JsToolRegistry, | |
| JsCompressionConfig | |
| JsToolRegistry |
| */ | ||
|
|
||
| import * as vscode from 'vscode'; | ||
| import * as path from 'path'; |
There was a problem hiding this comment.
Unused import path.
| import * as path from 'path'; |
Phase 7 - Sketch Renderer System: - Add Timeline component mirroring mpp-ui's ComposeRenderer - Add SketchRenderer with specialized sub-renderers: - CodeBlockRenderer: syntax highlighting with Copy/Insert/Apply actions - DiffRenderer: diff display with Accept/Reject/View actions - ThinkingRenderer: collapsible thinking blocks - TerminalRenderer: terminal command and output display - MarkdownRenderer: GFM markdown with react-markdown - ToolCallRenderer: tool call information display - Add codeFence parser mirroring mpp-core's CodeFence.parseAll() - Add timeline types (TimelineItem, AgentState, ToolCallInfo, etc.) ChatViewProvider Refactoring: - Replace LLMService with CodingAgent for agent-based interactions - Add VSCodeRenderer implementing JsCodingAgentRenderer interface - Forward agent events (toolCall, toolResult, terminalOutput) to webview - Add action handlers (insert, apply, run-command, accept-diff, etc.) - Remove API key requirement - use helpful guidance for non-DevIns input All 63 tests passing. Refs #31
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
mpp-vscode/src/bridge/mpp-core.ts (2)
19-20: Unused importJsCompressionConfig.
JsCompressionConfigis imported but never used in this file.
93-108: Error thrown inonErrorcallback may not propagate correctly.The error thrown inside the
onErrorcallback ofstreamPromptmay not reject the outer promise, depending on how the Kotlin/JS implementation handles callbacks. This was flagged in a previous review with a suggested fix to capture the error and throw afterawaitcompletes.mpp-vscode/src/providers/chat-view.ts (2)
43-55: Track disposables to prevent resource leaks.The
onDidReceiveMessagelistener returns aDisposablethat should be tracked and disposed when the webview is destroyed.
64-69: Guard against undefined webview before handling messages.If
this.webviewViewis undefined, thehandleUserMessagecall will proceed butpostMessagecalls inside will silently fail.
🧹 Nitpick comments (14)
mpp-vscode/webview/src/components/sketch/TerminalRenderer.css (1)
1-137: Extract repeated spacing and sizing values into CSS custom properties for maintainability.Spacing values (4px, 6px, 8px, 12px) and font-size values are repeated throughout the file. Defining these as CSS custom properties would reduce duplication and make future adjustments easier.
Apply this diff to extract spacing values:
+:root { + --spacing-xs: 4px; + --spacing-sm: 6px; + --spacing-md: 8px; + --spacing-lg: 12px; + --font-size-sm: 11px; + --font-size-xs: 12px; + --font-size-md: 13px; +} + .terminal-renderer { border: 1px solid var(--vscode-panel-border); border-radius: 6px; overflow: hidden; background-color: var(--vscode-terminal-background, #1e1e1e); } .terminal-header { display: flex; align-items: center; - gap: 8px; - padding: 6px 12px; + gap: var(--spacing-md); + padding: var(--spacing-sm) var(--spacing-lg); background-color: var(--vscode-editorWidget-background); border-bottom: 1px solid var(--vscode-panel-border); } .terminal-icon { - font-size: 12px; + font-size: var(--font-size-xs); } .terminal-title { - font-size: 12px; + font-size: var(--font-size-xs); font-weight: 500; color: var(--vscode-descriptionForeground); flex: 1; } .terminal-status { - font-size: 11px; - padding: 2px 6px; + font-size: var(--font-size-sm); + padding: 2px var(--spacing-sm); border-radius: 3px; }mpp-vscode/webview/src/components/sketch/DiffRenderer.css (1)
53-69: Hardcoded hover colors may not match custom VSCode themes.The
.acceptand.rejectbutton hover states use hardcoded hex colors (#45a049,#da190b) instead of VSCode CSS variables. While the default states correctly usevar(--vscode-testing-iconPassed)andvar(--vscode-testing-iconFailed)with fallbacks, the hover states bypass theming entirely.Consider using
filter: brightness(0.9)or similar approach to derive hover colors from the themed base colors:.diff-action-btn.accept:hover:not(:disabled) { - background-color: #45a049; + filter: brightness(0.9); } ... .diff-action-btn.reject:hover:not(:disabled) { - background-color: #da190b; + filter: brightness(0.9); }mpp-vscode/webview/src/components/sketch/SketchRenderer.css (1)
43-52: Duplicated@keyframes pulseanimation.This
pulsekeyframe animation is identical to the one inThinkingRenderer.css(lines 93-102). Consider extracting shared animations to a common CSS file (e.g.,animations.css) to maintain DRY principles./* shared/animations.css */ @keyframes pulse { 0%, 80%, 100% { opacity: 0.4; transform: scale(1); } 40% { opacity: 1; transform: scale(1.2); } }Then import this shared file in both components.
mpp-vscode/webview/src/components/Timeline.tsx (1)
133-145: Prop types don't align with the TimelineItem union types.
TaskCompleteItemRendererandErrorItemRendereruse inline object types that don't reference the actualTaskCompleteTimelineItemandErrorTimelineItemtypes fromtypes/timeline.ts. This creates inconsistency with other renderers and loses type safety.Import and use the proper types:
+import type { TimelineItem, MessageTimelineItem, ToolCallTimelineItem, TerminalTimelineItem, TaskCompleteTimelineItem, ErrorTimelineItem } from '../types/timeline'; ... -const TaskCompleteItemRenderer: React.FC<{ item: { success: boolean; message: string } }> = ({ item }) => ( +const TaskCompleteItemRenderer: React.FC<{ item: TaskCompleteTimelineItem }> = ({ item }) => ( <div className={`timeline-item task-complete ${item.success ? 'success' : 'error'}`}> ... -const ErrorItemRenderer: React.FC<{ item: { message: string } }> = ({ item }) => ( +const ErrorItemRenderer: React.FC<{ item: ErrorTimelineItem }> = ({ item }) => ( <div className="timeline-item error">mpp-vscode/webview/src/components/sketch/ThinkingRenderer.tsx (1)
48-50: Empty lines render as empty<p>elements.When content contains consecutive newlines, empty
<p>tags are created which may affect layout. Consider filtering empty lines or using a different approach.{content.split('\n').map((line, index) => ( - <p key={index}>{line}</p> + line.trim() ? <p key={index}>{line}</p> : <br key={index} /> ))}Alternatively, filter out empty lines if they shouldn't render at all.
mpp-vscode/webview/src/components/sketch/CodeBlockRenderer.tsx (1)
9-14: TightenonActiontyping for safer integrationRight now
onActionis(action: string, data: any), but all callsites here use just'insert'and'apply'with{ code, language }. Narrowing this improves safety and DX:-interface CodeBlockRendererProps { +interface CodeBlockActionPayload { + code: string; + language: string; +} + +type CodeBlockAction = 'insert' | 'apply'; + +interface CodeBlockRendererProps { code: string; language: string; isComplete?: boolean; - onAction?: (action: string, data: any) => void; + onAction?: (action: CodeBlockAction, data: CodeBlockActionPayload) => void; }Callsites then get proper autocomplete and cannot send arbitrary actions.
Also applies to: 35-41
mpp-vscode/webview/src/components/Timeline.css (1)
1-154: Timeline styling is coherent and VSCode-theme friendlyThe role-based message styling, streaming indicator, and task/error variants are well structured and rely on VSCode CSS vars, which is ideal for theming. Note that
@keyframes pulsealso appears in other stylesheets; if more animations are added later, consider centralizing it to reduce duplication, but it’s fine as-is.mpp-vscode/webview/src/App.tsx (2)
94-115: Avoid mutating timeline items in-place when handlingtoolResultInside
toolResult, you clone the array but then mutate the found item:const timeline = [...prev.timeline]; // ... const item = timeline[i] as ToolCallTimelineItem; item.toolCall = { ...item.toolCall, success: msg.data?.success as boolean | undefined, output: msg.data?.output as string | undefined, summary: msg.data?.summary as string | undefined };This works because the array reference changes, but mutating
itembreaks a pure-immutable pattern and can make future refactors trickier.Consider cloning the item as well:
- if (timeline[i].type === 'tool_call') { - const item = timeline[i] as ToolCallTimelineItem; - item.toolCall = { - ...item.toolCall, - success: msg.data?.success as boolean | undefined, - output: msg.data?.output as string | undefined, - summary: msg.data?.summary as string | undefined - }; - break; - } + if (timeline[i].type === 'tool_call') { + const item = timeline[i] as ToolCallTimelineItem; + timeline[i] = { + ...item, + toolCall: { + ...item.toolCall, + success: msg.data?.success as boolean | undefined, + output: msg.data?.output as string | undefined, + summary: msg.data?.summary as string | undefined + } + }; + break; + }This keeps state updates fully immutable and easier to reason about.
17-24: Factor out a sharedinitialAgentStateinstead of duplicating the literalThe initial
AgentStateliteral is repeated in theuseStateinitializer and thehistoryClearedhandler. To avoid drift if fields are added later, consider extracting a shared constant:-const App: React.FC = () => { - // Agent state - mirrors ComposeRenderer's state - const [agentState, setAgentState] = useState<AgentState>({ - timeline: [], - currentStreamingContent: '', - isProcessing: false, - currentIteration: 0, - maxIterations: 10, - tasks: [], - }); +const initialAgentState: AgentState = { + timeline: [], + currentStreamingContent: '', + isProcessing: false, + currentIteration: 0, + maxIterations: 10, + tasks: [], +}; + +const App: React.FC = () => { + const [agentState, setAgentState] = useState<AgentState>(initialAgentState); // ... - case 'historyCleared': - setAgentState({ - timeline: [], - currentStreamingContent: '', - isProcessing: false, - currentIteration: 0, - maxIterations: 10, - tasks: [], - }); + case 'historyCleared': + setAgentState(initialAgentState); break;Keeps the reset behavior consistent as the state shape evolves.
Also applies to: 167-175
mpp-vscode/webview/src/components/sketch/CodeBlockRenderer.css (1)
72-99: Streaming indicator relies on global@keyframes pulse
.code-streaming-indicator .dotusesanimation: pulsebut this file doesn’t define the keyframes. SinceApp.css/Timeline.cssalready define@keyframes pulse, it works in this app, but if this component/CSS were ever reused in isolation, the dots would stop animating.Not urgent, but you could either:
- Add a local
@keyframes code-block-pulseand reference that here, or- Centralize
@keyframes pulsein a shared stylesheet imported everywhere.mpp-vscode/webview/src/components/sketch/MarkdownRenderer.tsx (1)
43-52: Reorder spread props for task listinputto avoid overriding your attributesIn the
inputrenderer:input: ({ checked, ...props }) => ( <input type="checkbox" checked={checked} readOnly className="md-checkbox" {...props} /> ),Because
{...props}comes last, anytype,checked,readOnly, orclassNamefrompropswill override your intended values and styling.Safer pattern:
- input: ({ checked, ...props }) => ( - <input - type="checkbox" - checked={checked} - readOnly - className="md-checkbox" - {...props} - /> - ), + input: ({ checked, className, ...props }) => ( + <input + {...props} + type="checkbox" + checked={!!checked} + readOnly + className={`md-checkbox${className ? ` ${className}` : ''}`} + /> + ),This keeps your semantics and styling stable while still honoring extra props.
mpp-vscode/webview/src/components/sketch/TerminalRenderer.tsx (1)
8-15: NarrowonActiontype for terminal actionsHere too
onActionis very loose:onAction?: (action: string, data: any) => void;But the only action emitted is
'run-command'with{ command }. You can mirror that explicitly:+type TerminalAction = 'run-command'; + +interface TerminalActionPayload { + command: string; +} + interface TerminalRendererProps { command: string; output?: string; exitCode?: number; executionTimeMs?: number; isComplete?: boolean; - onAction?: (action: string, data: any) => void; + onAction?: (action: TerminalAction, data: TerminalActionPayload) => void; }This makes downstream handling safer and self-documenting.
Also applies to: 37-39
mpp-vscode/webview/src/App.css (1)
1-74: App shell styling looks solid; consider centralizing shared animationThe app layout and header/status styling align well with VSCode theming tokens and provide a clean top-level shell. As with
Timeline.css,@keyframes pulseis duplicated here; if more animated elements are added later, you might centralize this in a shared stylesheet, but it’s not a blocker.mpp-vscode/webview/src/hooks/useVSCode.ts (1)
87-112: Consider memoizing the API reference for idiomatic React.The hook works correctly since
getVSCodeAPI()returns a singleton, but calling it directly at line 91 (outside React's state/ref system) and includingapiin the dependency array (line 109) isn't idiomatic React.For better React conventions, consider:
export function useVSCodeState<T>(key: string, initialValue: T): [T, (value: T) => void] { - const api = getVSCodeAPI(); + const [api] = useState<VSCodeAPI | null>(() => getVSCodeAPI()); const [state, setState] = useState<T>(() => {This makes the API reference part of React's state management and clarifies that it's stable across renders. The functionality remains the same, but the code better aligns with React patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
mpp-vscode/src/bridge/mpp-core.ts(1 hunks)mpp-vscode/src/providers/chat-view.ts(1 hunks)mpp-vscode/webview/src/App.css(1 hunks)mpp-vscode/webview/src/App.tsx(1 hunks)mpp-vscode/webview/src/components/Timeline.css(1 hunks)mpp-vscode/webview/src/components/Timeline.tsx(1 hunks)mpp-vscode/webview/src/components/sketch/CodeBlockRenderer.css(1 hunks)mpp-vscode/webview/src/components/sketch/CodeBlockRenderer.tsx(1 hunks)mpp-vscode/webview/src/components/sketch/DiffRenderer.css(1 hunks)mpp-vscode/webview/src/components/sketch/DiffRenderer.tsx(1 hunks)mpp-vscode/webview/src/components/sketch/MarkdownRenderer.css(1 hunks)mpp-vscode/webview/src/components/sketch/MarkdownRenderer.tsx(1 hunks)mpp-vscode/webview/src/components/sketch/SketchRenderer.css(1 hunks)mpp-vscode/webview/src/components/sketch/SketchRenderer.tsx(1 hunks)mpp-vscode/webview/src/components/sketch/TerminalRenderer.css(1 hunks)mpp-vscode/webview/src/components/sketch/TerminalRenderer.tsx(1 hunks)mpp-vscode/webview/src/components/sketch/ThinkingRenderer.css(1 hunks)mpp-vscode/webview/src/components/sketch/ThinkingRenderer.tsx(1 hunks)mpp-vscode/webview/src/components/sketch/ToolCallRenderer.css(1 hunks)mpp-vscode/webview/src/components/sketch/ToolCallRenderer.tsx(1 hunks)mpp-vscode/webview/src/components/sketch/index.ts(1 hunks)mpp-vscode/webview/src/hooks/useVSCode.ts(1 hunks)mpp-vscode/webview/src/types/timeline.ts(1 hunks)mpp-vscode/webview/src/utils/codeFence.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Build MPP Core with: `cd /Volumes/source/ai/autocrud && ./gradlew :mpp-core:assembleJsPackage`
📚 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 mpp-ui/src/jsMain/typescript/**/*.{ts,tsx} : In CLI/TUI (TypeScript), use `mpp-ui/src/jsMain/typescript/design-system/` and import `semanticInk` / `semanticChalk`
Applied to files:
mpp-vscode/webview/src/components/Timeline.cssmpp-vscode/webview/src/components/sketch/TerminalRenderer.cssmpp-vscode/webview/src/components/sketch/SketchRenderer.tsxmpp-vscode/webview/src/components/sketch/index.tsmpp-vscode/src/bridge/mpp-core.ts
📚 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-vscode/src/bridge/mpp-core.ts
📚 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-vscode/src/bridge/mpp-core.ts
🧬 Code graph analysis (11)
mpp-vscode/webview/src/utils/codeFence.ts (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/parser/CodeFence.kt (1)
languageId(7-348)
mpp-vscode/webview/src/components/Timeline.tsx (4)
mpp-vscode/webview/src/types/timeline.ts (4)
TimelineItem(81-86)MessageTimelineItem(54-58)ToolCallTimelineItem(60-63)TerminalTimelineItem(65-68)mpp-vscode/webview/src/components/sketch/SketchRenderer.tsx (1)
SketchRenderer(29-60)mpp-vscode/webview/src/components/sketch/ToolCallRenderer.tsx (1)
ToolCallRenderer(15-112)mpp-vscode/webview/src/components/sketch/TerminalRenderer.tsx (1)
TerminalRenderer(17-101)
mpp-vscode/webview/src/components/sketch/ThinkingRenderer.tsx (1)
mpp-vscode/webview/src/components/sketch/index.ts (1)
ThinkingRenderer(19-19)
mpp-vscode/webview/src/components/sketch/DiffRenderer.tsx (1)
mpp-vscode/webview/src/components/sketch/index.ts (1)
DiffRenderer(18-18)
mpp-vscode/webview/src/components/sketch/SketchRenderer.tsx (8)
mpp-vscode/webview/src/components/sketch/index.ts (6)
SketchRenderer(15-15)MarkdownRenderer(16-16)DiffRenderer(18-18)ThinkingRenderer(19-19)TerminalRenderer(20-20)CodeBlockRenderer(17-17)mpp-vscode/webview/src/utils/codeFence.ts (2)
parseCodeBlocks(17-87)CodeBlock(6-11)mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/parser/CodeFence.kt (1)
languageId(7-348)mpp-vscode/webview/src/components/sketch/MarkdownRenderer.tsx (1)
MarkdownRenderer(17-115)mpp-vscode/webview/src/components/sketch/DiffRenderer.tsx (1)
DiffRenderer(19-86)mpp-vscode/webview/src/components/sketch/ThinkingRenderer.tsx (1)
ThinkingRenderer(14-59)mpp-vscode/webview/src/components/sketch/TerminalRenderer.tsx (1)
TerminalRenderer(17-101)mpp-vscode/webview/src/components/sketch/CodeBlockRenderer.tsx (1)
CodeBlockRenderer(16-87)
mpp-vscode/webview/src/components/sketch/TerminalRenderer.tsx (2)
mpp-vscode/webview/src/components/sketch/index.ts (1)
TerminalRenderer(20-20)mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/tool/impl/NodeFetchHttpFetcher.js.kt (1)
setTimeout(134-134)
mpp-vscode/webview/src/components/sketch/CodeBlockRenderer.tsx (2)
mpp-vscode/webview/src/components/sketch/index.ts (1)
CodeBlockRenderer(17-17)mpp-vscode/webview/src/utils/codeFence.ts (1)
getDisplayName(130-165)
mpp-vscode/webview/src/components/sketch/ToolCallRenderer.tsx (3)
mpp-vscode/webview/src/types/timeline.ts (1)
ToolCallInfo(13-25)mpp-vscode/webview/src/components/sketch/index.ts (1)
ToolCallRenderer(21-21)mpp-ui/src/test/framework/reporters/ConsoleReporter.ts (1)
getStatusIcon(196-205)
mpp-vscode/webview/src/App.tsx (4)
mpp-vscode/webview/src/types/timeline.ts (4)
AgentState(89-97)ToolCallInfo(13-25)ToolCallTimelineItem(60-63)TerminalOutput(27-32)mpp-vscode/webview/src/hooks/useVSCode.ts (2)
useVSCode(64-85)ExtensionMessage(15-35)mpp-vscode/webview/src/components/Timeline.tsx (1)
Timeline(19-56)mpp-vscode/webview/src/components/ChatInput.tsx (1)
ChatInput(11-93)
mpp-vscode/src/bridge/mpp-core.ts (2)
mpp-core/src/jsMain/kotlin/cc/unitmesh/llm/JsExports.kt (1)
providerName(168-197)mpp-core/src/commonMain/kotlin/cc/unitmesh/devins/completion/providers/ToolBasedCommandCompletionProvider.kt (1)
toolRegistry(13-65)
mpp-vscode/webview/src/hooks/useVSCode.ts (2)
mpp-vscode/src/providers/chat-view.ts (1)
postMessage(74-76)mpp-vscode/test/mocks/vscode.ts (1)
window(30-43)
🪛 Biome (2.1.2)
mpp-vscode/src/providers/chat-view.ts
[error] 179-179: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (26)
mpp-vscode/src/bridge/mpp-core.ts (3)
316-418: LGTM!The
VSCodeRendererclass properly implements a forward-to-webview pattern, translating agent events into structured messages. The method signatures align well with typical agent rendering callbacks.
505-526: LGTM!The
executeTaskmethod correctly maps the Kotlin result structure to TypeScript interfaces usingArray.from()for collection conversion.
531-544: Defensive optional chaining on potentially undefined methods.Using
this.agent.clearHistory?.()andthis.agent.getConversationHistory?.()is a good defensive pattern for Kotlin/JS interop where method availability may vary.mpp-vscode/src/providers/chat-view.ts (3)
112-145: Re-initialization on every message may silently fail.If
initializeAgent()fails (e.g., missing API key), subsequent messages will repeatedly attempt re-initialization. The current pattern catches the error silently ininitializeAgent, but thenexecute()is still called with a potentially undefinedcodingAgent. Line 126 guards this, but non-DevIns commands at line 131-138 proceed without the agent, which is the intended fallback behavior.
229-313: Webview HTML and CSP implementation.The CSP implementation with nonces for scripts is well done. Previous comments noted
'unsafe-inline'for styles and inline styles in the fallback HTML; these remain present but are reasonable tradeoffs for a fallback UI that only activates when the React bundle fails to load.
315-322: LGTM!The nonce generation uses a 32-character random string from a safe character set, which is sufficient for CSP script protection.
mpp-vscode/webview/src/components/sketch/DiffRenderer.css (1)
1-130: LGTM overall - well-structured CSS with good VSCode theming integration.The stylesheet properly uses VSCode CSS variables for consistent theming, includes sensible fallbacks, handles disabled states correctly, and provides good visual feedback for diff operations.
mpp-vscode/webview/src/components/sketch/ThinkingRenderer.css (1)
1-103: LGTM - clean implementation with consistent theming.The styling properly follows the established patterns in other sketch components, uses performant CSS animations (opacity + transform), and integrates well with VSCode theming.
mpp-vscode/webview/src/components/sketch/ToolCallRenderer.css (1)
1-131: LGTM - well-designed status indicators and consistent styling.The CSS effectively communicates tool call status through visual cues (colored left border) and maintains consistency with other sketch components in terms of theming and layout patterns.
mpp-vscode/webview/src/components/sketch/MarkdownRenderer.css (1)
1-144: LGTM - comprehensive Markdown styling with excellent theming.The stylesheet covers all standard Markdown elements with proper VSCode theming integration, responsive typography using em units, and consistent visual hierarchy.
mpp-vscode/webview/src/components/sketch/SketchRenderer.css (1)
1-42: LGTM - clean layout styling for the sketch container.The flexbox layout and loading indicator styling are well-implemented and consistent with the component architecture.
mpp-vscode/webview/src/components/Timeline.tsx (2)
19-56: LGTM - well-structured timeline rendering with appropriate delegation.The component effectively handles multiple timeline item types with a clean switch-based dispatch pattern and properly supports streaming state visualization.
46-50: No duplication issue exists withcurrentStreamingContentrendering.The state management in App.tsx properly ensures mutual exclusivity: during streaming (
isProcessing: true), content accumulates incurrentStreamingContentand is not added to theitemsarray. Only when the response completes (endResponsecase, lines 61-72) is the full content added to the timeline andcurrentStreamingContentcleared. The Timeline component correctly renders either the streaming indicator or completed items, never both for the same content.mpp-vscode/webview/src/components/sketch/ThinkingRenderer.tsx (1)
14-59: LGTM - clean collapsible component with appropriate state management.The component is well-structured with sensible defaults (collapsed by default, 100-char preview). The toggle behavior and streaming indicator integrate well with the sketch rendering architecture.
mpp-vscode/webview/src/components/sketch/index.ts (1)
1-21: Barrel exports for sketch renderers look goodThe index cleanly re-exports all renderer components and matches the documented list in the header comment. Good centralization of the public surface.
mpp-vscode/webview/src/components/sketch/SketchRenderer.tsx (2)
1-27: LGTM - Clean component structure.The imports and interface definition are well-organized. The props interface appropriately includes optional
isCompleteandonActionfor streaming and action handling.
29-60: LGTM - Streaming logic is correct.The component correctly handles streaming content by treating the last block as incomplete when overall content is still streaming (line 40). The loading indicator is appropriately conditional on both completion state and content presence.
mpp-vscode/webview/src/components/sketch/DiffRenderer.tsx (2)
1-86: LGTM - Well-implemented diff viewer.The component provides a clean interface for viewing and acting on diffs. The state management correctly prevents duplicate actions by disabling buttons after acceptance, and the applied overlay provides clear visual feedback.
88-101: LGTM - Adequate diff parsing for the use case.The
parseDifffunction correctly handles standard unified diff format. While it doesn't cover all edge cases (e.g., complex diff metadata), it's sufficient for typical LLM-generated diff content in this context.mpp-vscode/webview/src/utils/codeFence.ts (2)
89-125: LGTM - Comprehensive extension mapping.The extension map covers common programming languages with a sensible fallback. The implementation is clean and maintainable.
127-165: LGTM - Good display name mapping with fallback.The display name function provides user-friendly labels for common languages. The fallback
languageId || 'Text'appropriately handles empty strings.mpp-vscode/webview/src/components/sketch/ToolCallRenderer.tsx (2)
1-112: LGTM - Well-designed tool call renderer.The component provides an intuitive interface for viewing tool calls with expand/collapse, status indicators, and tool-specific parameter formatting. The error handling around
JSON.parse(line 65) is appropriate, and the rerun action enables easy workflow iteration.
114-126: LGTM - Helpful utility functions.Both
formatTimeandtruncateOutputimprove the user experience by presenting data in digestible formats. The default 20-line truncation is reasonable for preventing UI clutter while the continuation indicator keeps users informed.mpp-vscode/webview/src/types/timeline.ts (1)
1-98: LGTM - Well-structured TypeScript types.The type definitions follow TypeScript best practices:
- Proper discriminated union using the
typefield as discriminator- Comprehensive interfaces with appropriate optional fields
- Clear naming and organization
These types provide a solid foundation for type-safe timeline rendering.
mpp-vscode/webview/src/hooks/useVSCode.ts (2)
1-59: LGTM - Solid API setup with fallback handling.The VSCode API acquisition uses an appropriate singleton pattern and provides helpful fallback logging for development outside VSCode. The message type definitions are comprehensive and well-structured.
61-85: LGTM - Well-implemented communication hook.The
useVSCodehook correctly usesuseStatewith an initializer function (line 65), ensuring the API is acquired only once. The fallback logging and cleanup function for event listeners demonstrate good practices.
| config.apiKey, | ||
| config.temperature ?? 0.7, | ||
| config.maxTokens ?? 8192, | ||
| config.baseUrl ?? '' |
There was a problem hiding this comment.
maxTokens default differs from Kotlin's JsModelConfig default.
The TypeScript wrapper defaults maxTokens to 8192, but the Kotlin JsModelConfig defines maxTokens: Int = 128000 as its default. This mismatch may cause unexpected token limit behavior when the caller doesn't explicitly provide a value.
Consider aligning with the Kotlin default:
- config.maxTokens ?? 8192,
+ config.maxTokens ?? 128000,Apply the same change at line 447 in the CodingAgent constructor.
Also applies to: 446-448
🤖 Prompt for AI Agents
In mpp-vscode/src/bridge/mpp-core.ts around lines 69 to 72 (and also update the
CodingAgent constructor at lines ~446-448 / line 447), the TypeScript wrapper
currently defaults maxTokens to 8192 which mismatches Kotlin's JsModelConfig
default of 128000; update the default value for maxTokens to 128000 in both
locations so the JS/TS defaults align with the Kotlin default.
| switch (action) { | ||
| case 'insert': | ||
| // Insert code at cursor | ||
| const editor = vscode.window.activeTextEditor; | ||
| if (editor && data.code) { | ||
| await editor.edit(editBuilder => { | ||
| editBuilder.insert(editor.selection.active, data.code); | ||
| }); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Wrap switch case declaration in a block.
The const editor declaration inside the switch case can be erroneously accessed by other case clauses. Wrap the case body in a block to restrict scope.
case 'insert':
- // Insert code at cursor
- const editor = vscode.window.activeTextEditor;
- if (editor && data.code) {
- await editor.edit(editBuilder => {
- editBuilder.insert(editor.selection.active, data.code);
- });
- }
+ {
+ // Insert code at cursor
+ const editor = vscode.window.activeTextEditor;
+ if (editor && data.code) {
+ await editor.edit(editBuilder => {
+ editBuilder.insert(editor.selection.active, data.code);
+ });
+ }
+ }
break;📝 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.
| switch (action) { | |
| case 'insert': | |
| // Insert code at cursor | |
| const editor = vscode.window.activeTextEditor; | |
| if (editor && data.code) { | |
| await editor.edit(editBuilder => { | |
| editBuilder.insert(editor.selection.active, data.code); | |
| }); | |
| } | |
| break; | |
| switch (action) { | |
| case 'insert': | |
| { | |
| // Insert code at cursor | |
| const editor = vscode.window.activeTextEditor; | |
| if (editor && data.code) { | |
| await editor.edit(editBuilder => { | |
| editBuilder.insert(editor.selection.active, data.code); | |
| }); | |
| } | |
| } | |
| break; |
🧰 Tools
🪛 Biome (2.1.2)
[error] 179-179: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In mpp-vscode/src/providers/chat-view.ts around lines 176 to 185, the const
editor declared inside the 'insert' switch case is not block-scoped to the case
and could be accessed by other case clauses; wrap the entire 'insert' case body
in its own block (add { ... } around the statements after case 'insert':) so the
const editor (and any other local declarations) are scoped to that case, keeping
the await editor.edit call and the break inside the new block.
| // Iteration update | ||
| case 'iterationUpdate': | ||
| setAgentState(prev => ({ | ||
| ...prev, | ||
| currentIteration: Number(msg.data?.current) || prev.currentIteration, | ||
| maxIterations: Number(msg.data?.max) || prev.maxIterations | ||
| })); | ||
| break; |
There was a problem hiding this comment.
iterationUpdate uses || and will ignore legitimate zero values
Using Number(... ) || prev.xxx means 0 is treated as falsy and replaced by the previous value, which is usually not what you want for counters:
currentIteration: Number(msg.data?.current) || prev.currentIteration,
maxIterations: Number(msg.data?.max) || prev.maxIterationsIf the extension ever sends 0 (or 0 is a valid value), the update will be dropped. Safer variants:
- currentIteration: Number(msg.data?.current) || prev.currentIteration,
- maxIterations: Number(msg.data?.max) || prev.maxIterations
+ const current = msg.data?.current;
+ const max = msg.data?.max;
+ currentIteration:
+ current !== undefined ? Number(current) : prev.currentIteration,
+ maxIterations:
+ max !== undefined ? Number(max) : prev.maxIterations(or use ?? with a prior Number(...) plus Number.isFinite checks).
🤖 Prompt for AI Agents
In mpp-vscode/webview/src/App.tsx around lines 178 to 185, the code uses
`Number(msg.data?.current) || prev.currentIteration` and `Number(msg.data?.max)
|| prev.maxIterations`, which treats legitimate zero values as falsy and
preserves the previous values; instead parse the numbers and only fall back when
the parsed value is not a finite number — e.g. compute parsedCurrent =
Number(msg.data?.current) and use parsedCurrent if
Number.isFinite(parsedCurrent) otherwise prev.currentIteration, and do the same
for maxIterations.
| case 'mermaid': | ||
| case 'mmd': | ||
| // TODO: Implement MermaidRenderer | ||
| return ( | ||
| <CodeBlockRenderer | ||
| code={block.text} | ||
| language="mermaid" | ||
| isComplete={isComplete} | ||
| /> | ||
| ); | ||
|
|
||
| case 'devin': | ||
| // TODO: Implement DevInRenderer | ||
| return ( | ||
| <CodeBlockRenderer | ||
| code={block.text} | ||
| language="devin" | ||
| isComplete={isComplete} | ||
| /> | ||
| ); | ||
|
|
||
| case 'plan': | ||
| // TODO: Implement PlanRenderer | ||
| return ( | ||
| <CodeBlockRenderer | ||
| code={block.text} | ||
| language="plan" | ||
| isComplete={isComplete} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Pass onAction consistently to all CodeBlockRenderer instances.
The special-case CodeBlockRenderer calls (mermaid, devin, plan) don't pass the onAction prop, while the default case at lines 141-147 does. This inconsistency means users won't be able to trigger actions (copy, insert, apply) on these block types.
Apply this diff to pass onAction consistently:
case 'mermaid':
case 'mmd':
// TODO: Implement MermaidRenderer
return (
<CodeBlockRenderer
code={block.text}
language="mermaid"
isComplete={isComplete}
+ onAction={onAction}
/>
);
case 'devin':
// TODO: Implement DevInRenderer
return (
<CodeBlockRenderer
code={block.text}
language="devin"
isComplete={isComplete}
+ onAction={onAction}
/>
);
case 'plan':
// TODO: Implement PlanRenderer
return (
<CodeBlockRenderer
code={block.text}
language="plan"
isComplete={isComplete}
+ onAction={onAction}
/>
);📝 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.
| case 'mermaid': | |
| case 'mmd': | |
| // TODO: Implement MermaidRenderer | |
| return ( | |
| <CodeBlockRenderer | |
| code={block.text} | |
| language="mermaid" | |
| isComplete={isComplete} | |
| /> | |
| ); | |
| case 'devin': | |
| // TODO: Implement DevInRenderer | |
| return ( | |
| <CodeBlockRenderer | |
| code={block.text} | |
| language="devin" | |
| isComplete={isComplete} | |
| /> | |
| ); | |
| case 'plan': | |
| // TODO: Implement PlanRenderer | |
| return ( | |
| <CodeBlockRenderer | |
| code={block.text} | |
| language="plan" | |
| isComplete={isComplete} | |
| /> | |
| ); | |
| case 'mermaid': | |
| case 'mmd': | |
| // TODO: Implement MermaidRenderer | |
| return ( | |
| <CodeBlockRenderer | |
| code={block.text} | |
| language="mermaid" | |
| isComplete={isComplete} | |
| onAction={onAction} | |
| /> | |
| ); | |
| case 'devin': | |
| // TODO: Implement DevInRenderer | |
| return ( | |
| <CodeBlockRenderer | |
| code={block.text} | |
| language="devin" | |
| isComplete={isComplete} | |
| onAction={onAction} | |
| /> | |
| ); | |
| case 'plan': | |
| // TODO: Implement PlanRenderer | |
| return ( | |
| <CodeBlockRenderer | |
| code={block.text} | |
| language="plan" | |
| isComplete={isComplete} | |
| onAction={onAction} | |
| /> | |
| ); |
🤖 Prompt for AI Agents
In mpp-vscode/webview/src/components/sketch/SketchRenderer.tsx around lines 108
to 137, the CodeBlockRenderer calls for 'mermaid'/'mmd', 'devin', and 'plan' are
missing the onAction prop which the default case provides; update each of those
CodeBlockRenderer returns to include the onAction={onAction} prop so all block
types receive the same action handler (ensure the prop name matches the existing
handler variable in this file).
| .terminal-running { | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 6px; | ||
| margin-top: 8px; | ||
| color: var(--vscode-terminal-ansiYellow, #dcdcaa); | ||
| } | ||
|
|
||
| .terminal-running .dot { | ||
| width: 4px; | ||
| height: 4px; | ||
| background-color: var(--vscode-terminal-ansiYellow, #dcdcaa); | ||
| border-radius: 50%; | ||
| animation: pulse 1.4s ease-in-out infinite; | ||
| } | ||
|
|
||
| .terminal-running .dot:nth-child(1) { animation-delay: 0s; } | ||
| .terminal-running .dot:nth-child(2) { animation-delay: 0.2s; } | ||
| .terminal-running .dot:nth-child(3) { animation-delay: 0.4s; } | ||
|
|
||
| .running-text { | ||
| font-size: 12px; | ||
| } | ||
|
|
||
| @keyframes pulse { | ||
| 0%, 80%, 100% { opacity: 0.4; transform: scale(1); } | ||
| 40% { opacity: 1; transform: scale(1.2); } | ||
| } |
There was a problem hiding this comment.
Add prefers-reduced-motion media query for animation accessibility.
The pulsing animation on lines 118-128 and 134-137 doesn't respect the user's motion preferences. Users with vestibular disorders or motion sensitivity have the prefers-reduced-motion preference enabled in their OS, and animations should be disabled or significantly reduced for accessibility compliance.
Apply this diff to respect motion preferences:
@keyframes pulse {
0%, 80%, 100% { opacity: 0.4; transform: scale(1); }
40% { opacity: 1; transform: scale(1.2); }
}
+
+@media (prefers-reduced-motion: reduce) {
+ .terminal-running .dot {
+ animation: none;
+ opacity: 0.8;
+ }
+}📝 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.
| .terminal-running { | |
| display: flex; | |
| align-items: center; | |
| gap: 6px; | |
| margin-top: 8px; | |
| color: var(--vscode-terminal-ansiYellow, #dcdcaa); | |
| } | |
| .terminal-running .dot { | |
| width: 4px; | |
| height: 4px; | |
| background-color: var(--vscode-terminal-ansiYellow, #dcdcaa); | |
| border-radius: 50%; | |
| animation: pulse 1.4s ease-in-out infinite; | |
| } | |
| .terminal-running .dot:nth-child(1) { animation-delay: 0s; } | |
| .terminal-running .dot:nth-child(2) { animation-delay: 0.2s; } | |
| .terminal-running .dot:nth-child(3) { animation-delay: 0.4s; } | |
| .running-text { | |
| font-size: 12px; | |
| } | |
| @keyframes pulse { | |
| 0%, 80%, 100% { opacity: 0.4; transform: scale(1); } | |
| 40% { opacity: 1; transform: scale(1.2); } | |
| } | |
| .terminal-running { | |
| display: flex; | |
| align-items: center; | |
| gap: 6px; | |
| margin-top: 8px; | |
| color: var(--vscode-terminal-ansiYellow, #dcdcaa); | |
| } | |
| .terminal-running .dot { | |
| width: 4px; | |
| height: 4px; | |
| background-color: var(--vscode-terminal-ansiYellow, #dcdcaa); | |
| border-radius: 50%; | |
| animation: pulse 1.4s ease-in-out infinite; | |
| } | |
| .terminal-running .dot:nth-child(1) { animation-delay: 0s; } | |
| .terminal-running .dot:nth-child(2) { animation-delay: 0.2s; } | |
| .terminal-running .dot:nth-child(3) { animation-delay: 0.4s; } | |
| .running-text { | |
| font-size: 12px; | |
| } | |
| @keyframes pulse { | |
| 0%, 80%, 100% { opacity: 0.4; transform: scale(1); } | |
| 40% { opacity: 1; transform: scale(1.2); } | |
| } | |
| @media (prefers-reduced-motion: reduce) { | |
| .terminal-running .dot { | |
| animation: none; | |
| opacity: 0.8; | |
| } | |
| } |
🤖 Prompt for AI Agents
In mpp-vscode/webview/src/components/sketch/TerminalRenderer.css around lines
110 to 137, the pulsing dot animation does not respect users'
prefers-reduced-motion setting; add a prefers-reduced-motion media query that
disables or removes the animation for users who opt out of motion (e.g., set
.terminal-running .dot { animation: none; } and remove any transforms/opacity
transitions inside that media query) so the dots remain static when
prefers-reduced-motion: reduce is set.
| /** | ||
| * CodeFence parser - mirrors mpp-core's CodeFence.parseAll() | ||
| * Parses markdown content into code blocks and text blocks | ||
| */ | ||
|
|
||
| export interface CodeBlock { | ||
| languageId: string; | ||
| text: string; | ||
| isComplete: boolean; | ||
| extension?: string; | ||
| } | ||
|
|
||
| /** | ||
| * Parse content into code blocks and text blocks | ||
| * Handles markdown code fences (```) and special block types | ||
| */ | ||
| export function parseCodeBlocks(content: string): CodeBlock[] { | ||
| const blocks: CodeBlock[] = []; | ||
| const lines = content.split('\n'); | ||
|
|
||
| let currentBlock: CodeBlock | null = null; | ||
| let textBuffer: string[] = []; | ||
|
|
||
| for (let i = 0; i < lines.length; i++) { | ||
| const line = lines[i]; | ||
| const trimmedLine = line.trim(); | ||
|
|
||
| // Check for code fence start | ||
| if (trimmedLine.startsWith('```')) { | ||
| // Flush text buffer | ||
| if (textBuffer.length > 0) { | ||
| const text = textBuffer.join('\n').trim(); | ||
| if (text) { | ||
| blocks.push({ | ||
| languageId: '', | ||
| text, | ||
| isComplete: true | ||
| }); | ||
| } | ||
| textBuffer = []; | ||
| } | ||
|
|
||
| if (currentBlock) { | ||
| // End of code block | ||
| currentBlock.isComplete = true; | ||
| blocks.push(currentBlock); | ||
| currentBlock = null; | ||
| } else { | ||
| // Start of code block | ||
| const langMatch = trimmedLine.match(/^```(\w+)?/); | ||
| const languageId = langMatch?.[1] || ''; | ||
| currentBlock = { | ||
| languageId, | ||
| text: '', | ||
| isComplete: false, | ||
| extension: getExtensionForLanguage(languageId) | ||
| }; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (currentBlock) { | ||
| // Inside code block | ||
| currentBlock.text += (currentBlock.text ? '\n' : '') + line; | ||
| } else { | ||
| // Regular text | ||
| textBuffer.push(line); | ||
| } | ||
| } | ||
|
|
||
| // Handle remaining content | ||
| if (currentBlock) { | ||
| // Unclosed code block | ||
| blocks.push(currentBlock); | ||
| } else if (textBuffer.length > 0) { | ||
| const text = textBuffer.join('\n').trim(); | ||
| if (text) { | ||
| blocks.push({ | ||
| languageId: '', | ||
| text, | ||
| isComplete: true | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return blocks; | ||
| } |
There was a problem hiding this comment.
Update comment and consider expanding language regex.
The comment claims this "mirrors mpp-core's CodeFence.parseAll()" but the TypeScript implementation is significantly simplified:
-
Missing special tag support: The Kotlin version handles
<devin>,<thinking>, and<!-- walkthrough_start -->tags, while this implementation only handles standard markdown code fences. -
Limited language detection: Line 50 uses
/^```(\w+)?/which only matches word characters. The Kotlin version usesRegex("\\s*```([\\w#+ ]*)")to support languages like "c++", "c#", and "shell script" with spaces.
Consider these improvements:
/**
- * CodeFence parser - mirrors mpp-core's CodeFence.parseAll()
+ * CodeFence parser - simplified version of mpp-core's CodeFence.parseAll()
* Parses markdown content into code blocks and text blocks
+ * Note: This version only handles standard markdown code fences (```).
+ * Special tags (<devin>, <thinking>, etc.) are not supported in the webview parser.
*/And optionally expand the language regex to support special characters:
} else {
// Start of code block
- const langMatch = trimmedLine.match(/^```(\w+)?/);
+ const langMatch = trimmedLine.match(/^```([\w#+\- ]*)/);
const languageId = langMatch?.[1] || '';🤖 Prompt for AI Agents
mpp-vscode/webview/src/utils/codeFence.ts lines 1-87: the parser currently
misstates it mirrors mpp-core and lacks special-tag handling and robust language
detection; update the top comment to remove or qualify the "mirrors mpp-core"
claim, add parsing for the special tags used in the Kotlin version (<devin>,
<thinking>, <!-- walkthrough_start --> and similar) so they produce appropriate
CodeBlock entries (treat them as text/code blocks with correct isComplete and
languageId behavior), and replace the language regex with one that allows
characters like +, #, -, and spaces (e.g., use a pattern equivalent to
^```([\w#+\- ]*) ) so languages like "c++", "c#", and "shell script" are
recognized; ensure unclosed code fences and text buffer handling remains
consistent with these additions.
Phase 8 - Configuration Management Integration Changes: - Add ConfigManager service to load config from ~/.autodev/config.yaml - Refactor ChatViewProvider to use ConfigManager instead of VSCode settings - Mirror IdeaAgentViewModel's loadConfiguration() pattern - Add lazy initialization of CodingAgent with proper config validation - Create renderer that forwards events to webview (like TuiRenderer) - Add 'Open Config' button in header and config prompt - Support all LLM providers: openai, anthropic, deepseek, ollama, etc. - Auto-create config template when file doesn't exist Architecture: - ConfigManager.load() → AutoDevConfigWrapper → getActiveConfig() - isValid() check before agent initialization - JsKoogLLMService + JsCodingAgent from mpp-core - Renderer implements all JsCodingAgentRenderer methods All 63 tests passing. Refs #31
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
mpp-vscode/src/providers/chat-view.ts (4)
422-455: Consider tightening CSP by removing inline styles from the fallback UI.The CSP currently allows
'unsafe-inline'styles and the fallback chat UI uses multiple inlinestyleattributes. This works but weakens CSP and makes future hardening harder.A cleaner approach would be:
- Move the fallback layout styles into
index.css(e.g.,.fallback-root,.fallback-input,.fallback-button,.message-user,.message-assistant,.message-error).- Then remove
'unsafe-inline'fromstyle-src:- <meta http-equiv="Content-Security-Policy" content="default-src 'none'; style-src ${webview.cspSource} 'unsafe-inline'; script-src 'nonce-${nonce}';"> + <meta http-equiv="Content-Security-Policy" content="default-src 'none'; style-src ${webview.cspSource}; script-src 'nonce-${nonce}';">This keeps the CSP strict while preserving the fallback experience.
52-72: Track webview disposables to avoid event‑handler leaks.
onDidReceiveMessagereturns aDisposablethat is never tracked or disposed, andresolveWebviewViewdoes not register any cleanup when the view is destroyed. Re‑opening the view can accumulate listeners.Consider tracking disposables and cleaning them up on dispose:
export class ChatViewProvider implements vscode.WebviewViewProvider { private webviewView: vscode.WebviewView | undefined; + private readonly disposables: vscode.Disposable[] = []; @@ - // Handle messages from webview - webviewView.webview.onDidReceiveMessage(async (message) => { + // Handle messages from webview + this.disposables.push( + webviewView.webview.onDidReceiveMessage(async (message) => { switch (message.type) { case 'sendMessage': await this.handleUserMessage(message.content); break; @@ case 'openConfig': await this.openConfigFile(); break; } - } - }); + }) + ); + + // Clean up when webview is disposed + this.disposables.push( + webviewView.onDidDispose(() => { + this.disposables.forEach(d => d.dispose()); + this.disposables.length = 0; + this.webviewView = undefined; + }) + );
77-82: Guard sendMessage when the webview is not yet initialized.
sendMessagewill callhandleUserMessageeven ifthis.webviewViewis stillundefined, so allpostMessagecalls become no‑ops and the user sees nothing.A small guard makes the behavior explicit and easier to debug:
async sendMessage(content: string): Promise<void> { - if (this.webviewView) { - this.webviewView.show(true); - } - await this.handleUserMessage(content); + if (!this.webviewView) { + this.log('Cannot send message: chat webview is not initialized'); + return; + } + + this.webviewView.show(true); + await this.handleUserMessage(content); }
354-363: Wrap theinsertcase body in a block to satisfy noSwitchDeclarations.The
const editordeclared inside theinsertcase is currently visible to other cases, which Biome flags. Wrapping the case body in a block scopeseditorcorrectly:- switch (action) { - case 'insert': - // Insert code at cursor - const editor = vscode.window.activeTextEditor; - if (editor && data.code) { - await editor.edit(editBuilder => { - editBuilder.insert(editor.selection.active, data.code); - }); - } - break; + switch (action) { + case 'insert': { + // Insert code at cursor + const editor = vscode.window.activeTextEditor; + if (editor && data.code) { + await editor.edit(editBuilder => { + editBuilder.insert(editor.selection.active, data.code); + }); + } + break; + }
🧹 Nitpick comments (4)
mpp-vscode/src/services/config-manager.ts (3)
31-42: Consider adding validation for MCP server configs.The Kotlin implementation in
mpp-uifilters enabled MCP servers with both!disabledandvalidate()checks. The TypeScript version only checksdisabled, potentially allowing invalid server configs to be returned bygetEnabledMcpServers().Consider adding a validation helper:
export interface McpServerConfig { command?: string; url?: string; args?: string[]; disabled?: boolean; autoApprove?: string[]; env?: Record<string, string>; timeout?: number; trust?: boolean; headers?: Record<string, string>; cwd?: string; } + +function isValidMcpServer(server: McpServerConfig): boolean { + // Server must have either command (stdio) or url (SSE/HTTP) + return !!(server.command || server.url); +}Then update
getEnabledMcpServersto use it.
118-158: Consider using async fs methods and improving error visibility.Two observations:
- Sync/async mismatch: The method is marked
asyncbut uses synchronous fs operations (existsSync,mkdirSync,readFileSync). This can block the extension host. Consider usingfs.promisesfor consistency:- static async load(): Promise<AutoDevConfigWrapper> { + static async load(): Promise<AutoDevConfigWrapper> { + const fsPromises = fs.promises; try { - if (!fs.existsSync(this.CONFIG_DIR)) { - fs.mkdirSync(this.CONFIG_DIR, { recursive: true }); + try { + await fsPromises.access(this.CONFIG_DIR); + } catch { + await fsPromises.mkdir(this.CONFIG_DIR, { recursive: true }); } + + try { + await fsPromises.access(this.CONFIG_FILE); + } catch { + return this.createEmpty(); + } - if (!fs.existsSync(this.CONFIG_FILE)) { - return this.createEmpty(); - } - - const content = fs.readFileSync(this.CONFIG_FILE, 'utf-8'); + const content = await fsPromises.readFile(this.CONFIG_FILE, 'utf-8');
- Silent error swallowing: The catch block silently returns an empty config, which could hide YAML parsing errors or permission issues. Consider logging the error:
} catch (error) { + console.warn('Failed to load config:', error); return this.createEmpty(); }
137-152: Migration doesn't persist the converted format.The legacy config is converted in memory each time
load()is called but not written back to disk. This is functionally correct but means the conversion runs on every load.Consider persisting the migrated config if this becomes a performance concern in the future, or add a comment clarifying this is intentional.
mpp-vscode/webview/src/hooks/useVSCode.ts (1)
75-82: Optionally filterwindowmessage events to just VSCode payloads.
onMessageforwards everymessageevent’sevent.datato the handler. In browser dev/standalone mode, other frames or tooling can also postmessageevents, which may not matchExtensionMessage.You can defensively filter to your expected shape or origin, e.g.:
const onMessage = useCallback((handler: (message: ExtensionMessage) => void) => { - const listener = (event: MessageEvent<ExtensionMessage>) => { - handler(event.data); - }; + const listener = (event: MessageEvent<unknown>) => { + const data = event.data as ExtensionMessage; + if (!data || typeof data !== 'object' || !('type' in data)) return; + handler(data); + };This keeps the hook resilient if other
postMessagetraffic is present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
mpp-vscode/package.json(1 hunks)mpp-vscode/src/providers/chat-view.ts(1 hunks)mpp-vscode/src/services/config-manager.ts(1 hunks)mpp-vscode/webview/src/App.css(1 hunks)mpp-vscode/webview/src/App.tsx(1 hunks)mpp-vscode/webview/src/hooks/useVSCode.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- mpp-vscode/webview/src/App.tsx
- mpp-vscode/webview/src/App.css
- mpp-vscode/package.json
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Build MPP Core with: `cd /Volumes/source/ai/autocrud && ./gradlew :mpp-core:assembleJsPackage`
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Build and run MPP CLI with: `cd mpp-ui && npm run build && npm run start`
🧬 Code graph analysis (2)
mpp-vscode/src/services/config-manager.ts (1)
mpp-ui/src/commonMain/kotlin/cc/unitmesh/devins/ui/config/ConfigFile.kt (1)
configFile(88-145)
mpp-vscode/webview/src/hooks/useVSCode.ts (2)
mpp-vscode/src/providers/chat-view.ts (1)
postMessage(87-89)mpp-vscode/test/mocks/vscode.ts (1)
window(30-43)
🪛 Biome (2.1.2)
mpp-vscode/src/providers/chat-view.ts
[error] 357-358: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (1)
mpp-vscode/src/services/config-manager.ts (1)
56-106: LGTM!The
AutoDevConfigWrapperclass correctly mirrors the Kotlin implementation's core logic, including:
- Fallback to first config when active name isn't found (line 68)
- Provider-specific validation for
ollama(no API key required) andcustom-openai-base(requires baseUrl)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 60 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
mpp-core/src/commonMain/kotlin/cc/unitmesh/markdown/MarkdownTextParser.kt:3
- The import statement for
MarkdownElementTypesis missing. The code usesMarkdownElementTypes.LINK_TEXT,MarkdownElementTypes.LINK_DESTINATIONat lines 108, 118, and 126, but only importsMarkdownTokenTypesfrom the markdown library. This will cause a compilation error.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const colors: Record<StatusBarState, string | undefined> = { | ||
| idle: undefined, | ||
| thinking: new vscode.ThemeColor('statusBarItem.warningForeground').toString(), | ||
| streaming: new vscode.ThemeColor('statusBarItem.prominentForeground').toString(), | ||
| error: new vscode.ThemeColor('statusBarItem.errorForeground').toString() | ||
| }; | ||
|
|
There was a problem hiding this comment.
Unused variable colors.
| const colors: Record<StatusBarState, string | undefined> = { | |
| idle: undefined, | |
| thinking: new vscode.ThemeColor('statusBarItem.warningForeground').toString(), | |
| streaming: new vscode.ThemeColor('statusBarItem.prominentForeground').toString(), | |
| error: new vscode.ThemeColor('statusBarItem.errorForeground').toString() | |
| }; |
- Add ModelSelector component similar to IdeaModelSelector.kt - Update ChatInput with toolbar: ModelSelector on left, Stop/Send on right - Add config state management in App.tsx - Add sendConfigUpdate, stopExecution, selectConfig in chat-view.ts - Add new message types: configUpdate, stopExecution, selectConfig All 63 tests passing. Refs #31
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
mpp-vscode/webview/src/App.tsx (1)
190-197:iterationUpdatestill drops legitimate zero values
iterationUpdateusesNumber(...) || prev..., so a valid0from the extension will be treated as falsy and replaced with the previous value:currentIteration: Number(msg.data?.current) || prev.currentIteration, maxIterations: Number(msg.data?.max) || prev.maxIterationsIf
currentormaxcan legitimately be0, these updates will never take effect. Prefer an explicit finite‑number check or??:case 'iterationUpdate': - setAgentState(prev => ({ - ...prev, - currentIteration: Number(msg.data?.current) || prev.currentIteration, - maxIterations: Number(msg.data?.max) || prev.maxIterations - })); + setAgentState(prev => { + const rawCurrent = msg.data?.current; + const rawMax = msg.data?.max; + const parsedCurrent = rawCurrent !== undefined ? Number(rawCurrent) : NaN; + const parsedMax = rawMax !== undefined ? Number(rawMax) : NaN; + + return { + ...prev, + currentIteration: Number.isFinite(parsedCurrent) + ? parsedCurrent + : prev.currentIteration, + maxIterations: Number.isFinite(parsedMax) + ? parsedMax + : prev.maxIterations + }; + });mpp-vscode/src/providers/chat-view.ts (4)
121-126: Guard sendMessage when webview is not yet initialized
sendMessagewill happily callhandleUserMessageeven whenthis.webviewViewis undefined. In that case, the agent runs but the user sees no UI updates becausepostMessageis a no-op.If you expect
sendMessageto be used programmatically before the view is resolved, this may be fine; otherwise, consider an early guard and log:async sendMessage(content: string): Promise<void> { - if (this.webviewView) { - this.webviewView.show(true); - } - await this.handleUserMessage(content); + if (!this.webviewView) { + this.log('Cannot send message: chat webview not initialized'); + return; + } + + this.webviewView.show(true); + await this.handleUserMessage(content); }
479-563: CSP is good, but consider tightening style-src by reducing inline stylesYou’ve already got a solid CSP with
default-src 'none'and nonce-based scripts. However,style-src ${webview.cspSource} 'unsafe-inline';plus the extensive inline styles in the fallback UI mean you can’t easily drop'unsafe-inline'later.If you want a stricter CSP over time, consider moving the inline fallback styles into the bundled CSS and using simple class names here; that would let you remove
'unsafe-inline'fromstyle-srcwhile keeping behavior the same.
24-31: Track webview message listener disposables and clean them up on dispose
onDidReceiveMessagereturns aDisposablethat is not tracked or disposed. If the view is resolved more than once, listeners will accumulate and may cause duplicate handling and leaks.Consider tracking these disposables and clearing them on
webviewView.onDidDispose:export class ChatViewProvider implements vscode.WebviewViewProvider { private webviewView: vscode.WebviewView | undefined; private codingAgent: any = null; private llmService: any = null; private configWrapper: AutoDevConfigWrapper | null = null; private isExecuting = false; private messages: Array<{ role: string; content: string }> = []; + private disposables: vscode.Disposable[] = []; resolveWebviewView( @@ - // Handle messages from webview - webviewView.webview.onDidReceiveMessage(async (message) => { + // Handle messages from webview + const messageDisposable = webviewView.webview.onDidReceiveMessage(async (message) => { switch (message.type) { @@ - } - }); + } + }); + this.disposables.push(messageDisposable); + + const viewDisposable = webviewView.onDidDispose(() => { + this.disposables.forEach(d => d.dispose()); + this.disposables = []; + this.webviewView = undefined; + }); + this.disposables.push(viewDisposable);Also applies to: 37-79
423-471: Wrap the ‘insert’ switch case body in a block to scopeeditorThe
const editordeclared directly undercase 'insert':is visible to other cases, which Biome flags (noSwitchDeclarations). Wrapping the case body in a block fixes the scoping issue and satisfies the linter:switch (action) { case 'insert': - // Insert code at cursor - const editor = vscode.window.activeTextEditor; - if (editor && data.code) { - await editor.edit(editBuilder => { - editBuilder.insert(editor.selection.active, data.code); - }); - } - break; + { + // Insert code at cursor + const editor = vscode.window.activeTextEditor; + if (editor && data.code) { + await editor.edit(editBuilder => { + editBuilder.insert(editor.selection.active, data.code); + }); + } + break; + }
🧹 Nitpick comments (7)
mpp-vscode/webview/src/hooks/useVSCode.ts (1)
47-60: Reduce repeated warnings and optionally guard inbound messagesThe VSCode API wrapper works, but two minor robustness/DX tweaks are worth considering:
getVSCodeAPI()logs a warning every time it’s called in a non‑VSCode environment (e.g., standalone browser dev).useVSCodeStatecalls it on each render, which can spam the console. Caching the “no VSCode API” result (or gating theconsole.warnbehind a boolean) would keep logs cleaner.onMessagecurrently forwards everywindow"message"event as anExtensionMessage. If this webview ever runs alongside other scripts usingpostMessage, a simple guard (e.g., checking thatevent.source === windowand thatevent.data?.typeis one of the expected strings) would avoid accidental handler invocations.Both are optional, but would make the hook a bit more resilient.
mpp-vscode/webview/src/App.tsx (1)
247-249: ClarifyneedsConfigboolean precedenceThe current expression:
const needsConfig = agentState.timeline.length === 0 && agentState.currentStreamingContent.includes('No configuration found') || agentState.currentStreamingContent.includes('Configuration Required');is parsed as:
(agentState.timeline.length === 0 && agentState.currentStreamingContent.includes('No configuration found')) || agentState.currentStreamingContent.includes('Configuration Required');So any time the streaming content includes “Configuration Required”,
needsConfigistrueeven when the timeline is non‑empty. If you intended the prompt to only appear when the timeline is empty for both messages, wrap the||part in parentheses:-const needsConfig = agentState.timeline.length === 0 && - agentState.currentStreamingContent.includes('No configuration found') || - agentState.currentStreamingContent.includes('Configuration Required'); +const needsConfig = + agentState.timeline.length === 0 && + (agentState.currentStreamingContent.includes('No configuration found') || + agentState.currentStreamingContent.includes('Configuration Required'));If the current behavior is intentional, adding parentheses anyway would still improve readability.
mpp-vscode/webview/src/components/ModelSelector.tsx (1)
30-107: Consider small accessibility improvements for the dropdownThe core behavior is good (buttons for actions, outside‑click close), but a couple of low‑cost tweaks would improve accessibility:
- Add ARIA attributes to the trigger, e.g.
aria-haspopup="listbox"andaria-expanded={isOpen}, so screen readers understand it’s a dropdown.- Optionally handle
Escapekey to close the dropdown when it’s open.- If you expect many configs, you might later promote the list to an explicit
role="listbox"/role="option"structure, but that’s not required now.These are incremental and don’t change behavior, just polish.
mpp-vscode/webview/src/components/ChatInput.tsx (1)
104-120: Guard optionalonStophandler for reuse safetyRight now, when
isExecutingis true, the Stop button is rendered with:<button className="action-button stop-button" onClick={onStop} title="Stop execution" >
onStopis optional, so if a future caller forgets to pass it while settingisExecuting, the Stop button will render but do nothing on click. Not a bug in your current usage (App passes it), but you could make the component more self‑defensive by guarding:- onClick={onStop} + onClick={onStop || undefined} + disabled={disabled || !onStop}and mirroring the disabled condition in the Stop vs Send toggle as needed.
mpp-vscode/src/providers/chat-view.ts (3)
95-116: Config switching is session-local only (not persisted)
selectConfigrecreates the LLM service and updates the webview, but it doesn’t persist the new active config back to~/.autodev/config.yaml. If the intention is for the user’s choice to survive extension reloads, you’ll eventually need a small persistence step (e.g., updatingactiveand saving throughConfigManager) rather than only switching in-memory.
473-477: Confirm that JsCodingAgent supports clearHistory
this.codingAgent?.clearHistory();assumes the Kotlin/JS agent exposes aclearHistory()method. GivencodingAgentis typed asany, this won’t be caught at compile time and would surface as a runtime error if the method doesn’t exist.Please double-check the mpp-core API (or the IDEA/mpp-ui counterparts) to ensure
clearHistoryis available onJsCodingAgent, or guard the call more defensively if not.
565-572: Nonce generation is acceptable for nowThe simple
Math.random-based nonce generator is fine for a VS Code webview that doesn’t ingest arbitrary HTML. If you later start embedding richer remote content, you may want to switch to a stronger source (e.g.crypto.getRandomValuesor Node’scrypto.randomBytes) to make CSP nonces harder to predict.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
mpp-vscode/src/providers/chat-view.ts(1 hunks)mpp-vscode/webview/src/App.tsx(1 hunks)mpp-vscode/webview/src/components/ChatInput.css(1 hunks)mpp-vscode/webview/src/components/ChatInput.tsx(1 hunks)mpp-vscode/webview/src/components/ModelSelector.css(1 hunks)mpp-vscode/webview/src/components/ModelSelector.tsx(1 hunks)mpp-vscode/webview/src/hooks/useVSCode.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mpp-vscode/webview/src/components/ChatInput.css
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Build MPP Core with: `cd /Volumes/source/ai/autocrud && ./gradlew :mpp-core:assembleJsPackage`
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Build and run MPP CLI with: `cd mpp-ui && npm run build && npm run start`
📚 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 mpp-ui/src/jsMain/typescript/**/*.{ts,tsx} : In CLI/TUI (TypeScript), use `mpp-ui/src/jsMain/typescript/design-system/` and import `semanticInk` / `semanticChalk`
Applied to files:
mpp-vscode/webview/src/hooks/useVSCode.ts
🧬 Code graph analysis (4)
mpp-vscode/webview/src/App.tsx (7)
mpp-vscode/webview/src/components/ModelSelector.tsx (1)
ModelConfig(11-15)mpp-vscode/src/bridge/mpp-core.ts (1)
ModelConfig(39-46)mpp-vscode/webview/src/types/timeline.ts (4)
AgentState(89-97)ToolCallInfo(13-25)ToolCallTimelineItem(60-63)TerminalOutput(27-32)mpp-vscode/webview/src/hooks/useVSCode.ts (2)
useVSCode(66-87)ExtensionMessage(15-37)mpp-vscode/src/providers/chat-view.ts (2)
postMessage(131-133)handleAction(423-471)mpp-vscode/webview/src/components/Timeline.tsx (1)
Timeline(19-56)mpp-vscode/webview/src/components/ChatInput.tsx (1)
ChatInput(18-137)
mpp-vscode/webview/src/hooks/useVSCode.ts (2)
mpp-vscode/src/providers/chat-view.ts (1)
postMessage(131-133)mpp-vscode/test/mocks/vscode.ts (1)
window(30-43)
mpp-vscode/webview/src/components/ChatInput.tsx (1)
mpp-vscode/webview/src/components/ModelSelector.tsx (2)
ModelConfig(11-15)ModelSelector(24-112)
mpp-vscode/src/providers/chat-view.ts (1)
mpp-vscode/src/services/config-manager.ts (3)
AutoDevConfigWrapper(56-106)ConfigManager(111-176)LLMConfig(18-26)
🪛 Biome (2.1.2)
mpp-vscode/src/providers/chat-view.ts
[error] 429-430: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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 (223)
- GitHub Check: Build (241)
🔇 Additional comments (2)
mpp-vscode/webview/src/components/ModelSelector.css (1)
3-121: Theme‑aware styling looks solidCSS cleanly uses VS Code theme variables (with sensible fallbacks), covers hover/selected/disabled states, and constrains long labels without obvious layout issues. No changes needed from a functionality standpoint.
mpp-vscode/src/providers/chat-view.ts (1)
199-242: Agent initialization and streaming renderer wiring look solidThe way you:
- Build
JsModelConfigfromLLMConfig,- Lazily create
JsCodingAgentwith workspace path and optional MCP servers,- And map mpp-core callbacks (
renderLLMResponseStart/Chunk/End,renderToolCall/Result,renderTaskComplete, etc.) into typed webview messages,is clean and mirrors the documented patterns from mpp-ui/AgentMode. Combined with the
isExecutinggate and error handling inhandleUserMessage, this is a good foundation for the chat loop.Also applies to: 248-301, 307-352
| private stopExecution(): void { | ||
| if (this.isExecuting) { | ||
| this.isExecuting = false; | ||
| this.postMessage({ type: 'taskComplete', data: { success: false, message: 'Stopped by user' } }); | ||
| this.log('Execution stopped by user'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the chat-view.ts file structure
fd "chat-view.ts" --type fRepository: phodal/auto-dev-sketch
Length of output: 103
🏁 Script executed:
# Get file size first
wc -l mpp-vscode/src/providers/chat-view.tsRepository: phodal/auto-dev-sketch
Length of output: 107
🏁 Script executed:
# Read the full file or relevant sections
cat -n mpp-vscode/src/providers/chat-view.ts | head -200Repository: phodal/auto-dev-sketch
Length of output: 7484
🏁 Script executed:
# Search for JsCodingAgent class definition
rg "class JsCodingAgent" -A 30Repository: phodal/auto-dev-sketch
Length of output: 3511
🏁 Script executed:
# Search for stop/cancel methods in the codebase
rg "\.stop\(\)|cancel\(\)|abort" -t ts --max-count 20Repository: phodal/auto-dev-sketch
Length of output: 225
🏁 Script executed:
# Read more of chat-view.ts to find handleUserMessage implementation
cat -n mpp-vscode/src/providers/chat-view.ts | sed -n '200,350p'Repository: phodal/auto-dev-sketch
Length of output: 6198
🏁 Script executed:
# Read the rest of the chat-view.ts file to find handleUserMessage
cat -n mpp-vscode/src/providers/chat-view.ts | sed -n '350,500p'Repository: phodal/auto-dev-sketch
Length of output: 5294
🏁 Script executed:
# Get full CodingAgentExports.kt file to check for stop/cancel methods
wc -l mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.ktRepository: phodal/auto-dev-sketch
Length of output: 136
🏁 Script executed:
# Read the CodingAgentExports.kt file to find all public methods
cat mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.ktRepository: phodal/auto-dev-sketch
Length of output: 5751
🏁 Script executed:
# Check if CodingAgent in Kotlin has any cancellation mechanism
rg "cancel|abort|coroutine.*cancel" mpp-core/src/commonMain/kotlin -t kt -A 3 --max-count 10Repository: phodal/auto-dev-sketch
Length of output: 92
🏁 Script executed:
# Look for any Job or cancellation token usage in CodingAgent
rg "class CodingAgent|fun executeTask" mpp-core/src/commonMain/kotlin -A 10 -t kt | head -60Repository: phodal/auto-dev-sketch
Length of output: 92
🏁 Script executed:
# Search for CodingAgent class definition
fd "CodingAgent.kt" -type fRepository: phodal/auto-dev-sketch
Length of output: 236
🏁 Script executed:
# Verify the exact lines of stopExecution one more time for complete accuracy
sed -n '84,90p' mpp-vscode/src/providers/chat-view.tsRepository: phodal/auto-dev-sketch
Length of output: 318
🏁 Script executed:
# Verify handleUserMessage concurrent execution path
sed -n '307,352p' mpp-vscode/src/providers/chat-view.tsRepository: phodal/auto-dev-sketch
Length of output: 1569
Race condition: stopExecution() doesn't halt the task and allows concurrent executeTask() calls
stopExecution() only resets the isExecuting flag and posts a completion message. The underlying await agent.executeTask(task) (line 338) continues running in-flight. Once isExecuting is set to false (line 86) while the task is still executing, the next handleUserMessage() call bypasses the isExecuting check (line 308) and starts a second executeTask() concurrently on the same agent instance.
Until mpp-core's JsCodingAgent exposes a real cancellation API, avoid resetting the concurrency flag in stopExecution(). If available, call the agent's stop method:
private stopExecution(): void {
- if (this.isExecuting) {
- this.isExecuting = false;
- this.postMessage({ type: 'taskComplete', data: { success: false, message: 'Stopped by user' } });
- this.log('Execution stopped by user');
- }
+ if (!this.isExecuting) {
+ return;
+ }
+
+ // TODO: Wire this through to JsCodingAgent's cancellation API if available.
+ this.codingAgent?.stop?.();
+
+ this.postMessage({
+ type: 'taskComplete',
+ data: { success: false, message: 'Stop requested by user' }
+ });
+ this.log('Stop requested by user');
}Confirm whether JsCodingAgent exposes a stop/cancel method so the Kotlin-side task can actually terminate instead of just updating UI state.
📝 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 stopExecution(): void { | |
| if (this.isExecuting) { | |
| this.isExecuting = false; | |
| this.postMessage({ type: 'taskComplete', data: { success: false, message: 'Stopped by user' } }); | |
| this.log('Execution stopped by user'); | |
| } | |
| } | |
| private stopExecution(): void { | |
| if (!this.isExecuting) { | |
| return; | |
| } | |
| // TODO: Wire this through to JsCodingAgent's cancellation API if available. | |
| this.codingAgent?.stop?.(); | |
| this.postMessage({ | |
| type: 'taskComplete', | |
| data: { success: false, message: 'Stop requested by user' } | |
| }); | |
| this.log('Stop requested by user'); | |
| } |
🤖 Prompt for AI Agents
In mpp-vscode/src/providers/chat-view.ts around lines 84 to 90, stopExecution()
currently clears the isExecuting flag which creates a race where the in-flight
await agent.executeTask(task) keeps running and subsequent handleUserMessage()
can start another execution; to fix this, do not reset isExecuting to false
inside stopExecution() unless the agent provides a real cancellation API —
instead, call agent.stop()/cancel() if JsCodingAgent exposes it (confirm its API
and invoke it here), keep isExecuting true until the running executeTask()
resolves/rejects, and only then set isExecuting = false and post the
taskComplete message; if no stop API exists, remove the isExecuting reset from
stopExecution() and rely on the task completion path to clear the flag.
- Acquire VSCode API in HTML before React loads to avoid duplicate acquisition error - Store API on window.vscodeApi for React to use - Remove debug logging from production code All 63 tests passing. Refs #31
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
mpp-vscode/webview/src/App.tsx (1)
190-197: Fix falsy value handling in iteration updates.The
||operator treats0as falsy, so if the extension sendscurrent: 0ormax: 0, these legitimate values will be ignored and previous values retained.Apply this diff to handle zero values correctly:
case 'iterationUpdate': - setAgentState(prev => ({ - ...prev, - currentIteration: Number(msg.data?.current) || prev.currentIteration, - maxIterations: Number(msg.data?.max) || prev.maxIterations - })); + setAgentState(prev => { + const current = msg.data?.current; + const max = msg.data?.max; + const parsedCurrent = current !== undefined ? Number(current) : prev.currentIteration; + const parsedMax = max !== undefined ? Number(max) : prev.maxIterations; + return { + ...prev, + currentIteration: Number.isFinite(parsedCurrent) ? parsedCurrent : prev.currentIteration, + maxIterations: Number.isFinite(parsedMax) ? parsedMax : prev.maxIterations + }; + }); break;Note: This issue was previously flagged and marked as addressed, but the fix is not present in the current code.
mpp-vscode/src/providers/chat-view.ts (5)
37-80: Track disposables to prevent resource leaks.The
onDidReceiveMessagelistener (line 54) returns aDisposablethat is not tracked and disposed when the webview is destroyed. This creates a memory leak as event handlers accumulate on webview recreation.Based on past review comments, add disposal tracking by capturing the disposable in a collection and disposing it when the webview is disposed.
85-91: Race condition:stopExecution()doesn't halt the in-flight task.The method only resets
isExecutingbut the underlyingawait agent.executeTask(task)(line 341) continues running. OnceisExecutingis set tofalsewhile the task is still executing, the nexthandleUserMessage()call can start a concurrentexecuteTask()on the same agent instance.Based on past review comments, avoid resetting the concurrency flag or verify if
JsCodingAgentexposes a cancellation API to actually terminate the task.
122-127: Guard against uninitialized webview.While the method shows the webview if it exists (line 123), it unconditionally calls
handleUserMessage(line 126), which attempts to post messages viathis.webviewView?.webview.postMessage. If the webview hasn't been resolved yet, messages will silently fail.Based on past review comments, add an early return if
this.webviewViewis undefined.
428-436: Wrap switch case declaration in a block.The
const editordeclaration (line 430) inside the switch case is not block-scoped and can be erroneously accessed by other case clauses.Based on static analysis and past review comments, wrap the case body in a block
{ ... }to restrict the declaration's scope.
495-495: CSP allows 'unsafe-inline' which weakens XSS protection.The Content Security Policy (line 495) includes
'unsafe-inline'for bothstyle-srcandscript-src, which reduces protection against XSS attacks. While the VSCode webview provides some sandboxing, tightening the CSP using nonce-based script/style loading would improve security posture.Based on past review comments, consider using nonce-based CSP:
script-src 'nonce-${nonce}'; style-src 'nonce-${nonce}'and apply the nonce to inline scripts and styles.
🧹 Nitpick comments (4)
mpp-vscode/webview/src/App.css (1)
113-135: Consider accessibility: prefers-reduced-motion for animations.The loading indicator and status dot animations run indefinitely. To improve accessibility for users with motion sensitivity, consider wrapping animations in a
prefers-reduced-motionmedia query:@media (prefers-reduced-motion: no-preference) { .loading-spinner { animation: spin 0.8s linear infinite; } .status-dot { animation: pulse 1.4s ease-in-out infinite; } } /* Fallback: no animation for users who prefer reduced motion */This ensures users with
prefers-reduced-motion: reduceset in their OS settings won't see spinning/pulsing elements.mpp-vscode/webview/src/App.tsx (1)
107-127: Consider immutable update pattern for timeline items.The code mutates the timeline item directly (lines 115-120) after creating a shallow copy. While this works because the array reference changes, a fully immutable pattern is clearer and more maintainable.
Apply this diff to use an immutable update:
case 'toolResult': if (msg.data) { setAgentState(prev => { - const timeline = [...prev.timeline]; - // Find the last tool call and update it - for (let i = timeline.length - 1; i >= 0; i--) { - if (timeline[i].type === 'tool_call') { - const item = timeline[i] as ToolCallTimelineItem; - item.toolCall = { - ...item.toolCall, - success: msg.data?.success as boolean | undefined, - output: msg.data?.output as string | undefined, - summary: msg.data?.summary as string | undefined - }; - break; - } - } - return { ...prev, timeline }; + let updatedIndex = -1; + for (let i = prev.timeline.length - 1; i >= 0; i--) { + if (prev.timeline[i].type === 'tool_call') { + updatedIndex = i; + break; + } + } + if (updatedIndex === -1) return prev; + + const item = prev.timeline[updatedIndex] as ToolCallTimelineItem; + const timeline = [ + ...prev.timeline.slice(0, updatedIndex), + { + ...item, + toolCall: { + ...item.toolCall, + success: msg.data?.success as boolean | undefined, + output: msg.data?.output as string | undefined, + summary: msg.data?.summary as string | undefined + } + }, + ...prev.timeline.slice(updatedIndex + 1) + ]; + return { ...prev, timeline }; }); } break;mpp-vscode/src/providers/chat-view.ts (2)
301-305: Consider adding TODO comments for empty renderer methods.The
renderUserConfirmationRequestandaddLiveTerminalmethods have empty implementations. If these are intentionally unused in the VSCode context, consider adding brief TODO or explanatory comments.Apply this diff to add clarity:
- renderUserConfirmationRequest: () => {}, - addLiveTerminal: () => {}, + renderUserConfirmationRequest: () => { + // TODO: Implement user confirmation UI if needed + }, + addLiveTerminal: () => { + // Not applicable in VSCode webview context + },
575-582: Use cryptographically secure random for nonce generation.The nonce is generated using
Math.random()(line 579), which is not cryptographically secure and predictable. For security-sensitive tokens like CSP nonces, use Node.js'scryptomodule.Apply this diff to use secure random generation:
+import * as crypto from 'crypto'; + private getNonce(): string { - let text = ''; - const possible = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789'; - for (let i = 0; i < 32; i++) { - text += possible.charAt(Math.floor(Math.random() * possible.length)); - } - return text; + return crypto.randomBytes(16).toString('base64'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mpp-vscode/src/providers/chat-view.ts(1 hunks)mpp-vscode/webview/src/App.css(1 hunks)mpp-vscode/webview/src/App.tsx(1 hunks)mpp-vscode/webview/src/hooks/useVSCode.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mpp-vscode/webview/src/hooks/useVSCode.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Build MPP Core with: `cd /Volumes/source/ai/autocrud && ./gradlew :mpp-core:assembleJsPackage`
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Build and run MPP CLI with: `cd mpp-ui && npm run build && npm run start`
🧬 Code graph analysis (2)
mpp-vscode/src/providers/chat-view.ts (1)
mpp-vscode/src/services/config-manager.ts (3)
AutoDevConfigWrapper(56-106)ConfigManager(111-176)LLMConfig(18-26)
mpp-vscode/webview/src/App.tsx (4)
mpp-vscode/webview/src/types/timeline.ts (4)
AgentState(89-97)ToolCallInfo(13-25)ToolCallTimelineItem(60-63)TerminalOutput(27-32)mpp-vscode/webview/src/hooks/useVSCode.ts (2)
useVSCode(81-100)ExtensionMessage(20-42)mpp-vscode/webview/src/components/Timeline.tsx (1)
Timeline(19-56)mpp-vscode/webview/src/components/ChatInput.tsx (1)
ChatInput(18-137)
🪛 Biome (2.1.2)
mpp-vscode/src/providers/chat-view.ts
[error] 430-431: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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 (4)
mpp-vscode/webview/src/App.css (2)
39-50: Status indicator color: verify yellow is intentional for loading state.Line 42 uses
--vscode-charts-yellowfor the status dot, which renders as yellow/amber. For a VSCode extension, consider whether this is the right semantic choice:
- Yellow typically indicates "loading" or "in-progress" (appropriate)
- Alternatively, consider
--vscode-charts-greenfor "connected" or--vscode-errorForegroundfor "error" statesIf the UI has multiple states (connecting, ready, error), ensure the color aligns with user expectations.
Can you confirm the status dot is only used for the "loading" state, or if it represents multiple connection states?
1-135: CSS structure and performance: solid implementation.The overall CSS is well-organized and performant:
- ✓ Animations use efficient properties (
transform,opacity) — no layout thrashing- ✓ VSCode theme variables consistently applied with reasonable fallbacks
- ✓ Flexbox layout is clean and semantic
- ✓ Interactive elements have proper hover states
- ✓ No specificity issues or unnecessary nesting
mpp-vscode/webview/src/App.tsx (2)
1-38: LGTM! Clean setup and state initialization.The imports, type definitions, and initial state are well-structured and align with the Timeline-based architecture described in the PR objectives.
263-327: LGTM! Well-structured UI composition.The render logic correctly coordinates the Timeline, loading states, config prompts, and ChatInput. The conditional rendering of
currentStreamingContent(line 292) properly gates onisProcessing, and all user action handlers are cleanly wired.
| private clearHistory(): void { | ||
| this.messages = []; | ||
| this.codingAgent?.clearHistory(); | ||
| this.postMessage({ type: 'historyCleared' }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find all Kotlin files in mpp-core related to CodingAgent and exports
find . -type f -name "*.kt" | grep -i agent | head -20Repository: phodal/auto-dev-sketch
Length of output: 1530
🏁 Script executed:
# Search for CodingAgent class and clearHistory method
rg -n "class.*CodingAgent|fun clearHistory" --type kt -A 3Repository: phodal/auto-dev-sketch
Length of output: 92
🏁 Script executed:
# Look specifically in jsMain for JsCodingAgent or similar exports
rg -n "JsCodingAgent|clearHistory" mpp-core/src/jsMain -A 2 -B 2 --type ktRepository: phodal/auto-dev-sketch
Length of output: 92
🏁 Script executed:
# List the mpp-core directory structure
find mpp-core -type f -name "*.kt" | head -20Repository: phodal/auto-dev-sketch
Length of output: 1687
🏁 Script executed:
# Search for JsCodingAgent and clearHistory without type restrictions
rg -n "JsCodingAgent|fun clearHistory" mpp-core/ -A 2 -B 2Repository: phodal/auto-dev-sketch
Length of output: 4613
🏁 Script executed:
# Search more broadly for CodingAgent class definitions
rg -n "class.*CodingAgent" --max-count 20Repository: phodal/auto-dev-sketch
Length of output: 2297
🏁 Script executed:
# Check the jsMain directory structure
find mpp-core -type d -name jsMain | xargs ls -laRepository: phodal/auto-dev-sketch
Length of output: 232
🏁 Script executed:
# Read the JsCodingAgent class implementation to find clearHistory method
sed -n '92,200p' mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.ktRepository: phodal/auto-dev-sketch
Length of output: 3482
🏁 Script executed:
# Search for clearHistory in the CodingAgentExports file
rg -n "clearHistory" mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.kt -A 3 -B 3Repository: phodal/auto-dev-sketch
Length of output: 48
🏁 Script executed:
# Check if JsCodingAgent wraps or delegates to another agent that has clearHistory
rg -n "fun clearHistory|clearHistory()" mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/ -A 2 -B 2Repository: phodal/auto-dev-sketch
Length of output: 48
🏁 Script executed:
# Get the full CodingAgentExports.kt file
cat mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.kt | wc -lRepository: phodal/auto-dev-sketch
Length of output: 69
🏁 Script executed:
# Read the entire CodingAgentExports.kt to check for clearHistory
cat mpp-core/src/jsMain/kotlin/cc/unitmesh/agent/CodingAgentExports.ktRepository: phodal/auto-dev-sketch
Length of output: 5751
🏁 Script executed:
# Check if the underlying CodingAgent (Kotlin class) has clearHistory method
rg -n "fun clearHistory" mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgent.kt -A 3Repository: phodal/auto-dev-sketch
Length of output: 48
🏁 Script executed:
# Read ConversationManager around the clearHistory method
sed -n '80,95p' mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/conversation/ConversationManager.ktRepository: phodal/auto-dev-sketch
Length of output: 483
🏁 Script executed:
# Check if CodingAgent class delegates to ConversationManager
rg -n "conversationManager|ConversationManager" mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgent.kt -A 2 -B 2Repository: phodal/auto-dev-sketch
Length of output: 48
🏁 Script executed:
# Get full CodingAgent class to see available methods
head -80 mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgent.ktRepository: phodal/auto-dev-sketch
Length of output: 3452
🏁 Script executed:
# Search for clearHistory in CodingAgent.kt
rg -n "clearHistory" mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/CodingAgent.ktRepository: phodal/auto-dev-sketch
Length of output: 48
🏁 Script executed:
# Search for MainAgent definition and its methods
rg -n "class MainAgent|fun clearHistory" mpp-core/src/commonMain/kotlin/cc/unitmesh/agent/core/MainAgent.kt -A 1Repository: phodal/auto-dev-sketch
Length of output: 163
🏁 Script executed:
# Search for any clearHistory usage in the codebase
rg -n "\.clearHistory\(\)" --max-count 10Repository: phodal/auto-dev-sketch
Length of output: 733
🏁 Script executed:
# Check the TypeScript bridge definition for CodingAgent
cat mpp-vscode/src/bridge/mpp-core.ts | grep -A 20 "class CodingAgent"Repository: phodal/auto-dev-sketch
Length of output: 621
🏁 Script executed:
# Check the test file to see how clearHistory is called on the service
sed -n '105,115p' mpp-vscode/test/bridge/mpp-core.test.tsRepository: phodal/auto-dev-sketch
Length of output: 338
🏁 Script executed:
# Search for clearHistory in the TypeScript bridge CodingAgent
rg -n "clearHistory" mpp-vscode/src/bridge/mpp-core.ts -B 2 -A 2Repository: phodal/auto-dev-sketch
Length of output: 299
🏁 Script executed:
# Get more methods from the CodingAgent bridge class
sed -n '424,550p' mpp-vscode/src/bridge/mpp-core.tsRepository: phodal/auto-dev-sketch
Length of output: 3990
Add clearHistory() method to JsCodingAgent class.
The TypeScript bridge calls this.agent.clearHistory?.() (line 532 in mpp-core.ts), but JsCodingAgent does not expose this method. The optional chaining silently fails—the conversation history is never actually cleared. Expose clearHistory() in JsCodingAgent to delegate to the internal CodingAgent instance, similar to how getConversationHistory() is exposed.
🤖 Prompt for AI Agents
In mpp-vscode/src/providers/chat-view.ts around lines 474 to 478, the
clearHistory() implementation here clears the view-level messages but the
TypeScript bridge expects a clearHistory() method on JsCodingAgent that
delegates to the internal CodingAgent; add a public clearHistory(): void method
to JsCodingAgent that calls this.codingAgent.clearHistory() (or
this.codingAgent?.clearHistory() if the internal agent can be undefined) so the
bridge's this.agent.clearHistory?.() actually clears the underlying conversation
history, mirroring how getConversationHistory() is exposed.
| const needsConfig = agentState.timeline.length === 0 && | ||
| agentState.currentStreamingContent.includes('No configuration found') || | ||
| agentState.currentStreamingContent.includes('Configuration Required'); |
There was a problem hiding this comment.
Clarify operator precedence in config prompt condition.
The condition evaluates as:
(agentState.timeline.length === 0 && currentStreamingContent.includes('No configuration found'))
||
currentStreamingContent.includes('Configuration Required')
This means the config prompt appears whenever "Configuration Required" is in the streaming content, regardless of timeline length. If both checks should only apply to empty timelines, add explicit parentheses:
- const needsConfig = agentState.timeline.length === 0 &&
- agentState.currentStreamingContent.includes('No configuration found') ||
- agentState.currentStreamingContent.includes('Configuration Required');
+ const needsConfig = agentState.timeline.length === 0 && (
+ agentState.currentStreamingContent.includes('No configuration found') ||
+ agentState.currentStreamingContent.includes('Configuration Required')
+ );If the current behavior is intentional (showing config prompt for "Configuration Required" at any time), consider adding a comment to clarify.
🤖 Prompt for AI Agents
In mpp-vscode/webview/src/App.tsx around lines 259 to 261, the boolean
expression lacks explicit parentheses so the "Configuration Required" check runs
regardless of timeline length; to fix, add parentheses so both string checks are
combined with the timeline emptiness check (i.e., require
agentState.timeline.length === 0 AND (includes 'No configuration found' OR
includes 'Configuration Required')), or if the current behavior is intended, add
a concise comment explaining that "Configuration Required" should trigger the
prompt even when the timeline is not empty.
User message was being added twice - once in handleSend for immediate feedback and again when receiving userMessage from extension. Refs #31
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
mpp-vscode/webview/src/App.tsx (2)
183-190:iterationUpdateuses||and will ignore legitimate zero values.This was flagged in a previous review and remains unaddressed. Using
Number(...) || prev.xxxtreats0as falsy and preserves the previous value when0may be valid.Use nullish coalescing or explicit undefined checks:
case 'iterationUpdate': setAgentState(prev => ({ ...prev, - currentIteration: Number(msg.data?.current) || prev.currentIteration, - maxIterations: Number(msg.data?.max) || prev.maxIterations + currentIteration: msg.data?.current !== undefined + ? Number(msg.data.current) + : prev.currentIteration, + maxIterations: msg.data?.max !== undefined + ? Number(msg.data.max) + : prev.maxIterations })); break;
251-254: Clarify operator precedence in config prompt condition.This was flagged in a previous review. Due to operator precedence,
&&binds tighter than||, so the condition reads as:(timeline.length === 0 && includes('No configuration found')) || includes('Configuration Required')If both checks should require an empty timeline, add explicit parentheses.
- const needsConfig = agentState.timeline.length === 0 && - agentState.currentStreamingContent.includes('No configuration found') || - agentState.currentStreamingContent.includes('Configuration Required'); + const needsConfig = agentState.timeline.length === 0 && ( + agentState.currentStreamingContent.includes('No configuration found') || + agentState.currentStreamingContent.includes('Configuration Required') + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mpp-vscode/webview/src/App.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Build MPP Core with: `cd /Volumes/source/ai/autocrud && ./gradlew :mpp-core:assembleJsPackage`
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Build and run MPP CLI with: `cd mpp-ui && npm run build && npm run start`
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 development, use `cd mpp-idea && ../gradlew compileKotlin` instead of `./gradlew :mpp-idea:compileKotlin`
⏰ 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 (2)
mpp-vscode/webview/src/App.tsx (2)
204-249: LGTM!The message subscription lifecycle is correctly managed with cleanup, and the handler callbacks appropriately delegate to the extension via
postMessage. The optimistic UI update inhandleSendpaired with theuserMessageskip prevents duplicates.
256-320: LGTM!The component structure is well-organized with clear separation between header, content area, and input. Conditional rendering for loading states and config prompts is appropriate.
| case 'toolResult': | ||
| if (msg.data) { | ||
| setAgentState(prev => { | ||
| const timeline = [...prev.timeline]; | ||
| // Find the last tool call and update it | ||
| for (let i = timeline.length - 1; i >= 0; i--) { | ||
| if (timeline[i].type === 'tool_call') { | ||
| const item = timeline[i] as ToolCallTimelineItem; | ||
| item.toolCall = { | ||
| ...item.toolCall, | ||
| success: msg.data?.success as boolean | undefined, | ||
| output: msg.data?.output as string | undefined, | ||
| summary: msg.data?.summary as string | undefined | ||
| }; | ||
| break; | ||
| } | ||
| } | ||
| return { ...prev, timeline }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Direct mutation of state in toolResult handler.
The shallow copy [...prev.timeline] does not clone the nested objects. Assigning to item.toolCall mutates the original timeline item in prev, which can cause React to miss re-renders or produce stale closures.
Apply immutable update:
case 'toolResult':
if (msg.data) {
setAgentState(prev => {
- const timeline = [...prev.timeline];
- // Find the last tool call and update it
- for (let i = timeline.length - 1; i >= 0; i--) {
- if (timeline[i].type === 'tool_call') {
- const item = timeline[i] as ToolCallTimelineItem;
- item.toolCall = {
- ...item.toolCall,
- success: msg.data?.success as boolean | undefined,
- output: msg.data?.output as string | undefined,
- summary: msg.data?.summary as string | undefined
- };
- break;
+ const lastToolIdx = prev.timeline.findLastIndex(item => item.type === 'tool_call');
+ if (lastToolIdx === -1) return prev;
+
+ const timeline = prev.timeline.map((item, idx) => {
+ if (idx !== lastToolIdx) return item;
+ const toolItem = item as ToolCallTimelineItem;
+ return {
+ ...toolItem,
+ toolCall: {
+ ...toolItem.toolCall,
+ success: msg.data?.success as boolean | undefined,
+ output: msg.data?.output as string | undefined,
+ summary: msg.data?.summary as string | undefined
+ }
+ };
- }
- }
+ });
return { ...prev, timeline };
});
}
break;🤖 Prompt for AI Agents
In mpp-vscode/webview/src/App.tsx around lines 100 to 119, the handler for
'toolResult' mutates nested state by copying only the timeline array and
directly assigning to item.toolCall; change this to an immutable update: inside
setAgentState use prev.timeline.map to produce a new timeline array, and for the
matching last 'tool_call' return a new timeline item object that spreads the
original item and sets toolCall to a new object that spreads the original
item.toolCall and applies success/output/summary; return the updated state with
the new timeline so no nested objects from prev are mutated and React can
correctly detect changes.
- Fix repository URL from phodal/auto-dev to phodal/auto-dev-sketch - Remove unused colors variable in status-bar.ts - Add platform detection for welcome message keybinding (Cmd vs Ctrl)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mpp-vscode/src/services/status-bar.ts (1)
46-55: Consider tracking/cancelling message timeout for robustness
showMessageschedules asetTimeoutbut doesn’t keep or clear the handle. This is fine for most cases, but it means:
- Messages can still schedule a revert after
dispose()is called.- Multiple
showMessagecalls on the same state will all schedule reverts, with only timing determining which wins.Not a blocker, but you might want to store the timeout handle and clear it on subsequent
showMessagecalls or indispose()for tighter lifecycle control.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mpp-vscode/package.json(1 hunks)mpp-vscode/src/extension.ts(1 hunks)mpp-vscode/src/services/status-bar.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- mpp-vscode/package.json
- mpp-vscode/src/extension.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Build MPP Core with: `cd /Volumes/source/ai/autocrud && ./gradlew :mpp-core:assembleJsPackage`
Learnt from: CR
Repo: phodal/auto-dev-sketch PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-02T00:20:34.480Z
Learning: Build and run MPP CLI with: `cd mpp-ui && npm run build && npm run start`
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 development, use `cd mpp-idea && ../gradlew compileKotlin` instead of `./gradlew :mpp-idea:compileKotlin`
📚 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,mpp-ui/src/jsMain/typescript/**/*.{ts,tsx}} : DO NOT hardcode colors (e.g., `Color(0xFF...)` or `#hex`). Always use design tokens for consistency across platforms
Applied to files:
mpp-vscode/src/services/status-bar.ts
⏰ 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 (223)
- GitHub Check: Build (241)
🔇 Additional comments (2)
mpp-vscode/src/services/status-bar.ts (2)
65-119: Status bar state machine and animation logic look solidIcon selection, error coloring via
ThemeColor, and the animation frame cycling withstartAnimation/stopAnimationare cohesive and avoid timer leaks as long assetStateis the only caller. The state‑driven text defaults are clear and match expected AutoDev states.
18-27: No action required—command id is correctly registeredThe
'autodev.chat'command referenced in the status bar constructor is properly contributed inmpp-vscode/package.json(line 33) and will function as expected. The command is also bound to keybindings.
Summary
Initialize the mpp-vscode project - a VSCode extension based on Kotlin Multiplatform (KMP) technology.
Changes
package.jsonwith VSCode extension configurationtsconfig.jsonfor TypeScriptREADME.mdwith TODO list and architecture overviewArchitecture
The extension follows a lightweight architecture inspired by:
TODO (tracked in #31)
Closes #31
Pull Request opened by Augment Code with guidance from the PR author
Summary by CodeRabbit
New Features
Documentation
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.