【WIP】feat(vscode): add VSCode extension for AI code review#95
【WIP】feat(vscode): add VSCode extension for AI code review#95nigulasikk wants to merge 3 commits into
Conversation
|
墨失 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 OpenCodeReview found 53 issue(s) in this PR.
- ✅ 50 posted as inline comment(s)
- 📝 3 posted as summary
📄 extensions/vscode/src/extension/services/GitService.ts
Performance: Sequential independent async operations. The calls to repo.getBranches(), repo.log(), and runGit(status) are independent and each has its own error handling. They can be parallelized using Promise.allSettled to reduce overall latency, especially on large repositories.
📄 extensions/vscode/src/webview/views/IdleView.tsx
Missing dependency in useEffect hooks: onRequestModeFiles is used inside both useEffect callbacks but is not included in their dependency arrays. In the parent component (App.tsx), requestModeFiles is defined as a plain function (not wrapped in useCallback), so it is recreated on every render. This means these effects may capture a stale closure of onRequestModeFiles, potentially leading to missed or incorrect file-list requests.
Either add onRequestModeFiles to the dependency arrays, or stabilize it in the parent with useCallback.
💡 Suggested Change
Before:
useEffect(() => {
if (mode === 'branch' && from && to) onRequestModeFiles('branch', from, to);
}, [mode, from, to]);
// 选中某 commit 后,拉取该 commit 文件列表
useEffect(() => {
if (mode === 'commit' && commit) onRequestModeFiles('commit', undefined, undefined, commit);
}, [mode, commit]);
After:
useEffect(() => {
if (mode === 'branch' && from && to) onRequestModeFiles('branch', from, to);
}, [mode, from, to, onRequestModeFiles]);
// 选中某 commit 后,拉取该 commit 文件列表
useEffect(() => {
if (mode === 'commit' && commit) onRequestModeFiles('commit', undefined, undefined, commit);
}, [mode, commit, onRequestModeFiles]);
📄 extensions/vscode/webpack.config.js
Production build concern: devtool: 'source-map' is hardcoded for both configs regardless of build mode. While .vscodeignore excludes *.map files from the published package, production builds (yarn build) still generate full source maps unnecessarily, increasing build time and disk usage.
Consider making this conditional based on the mode:
devtool: process.env.NODE_ENV === 'production' ? false : 'source-map',Or accept the --mode value via webpack's env / argv parameter to differentiate.
| .superpowers/ | ||
| node_modules/ | ||
| reference | ||
| out/ No newline at end of file |
There was a problem hiding this comment.
Missing newline at end of file. Most text editors and tools expect files to end with a newline character. Without it, some tools may produce warnings or unexpected behavior when concatenating files.
Suggestion:
| out/ | |
| out/ |
| reg(COMMANDS.commentApply, (arg: vscode.CommentThread | number) => { | ||
| const i = idxOf(arg); | ||
| if (i >= 0) comments.apply(i); | ||
| }); |
There was a problem hiding this comment.
Unhandled async rejection: comments.apply(i) is an async method that returns Promise<void>. If it throws (e.g., file I/O error, workspace edit failure), the rejection will be unhandled. You should either await the result or attach a .catch() handler to surface errors to the user.
Also, the three command registrations share identical structure (extract index → guard → call method). Consider extracting this into a helper to reduce duplication.
Suggestion:
| reg(COMMANDS.commentApply, (arg: vscode.CommentThread | number) => { | |
| const i = idxOf(arg); | |
| if (i >= 0) comments.apply(i); | |
| }); | |
| const withIndex = (fn: (i: number) => void | Promise<void>) => | |
| (arg: vscode.CommentThread | number) => { | |
| const i = idxOf(arg); | |
| if (i >= 0) Promise.resolve(fn(i)).catch((err) => | |
| vscode.window.showErrorMessage(`Command failed: ${err}`)); | |
| }; | |
| reg(COMMANDS.commentApply, withIndex((i) => comments.apply(i))); |
| result.views = result.views || {}; | ||
| if (ocr.views && ocr.views[OCR_VIEW_CONTAINER]) { | ||
| result.views[OCR_VIEW_CONTAINER] = ocr.views[OCR_VIEW_CONTAINER]; | ||
| } |
There was a problem hiding this comment.
Potential issue: Unlike the commands and menus sections which filter out all old ocr.* entries before re-adding, the views section only replaces ocr-container if it exists in the new ocr.views. If a future version of the OCR package removes or renames ocr-container, the stale views from a previous merge will persist in result.views[OCR_VIEW_CONTAINER] with no cleanup.
Consider adding a cleanup step similar to how commands/menus are handled, e.g. always deleting the old ocr-container key before conditionally re-adding:
result.views = result.views || {};
delete result.views[OCR_VIEW_CONTAINER];
if (ocr.views && ocr.views[OCR_VIEW_CONTAINER]) {
result.views[OCR_VIEW_CONTAINER] = ocr.views[OCR_VIEW_CONTAINER];
}This ensures idempotent behavior consistent with the other sections.
| tsconfig*.json | ||
| webpack.config.js | ||
| jest.config.js | ||
| .gitignore |
There was a problem hiding this comment.
The file prototype.html exists at the extension root and does not appear to be referenced by any runtime code. It should be added to .vscodeignore to avoid including unnecessary development artifacts in the published extension package.
Suggestion:
| .gitignore | |
| .gitignore | |
| prototype.html |
| const reg = (id: string, fn: (...args: any[]) => any) => | ||
| subs.push(vscode.commands.registerCommand(id, fn)); |
There was a problem hiding this comment.
Avoid any type: The callback parameter uses any. Since the registered commands all accept vscode.CommentThread | number, you can use a more specific type here for better type safety.
Suggestion:
| const reg = (id: string, fn: (...args: any[]) => any) => | |
| subs.push(vscode.commands.registerCommand(id, fn)); | |
| const reg = (id: string, fn: (arg: vscode.CommentThread | number) => void | Promise<void>) => | |
| subs.push(vscode.commands.registerCommand(id, fn)); |
| ...state, | ||
| logs: starting ? [] : state.logs, | ||
| commentStatus: starting ? {} : state.commentStatus, | ||
| session: { state: msg.state, result: starting ? null : state.session.result }, |
There was a problem hiding this comment.
Bug: When a review fails (exception thrown in ReviewSession.run) or is cancelled, onDone is never called, so no reviewDone message is sent. This means state.session.result retains the stale result from a previous successful run.
While the current UI guards against displaying this (state.view === 'done' && state.session.result), the stale data in state is a latent correctness issue. For example, if future code reads session.result while in a failed or cancelled view, it would get incorrect data.
Consider clearing the result for all non-running terminal transitions, or at least for states where reviewDone will not follow:
Suggestion:
| session: { state: msg.state, result: starting ? null : state.session.result }, | |
| session: { state: msg.state, result: (starting || msg.state === 'failed' || msg.state === 'cancelled') ? null : state.session.result }, |
| const selectionReady = | ||
| mode === 'workspace' || (mode === 'branch' && !!from && !!to) || (mode === 'commit' && !!commit); |
There was a problem hiding this comment.
Duplicate logic: selectionReady is identical to willRequest defined two lines above. Having two variables with different names for the exact same expression increases maintenance burden and risks divergence if one is updated without the other. Consider reusing willRequest directly, or if the semantic distinction is intentional, add a comment explaining why they might diverge in the future.
Suggestion:
| const selectionReady = | |
| mode === 'workspace' || (mode === 'branch' && !!from && !!to) || (mode === 'commit' && !!commit); | |
| const selectionReady = willRequest; |
| {!configured ? '请先配置模型' | ||
| : running ? '审查中…' | ||
| : !selectionReady ? (mode === 'branch' ? '请选择对比分支' : '请选择提交') | ||
| : files.length === 0 ? '无可审查文件' | ||
| : '审查所有变更'} |
There was a problem hiding this comment.
Nested ternary expression: The button label uses a nested ternary (mode === 'branch' ? ... : ... inside the outer ternary chain), which violates the no-nested-ternary rule and reduces readability. Consider extracting this into a helper function or using an if/else block to determine the label text.
Suggestion:
| {!configured ? '请先配置模型' | |
| : running ? '审查中…' | |
| : !selectionReady ? (mode === 'branch' ? '请选择对比分支' : '请选择提交') | |
| : files.length === 0 ? '无可审查文件' | |
| : '审查所有变更'} | |
| {getButtonLabel(configured, running, selectionReady, mode, files.length)} |
| exclude: /node_modules/, | ||
| use: { loader: 'ts-loader', options: { configFile: 'tsconfig.webview.json' } }, | ||
| }, | ||
| { test: /\.css$/, use: ['style-loader', 'css-loader'] }, |
There was a problem hiding this comment.
Security concern: Using style-loader injects CSS via inline <style> tags at runtime, which requires 'unsafe-inline' in the Content-Security-Policy (as seen in SidebarProvider.ts). This weakens the CSP and could allow style-based injection attacks.
Consider using mini-css-extract-plugin instead to extract CSS into a separate .css file. You can then load it via a <link> tag in the webview HTML and remove 'unsafe-inline' from the style-src CSP directive, resulting in a stronger security posture.
Additionally, style-loader adds runtime overhead (JS bundle size increases due to embedded CSS + injection logic), whereas an extracted CSS file loads more efficiently.
| onSave(entries); | ||
| }; |
There was a problem hiding this comment.
Race condition in config save flow: The onSave callback (wired in App.tsx) sends each entry as a separate setConfig message via entries.forEach(...). Each setConfig on the extension side is async (CLI call + config read) and posts back a config response. The store reducer closes the modal (configOpen: false) upon receiving the first config response with non-null config.
This means:
- The modal closes after only the first config entry is confirmed saved, giving no feedback that all entries were persisted.
- Concurrent CLI
config setcalls may cause file write conflicts in~/.opencodereview/config.json. - There's no error handling if any individual
setConfigfails.
Consider either:
- Adding a batch
setConfigsmessage type that saves all entries atomically before posting back a single response, or - Not auto-closing the modal on
configmessages during save, and instead having theonSavecallback explicitly close it after all entries are confirmed.
| .superpowers/ | ||
| node_modules/ | ||
| reference | ||
| out/ No newline at end of file |
There was a problem hiding this comment.
Missing newline at end of file. Most text editors and tools expect files to end with a newline character. Without it, some tools may produce warnings or unexpected behavior when concatenating files.
Suggestion:
| out/ | |
| out/ |
| reg(COMMANDS.commentApply, (arg: vscode.CommentThread | number) => { | ||
| const i = idxOf(arg); | ||
| if (i >= 0) comments.apply(i); | ||
| }); |
There was a problem hiding this comment.
Unhandled async rejection: comments.apply(i) is an async method that returns Promise<void>. If it throws (e.g., file I/O error, workspace edit failure), the rejection will be unhandled. You should either await the result or attach a .catch() handler to surface errors to the user.
Also, the three command registrations share identical structure (extract index → guard → call method). Consider extracting this into a helper to reduce duplication.
Suggestion:
| reg(COMMANDS.commentApply, (arg: vscode.CommentThread | number) => { | |
| const i = idxOf(arg); | |
| if (i >= 0) comments.apply(i); | |
| }); | |
| const withIndex = (fn: (i: number) => void | Promise<void>) => | |
| (arg: vscode.CommentThread | number) => { | |
| const i = idxOf(arg); | |
| if (i >= 0) Promise.resolve(fn(i)).catch((err) => | |
| vscode.window.showErrorMessage(`Command failed: ${err}`)); | |
| }; | |
| reg(COMMANDS.commentApply, withIndex((i) => comments.apply(i))); |
| result.views = result.views || {}; | ||
| if (ocr.views && ocr.views[OCR_VIEW_CONTAINER]) { | ||
| result.views[OCR_VIEW_CONTAINER] = ocr.views[OCR_VIEW_CONTAINER]; | ||
| } |
There was a problem hiding this comment.
Potential issue: Unlike the commands and menus sections which filter out all old ocr.* entries before re-adding, the views section only replaces ocr-container if it exists in the new ocr.views. If a future version of the OCR package removes or renames ocr-container, the stale views from a previous merge will persist in result.views[OCR_VIEW_CONTAINER] with no cleanup.
Consider adding a cleanup step similar to how commands/menus are handled, e.g. always deleting the old ocr-container key before conditionally re-adding:
result.views = result.views || {};
delete result.views[OCR_VIEW_CONTAINER];
if (ocr.views && ocr.views[OCR_VIEW_CONTAINER]) {
result.views[OCR_VIEW_CONTAINER] = ocr.views[OCR_VIEW_CONTAINER];
}This ensures idempotent behavior consistent with the other sections.
| tsconfig*.json | ||
| webpack.config.js | ||
| jest.config.js | ||
| .gitignore |
There was a problem hiding this comment.
The file prototype.html exists at the extension root and does not appear to be referenced by any runtime code. It should be added to .vscodeignore to avoid including unnecessary development artifacts in the published extension package.
Suggestion:
| .gitignore | |
| .gitignore | |
| prototype.html |
| const reg = (id: string, fn: (...args: any[]) => any) => | ||
| subs.push(vscode.commands.registerCommand(id, fn)); |
There was a problem hiding this comment.
Avoid any type: The callback parameter uses any. Since the registered commands all accept vscode.CommentThread | number, you can use a more specific type here for better type safety.
Suggestion:
| const reg = (id: string, fn: (...args: any[]) => any) => | |
| subs.push(vscode.commands.registerCommand(id, fn)); | |
| const reg = (id: string, fn: (arg: vscode.CommentThread | number) => void | Promise<void>) => | |
| subs.push(vscode.commands.registerCommand(id, fn)); |
| transform: { | ||
| '^.+\\.tsx?$': ['ts-jest', { isolatedModules: true, tsconfig: 'tsconfig.extension.json' }], | ||
| }, |
There was a problem hiding this comment.
The transform is configured to use tsconfig.extension.json for all .ts/.tsx files, but this tsconfig only includes src/extension/**/* and src/shared/**/*. It does not cover src/webview/**/*.
While isolatedModules: true means ts-jest won't enforce include/exclude, the compilerOptions here (e.g., "module": "commonjs", no jsx setting) are designed for extension code. This could cause compilation issues for webview test files like src/webview/__tests__/store.test.ts that import webview modules using JSX/Preact.
Consider either:
- Using separate transform configurations for extension vs. webview test files via
projectsor multiple transform entries. - Creating a dedicated
tsconfig.test.jsonthat includes all source directories with appropriate compiler options.
| function openFile(file, line) { | ||
| var cards = _getCommentCards(); | ||
| var clickedCard = event ? event.target.closest('.comment-card') : null; | ||
| _currentReviewIdx = clickedCard ? cards.indexOf(clickedCard) : 0; |
There was a problem hiding this comment.
Bug: The openFile function relies on the implicit global window.event object to determine which comment card was clicked. This is deprecated and unreliable:
- In strict mode or certain browsers,
eventmay be undefined, causing aReferenceError. - When
openFileis called programmatically (e.g., fromapplyAndNext→_showReviewPanelflow), there is no user event, soeventwill be stale or null, leading to incorrect_currentReviewIdxcalculation.
The inline onclick="openFile('src/api.ts', 42)" handlers don't pass this or the event, so there's no way to identify the originating card. Consider passing this from the onclick handler and using it to find the closest .comment-card, or refactor to use event delegation.
Suggestion:
| function openFile(file, line) { | |
| var cards = _getCommentCards(); | |
| var clickedCard = event ? event.target.closest('.comment-card') : null; | |
| _currentReviewIdx = clickedCard ? cards.indexOf(clickedCard) : 0; | |
| function openFile(file, line, btn) { | |
| var cards = _getCommentCards(); | |
| var clickedCard = btn ? btn.closest('.comment-card') : null; | |
| _currentReviewIdx = clickedCard ? cards.indexOf(clickedCard) : 0; |
| <!-- | ||
| OCR VSCode 插件 UI 原型 | ||
| ======================== | ||
| 双击此文件在浏览器中打开即可查看。 |
There was a problem hiding this comment.
The prototype.html file is not listed in .vscodeignore, which means it will be included when the VSCode extension is packaged and published. Since this is a debug/prototype artifact (~1800 lines with hardcoded mock data and API keys placeholders), it should be excluded from production builds. Add prototype.html to .vscodeignore.
| const args = process.argv.slice(2); | ||
| const out = { target: '../aone-copilot-vscode/src/ocr' }; | ||
| for (let i = 0; i < args.length; i++) { | ||
| if (args[i] === '--target') out.target = args[++i]; |
There was a problem hiding this comment.
Bug (High): Missing argument validation for --target flag.
If --target is passed as the last argument without a value (e.g., node sync-to-aone.js --target), args[++i] returns undefined, and out.target becomes undefined. This causes path.resolve(repoRoot, undefined) to resolve to repoRoot itself, and then fs.rmSync(targetDir, { recursive: true, force: true }) on line 34 would recursively delete the entire extension source directory.
Add validation to ensure a value follows --target:
Suggestion:
| if (args[i] === '--target') out.target = args[++i]; | |
| if (args[i] === '--target') { | |
| if (i + 1 >= args.length) { | |
| console.error('Error: --target requires a value'); | |
| process.exit(1); | |
| } | |
| out.target = args[++i]; | |
| } |
|
|
||
| // 3. merge contributes 到 aone package.json | ||
| const ocrPkg = JSON.parse(fs.readFileSync(path.join(repoRoot, 'package.json'), 'utf8')); | ||
| const aonePkgPath = path.resolve(targetDir, '..', '..', 'package.json'); // aone 根 package.json |
There was a problem hiding this comment.
Issue (Medium): Hardcoded relative path assumption for aonePkgPath.
The path path.resolve(targetDir, '..', '..', 'package.json') assumes the target always has exactly two parent directories before reaching the aone root package.json. If a user passes a custom --target with a different depth (which the CLI supports), this will read/write the wrong package.json or crash.
Consider either:
- Adding a separate
--aone-pkgCLI flag to explicitly specify the aone package.json path, or - Validating that the resolved
aonePkgPathactually exists and contains expected fields before writing to it.
| disposables.push(viewReg, cmdReg, comments, output); | ||
| context.subscriptions.push(...disposables); |
There was a problem hiding this comment.
Double disposal bug: Disposables are pushed into both the local disposables array and context.subscriptions. When deactivateOcr() runs, it manually disposes all items. Then when VS Code tears down the extension context, it will dispose everything in context.subscriptions again, causing double-disposal of viewReg, cmdReg, comments, and output.
You should either:
- Only use
context.subscriptionsand let VS Code handle cleanup (removedeactivateOcrlogic), or - Only use the local
disposablesarray and NOT push them intocontext.subscriptions.
The idiomatic VS Code approach is to rely solely on context.subscriptions.
Suggestion:
| disposables.push(viewReg, cmdReg, comments, output); | |
| context.subscriptions.push(...disposables); | |
| context.subscriptions.push(viewReg, cmdReg, comments, output); |
| "engines": { "vscode": "^1.74.0" }, | ||
| "categories": ["Other"], | ||
| "main": "./out/extension.js", | ||
| "activationEvents": ["onStartupFinished"], |
There was a problem hiding this comment.
Unnecessary eager activation: onStartupFinished causes the extension to activate on every VS Code startup, even if the user never opens the sidebar or runs any OCR command. Since VS Code ≥1.74 automatically generates activation events from contributes declarations (views, commands, etc.), you can safely remove this entry entirely. The extension will then activate lazily when the user first interacts with the sidebar view or invokes a command, improving startup performance.
If there is a specific reason for early activation (e.g., background initialization), please add a comment explaining why.
Suggestion:
| "activationEvents": ["onStartupFinished"], | |
| "activationEvents": [], |
| "dependencies": { | ||
| "preact": "^10.19.0" | ||
| } |
There was a problem hiding this comment.
preact should be in devDependencies: Preact is bundled into out/webview.js by webpack at build time and is not required at runtime. Placing it in dependencies is semantically incorrect and may cause unnecessary installs in CI/production scenarios. Move it to devDependencies instead.
| async set(key: string, value: string): Promise<OcrConfig | null> { | ||
| await this.cli.runRaw(toConfigSetArgs(key, value), process.cwd(), () => {}); | ||
| return this.read(); | ||
| } |
There was a problem hiding this comment.
The set method lacks error handling for the this.cli.runRaw() call. If the CLI process fails to spawn (e.g., binary not found), runRaw will reject and the error will propagate uncaught to the caller (SidebarProvider.handle), resulting in an unhandled promise rejection. Consider wrapping the call in a try-catch and returning null (or re-throwing with a user-friendly message) so the caller can gracefully handle the failure.
Suggestion:
| async set(key: string, value: string): Promise<OcrConfig | null> { | |
| await this.cli.runRaw(toConfigSetArgs(key, value), process.cwd(), () => {}); | |
| return this.read(); | |
| } | |
| async set(key: string, value: string): Promise<OcrConfig | null> { | |
| try { | |
| await this.cli.runRaw(toConfigSetArgs(key, value), process.cwd(), () => {}); | |
| } catch { | |
| return null; | |
| } | |
| return this.read(); | |
| } |
| cancel(cb: Pick<SessionCallbacks, 'onState'>): void { | ||
| this.cancelled = true; | ||
| this.cli.cancel(); | ||
| cb.onState('cancelled'); | ||
| } |
There was a problem hiding this comment.
Potential duplicate state emission: When cancel() is called externally, it already emits cb.onState('cancelled'). However, after the CLI process is killed, the review() promise will either resolve or reject, and both paths check this.cancelled and emit cb.onState('cancelled') again. This causes the 'cancelled' state to be posted to the webview twice.
Consider returning early from cancel() without emitting state there, and let the run() method be the single place that emits the final state. Alternatively, guard against double emission with a flag like stateEmitted.
Co-developed-by: Aone Copilot <noreply@alibaba-inc.com>
Co-developed-by: Aone Copilot <noreply@alibaba-inc.com>
2 participants
Description
Add a VSCode extension under
extensions/vscode/that brings theopen-code-review(
ocr) CLI into the editor. The extension drives reviews from a sidebar,streams CLI logs live, and renders each finding as an in-editor
CommentThreadthat stays in sync with the sidebar (apply / ignore /mark-false-positive on either side updates the other).
Architecture
Adopts a Monolithic WebView + Thin Extension Host design:
src/extension/services/(CLI, Git, Config,providers/(Sidebar WebView, CommentThread),commands/src/webview/views/(Idle / Running / Done / Empty /components/,store,bridgesrc/shared/postMessageprotocol shared by both ends (novscodedependency)The two layers communicate via
postMessage, with shared types insrc/shared/guaranteeing type safety across the boundary.Features
(
--from/--to), single commit (--commit).--backgroundhint per review.CommentThreads; apply / ignore / false-positive stay in sync.via
ocr config set).from the status bar.
Type of Change
How Has This Been Tested?
yarn testpasses locally (Jest)Unit tests cover:
providers/__tests__/lineOffset.test.ts).services/__tests__/cliParse.test.ts,CliService.test.ts).services/__tests__/configParse.test.ts).services/__tests__/gitMap.test.ts).services/__tests__/shellEnv.test.ts).services/__tests__/ReviewSession.test.ts).webview/__tests__/store.test.ts).Manual smoke tests (F5 → Extension Development Host):
ocr config set.Checklist
extensions/vscode/README.md)