Fix issues in the MI Extension#1044
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds OpenAPI ↔ Synapse API synchronization on Swagger file save, normalizes YAML extensions from Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Editor as VSCode Editor
participant SaveHook as Save Handler (activate.ts)
participant RpcMgr as MiDiagramRpcManager
participant LC as MILanguageClient
participant Swagger as Swagger File
participant Synapse as Synapse API File
User->>Editor: Save Swagger document
Editor->>SaveHook: onDidSaveTextDocument
SaveHook->>SaveHook: check path under resources/api-definitions
alt counterpart found
SaveHook->>RpcMgr: init & build apiPath/apiName
SaveHook->>LC: request syntax tree for API
SaveHook->>RpcMgr: compareSwaggerAndAPI
alt definitions differ
SaveHook->>User: prompt Update API / Update Swagger
alt Update Swagger
User->>SaveHook: choose Update Swagger
SaveHook->>RpcMgr: updateSwaggerFromAPI(...)
RpcMgr->>Swagger: applyEdit
Swagger->>Editor: open (if needed)
Editor->>Swagger: save file
else Update API
User->>SaveHook: choose Update API
SaveHook->>LC: getResources(syntaxTree)
SaveHook->>RpcMgr: updateAPIFromSwagger(... insertPosition ...)
RpcMgr->>Synapse: applyEdit
Synapse->>Editor: open (if needed)
Editor->>Synapse: save file
end
else definitions match
SaveHook->>SaveHook: no action
end
else no counterpart
SaveHook->>SaveHook: skip sync
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ChinthakaJ98
requested review from
Copilot
and removed request for
gigara and
hevayo
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
workspaces/mi/mi-extension/src/util/fileOperations.ts (1)
601-621: Use ofUri.filehere is correct and safer for FS pathsSwitching to
Uri.file(projectDirPath)makes the deletion more robust across platforms compared toUri.parse, and matches howprojectDirPathis constructed.If you ever need stricter guarantees that the folder is gone before returning, you could
await workspace.fs.delete(...)and handle failures, but that would be a future refinement rather than a blocker for this PR.workspaces/mi/mi-extension/src/util/workspace.ts (1)
26-72: Persisting edits viadocument.save()aligns with save‑driven workflowsEnsuring the edited document is opened (if needed) and explicitly saved after
workspace.applyEditlooks correct and matches the new “on save” workflows.Two minor points you might consider (non‑blocking):
const uri = documentUri;is redundant; you can usedocumentUridirectly.- This will now trigger any
onDidSaveTextDocumenthandlers and format‑on‑save for these edits; verify this is intended for all callers ofreplaceFullContentToFileto avoid accidental save loops or extra formatting.workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts (1)
2652-2657: Auto‑saving afterapplyEditis useful; verify multi‑document edit behaviorSaving the target document after
workspace.applyEdit(edit)is a good addition for keeping swagger/API files in sync with editor state. Two things to double‑check:
- For
ApplyEditsRequestwith multiple edits, you currently always save onlyparams.documentUri. If in future anyeditRequest.documentUrican differ fromparams.documentUri, those other documents would not be saved.- The code assumes
params.documentUriis always a filesystem path usable with bothUri.file(uri)anddoc.uri.fsPath === uri. Make sure all current callers pass a path (not afile://URI string) to avoid mismatches.If the contract guarantees a single target document per applyEdit, this is fine; otherwise consider collecting the distinct document URIs you touch in the
'edits' in paramsbranch and saving each once.workspaces/mi/mi-extension/src/visualizer/activate.ts (1)
475-488: MakegetResourcesmore defensive against unexpected syntax‑tree shapes
getResourcesassumesst.syntaxTree.api.resourceis always an array of nodes exposingmethods,uriTemplate/urlMapping, andrange.startTagRange/endTagRange. If the language server ever returns a single object forresource(or omitsapi/resourcefor some edge cases), this will throw.Consider:
- Normalizing to an array and guarding against missing nodes:
const getResources = (st: any): any[] => { const api = st?.syntaxTree?.api; if (!api || !api.resource) { return []; } const resources = Array.isArray(api.resource) ? api.resource : [api.resource]; return resources.map(resource => ({ methods: resource.methods, path: resource.uriTemplate || resource.urlMapping, position: { startLine: resource.range.startTagRange.start.line, startColumn: resource.range.startTagRange.start.character, endLine: resource.range.endTagRange.end.line, endColumn: resource.range.endTagRange.end.character, }, expandable: false, })); };
- Optionally tightening the type of
st/ returned items instead ofanyfor better editor support.This reduces the chance of runtime errors if the syntax‑tree schema evolves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts(3 hunks)workspaces/mi/mi-extension/src/util/fileOperations.ts(1 hunks)workspaces/mi/mi-extension/src/util/templates.ts(2 hunks)workspaces/mi/mi-extension/src/util/workspace.ts(1 hunks)workspaces/mi/mi-extension/src/visualizer/activate.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-26T06:35:19.217Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:173-178
Timestamp: 2025-11-26T06:35:19.217Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the commented-out debugging block in the verifyFileContent function (lines 172-177 containing console.log, page.pause, and updateDataFileSync) is intentionally kept as a developer utility for updating test data files when needed. It should not be removed.
Applied to files:
workspaces/mi/mi-extension/src/util/fileOperations.tsworkspaces/mi/mi-extension/src/util/workspace.ts
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/**/*.config.{js,ts} : Use minimatch-compatible glob patterns for file matching in build and test configuration files
Applied to files:
workspaces/mi/mi-extension/src/util/fileOperations.ts
📚 Learning: 2025-11-05T10:31:47.583Z
Learnt from: madushajg
Repo: wso2/vscode-extensions PR: 830
File: workspaces/ballerina/ballerina-extension/test/ai/post_proccess/post.test.ts:0-0
Timestamp: 2025-11-05T10:31:47.583Z
Learning: In the workspaces/ballerina/ballerina-extension project, the tsconfig.json uses "rootDirs": ["src", "test"], which allows test files to import from src using shorter relative paths. For example, from test/ai/post_proccess/, the import '../../stateMachine' correctly resolves to src/stateMachine.ts due to this configuration.
Applied to files:
workspaces/mi/mi-extension/src/util/fileOperations.ts
📚 Learning: 2025-11-26T07:49:56.428Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:136-141
Timestamp: 2025-11-26T07:49:56.428Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBackButton() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.
Applied to files:
workspaces/mi/mi-extension/src/util/fileOperations.ts
🪛 Biome (2.1.2)
workspaces/mi/mi-extension/src/visualizer/activate.ts
[error] 377-377: 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 (3)
workspaces/mi/mi-extension/src/util/templates.ts (2)
31-60: CAR plugin version constant bump is consistentUpdating
LATEST_CAR_PLUGIN_VERSIONto"5.4.13"keeps the generated root POM in sync with the intended plugin version and the property usage below; no issues seen here.
415-444: synapse-unit-test-maven-plugin version bump looks straightforwardChanging the
synapse-unit-test-maven-pluginversion to5.4.13in the generated POM is consistent with the rest of the version updates in this PR and does not alter the template structure.workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts (1)
633-636: YAML extension normalization for swagger artifacts looks consistentUsing
ext === ".yml" ? ".yaml" : extboth when computinggetSwaggerNameand when buildingswaggerRegPathkeeps API-XML and swagger filenames aligned and normalizes.ymlto.yaml. This should address mismatches between generated swagger filenames and the resources directory layout.Also applies to: 682-688
There was a problem hiding this comment.
Pull request overview
This PR addresses three issues in the MI VSCode extension by implementing automatic document saving and OpenAPI/Swagger synchronization features. The changes ensure file edits are persisted properly and add user prompts to resolve conflicts between Swagger definitions and Synapse API configurations.
Key changes:
- Added automatic document saving after workspace edits to ensure changes persist
- Implemented bidirectional synchronization between OpenAPI definitions and Synapse APIs with user confirmation dialogs
- Standardized Swagger file naming to use
.yamlextension instead of.yml - Updated plugin version to 5.4.13
- Fixed URI handling in file operations to use
Uri.file()instead ofUri.parse()
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
workspaces/mi/mi-extension/src/visualizer/activate.ts |
Adds Swagger/API synchronization logic with user prompts when definitions diverge, includes new helper function to extract API resources |
workspaces/mi/mi-extension/src/util/workspace.ts |
Adds explicit document save after applying workspace edits to persist changes |
workspaces/mi/mi-extension/src/util/templates.ts |
Updates CAR plugin and synapse-unit-test-maven-plugin versions to 5.4.13 |
workspaces/mi/mi-extension/src/util/fileOperations.ts |
Fixes directory deletion by using Uri.file() instead of Uri.parse() for proper file path handling |
workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts |
Normalizes Swagger file extensions from .yml to .yaml and adds document save after edits |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts
Outdated
Show resolved
Hide resolved
ChinthakaJ98
force-pushed
the
mi-fixes-12
branch
from
3761c2a to
323413b
Compare
ChinthakaJ98
force-pushed
the
mi-fixes-12
branch
from
323413b to
9e82d6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
workspaces/mi/mi-extension/src/visualizer/activate.ts (2)
352-399: Add error handling to the promise chain and wrapcase "Update API"in a block.The promise chain lacks error handling. If
getSyntaxTreeorcompareSwaggerAndAPIfails, it results in an unhandled promise rejection. Additionally, theconst resourcesdeclaration on line 377 requires block scope to satisfy thenoSwitchDeclarationslint rule.- langClient?.getSyntaxTree({ - documentIdentifier: { - uri: apiPath - } - }).then(st => { - rpcManager.compareSwaggerAndAPI({ - apiName: apiName, - apiPath: apiPath - }).then(async response => { + langClient?.getSyntaxTree({ + documentIdentifier: { + uri: apiPath + } + }).then(st => { + rpcManager.compareSwaggerAndAPI({ + apiName: apiName, + apiPath: apiPath + }).then(async response => { if (response.swaggerExists && !response.isEqual) { const confirmUpdate = await vscode.window.showInformationMessage( 'The OpenAPI definition is different from the Synapse API.', { modal: true }, "Update API", "Update Swagger" ); switch (confirmUpdate) { case "Update Swagger": rpcManager.updateSwaggerFromAPI({ apiName: apiName, apiPath: apiPath, existingSwagger: response.existingSwagger, generatedSwagger: response.generatedSwagger }); break; - case "Update API": - const resources = getResources(st); + case "Update API": { + const resources = getResources(st); rpcManager.updateAPIFromSwagger({ apiName: apiName, apiPath: apiPath, existingSwagger: response.existingSwagger, generatedSwagger: response.generatedSwagger, resources: resources.map(r => ({ path: r.path, methods: r.methods, position: r.position })), insertPosition: { line: st.syntaxTree.api.range.endTagRange.start.line, character: st.syntaxTree.api.range.endTagRange.start.character } }); break; + } default: break; } } - }) - }); + }).catch(error => { + console.error('Failed to compare Swagger and API:', error); + }); + }).catch(error => { + console.error('Failed to get syntax tree:', error); + });
475-488: Add defensive checks for resource range properties.If any resource in the syntax tree lacks the expected
range.startTagRangeorrange.endTagRangestructure, the code will throw a runtime error. Consider adding guards or fallback values.const getResources = (st: any): any[] => { - const resources: any[] = st?.syntaxTree?.api?.resource ?? []; + const resources: any[] = Array.isArray(st?.syntaxTree?.api?.resource) + ? st.syntaxTree.api.resource + : []; return resources.map((resource) => ({ methods: resource.methods, path: resource.uriTemplate || resource.urlMapping, position: { - startLine: resource.range.startTagRange.start.line, - startColumn: resource.range.startTagRange.start.character, - endLine: resource.range.endTagRange.end.line, - endColumn: resource.range.endTagRange.end.character, + startLine: resource.range?.startTagRange?.start?.line ?? 0, + startColumn: resource.range?.startTagRange?.start?.character ?? 0, + endLine: resource.range?.endTagRange?.end?.line ?? 0, + endColumn: resource.range?.endTagRange?.end?.character ?? 0, }, expandable: false - })); + })).filter(r => r.position.startLine > 0); };
🧹 Nitpick comments (1)
workspaces/mi/mi-extension/src/util/workspace.ts (1)
44-48: Consider checkingapplyEditresult before proceeding.
workspace.applyEdit(edit)returns aThenable<boolean>indicating success. If the edit fails, the code will still attempt to open and save the document, which could lead to unexpected behavior or saving stale content.- await workspace.applyEdit(edit); - const file = Uri.file(documentUri); - let document = workspace.textDocuments.find(doc => doc.uri.fsPath === documentUri) - || await workspace.openTextDocument(file); - await document.save(); + const editApplied = await workspace.applyEdit(edit); + if (!editApplied) { + console.error(`Failed to apply edit to ${documentUri}`); + return; + } + const file = Uri.file(documentUri); + let document = workspace.textDocuments.find(doc => doc.uri.fsPath === documentUri) + || await workspace.openTextDocument(file); + await document.save();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts(3 hunks)workspaces/mi/mi-extension/src/util/fileOperations.ts(1 hunks)workspaces/mi/mi-extension/src/util/templates.ts(2 hunks)workspaces/mi/mi-extension/src/util/workspace.ts(1 hunks)workspaces/mi/mi-extension/src/visualizer/activate.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts
- workspaces/mi/mi-extension/src/util/templates.ts
- workspaces/mi/mi-extension/src/util/fileOperations.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-26T06:35:19.217Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:173-178
Timestamp: 2025-11-26T06:35:19.217Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the commented-out debugging block in the verifyFileContent function (lines 172-177 containing console.log, page.pause, and updateDataFileSync) is intentionally kept as a developer utility for updating test data files when needed. It should not be removed.
Applied to files:
workspaces/mi/mi-extension/src/util/workspace.ts
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/**/*.{ts,tsx} : All async operations and promise handling in diagram utilities should use async/await syntax instead of .then() callbacks
Applied to files:
workspaces/mi/mi-extension/src/visualizer/activate.ts
📚 Learning: 2025-11-26T06:33:22.950Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:86-112
Timestamp: 2025-11-26T06:33:22.950Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, empty catch blocks around progress ring waitForSelector calls (state: 'attached' and 'detached') are intentional. The progress ring duration depends on machine performance and may appear very briefly, causing the wait to miss the event. The try-catch allows the test to proceed gracefully regardless of timing.
Applied to files:
workspaces/mi/mi-extension/src/visualizer/activate.ts
🧬 Code graph analysis (1)
workspaces/mi/mi-extension/src/visualizer/activate.ts (1)
workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts (1)
MiDiagramRpcManager(354-6128)
🪛 Biome (2.1.2)
workspaces/mi/mi-extension/src/visualizer/activate.ts
[error] 377-377: 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)
2 participants
This PR will fix the below mentioned issues.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.