[AI Datamapper] Fix check error handling and output formatting #877
Conversation
WalkthroughPropagates a CopilotEventHandler through mapping generation APIs, makes Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DataMapping as dataMapping.ts
participant Utils as utils.ts
participant DataMapper as datamapper.ts
participant Handler as CopilotEventHandler
User->>DataMapping: generateMappings(metadataWithAttachments, context, eventHandler)
DataMapping->>Utils: generateMappingExpressionsFromModel(model, attachments[], eventHandler)
alt eventHandler provided
Utils->>Handler: emit(content_block: "Processing mapping hints from attachments...")
Utils->>Handler: emit(content_block: "Generating data mappings...")
end
Utils->>DataMapper: generateMappingCodeCore(mappingExpressions, eventHandler)
DataMapper->>DataMapper: repairAndCheckDiagnostics()
alt diagnostics contain BCE3032
DataMapper->>DataMapper: addCheckExpressionErrors(...)
DataMapper->>Handler: emit(content_block: "Repairing generated code...")
end
DataMapper-->>Utils: CodeRepairResult(finalContent, customFunctionsContent)
Utils-->>DataMapping: Mapping[] (finalContent applied, attachments)
DataMapping-->>User: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
VellummyilumVinoth
force-pushed
the
check-error
branch
from
5edca37 to
767254a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts(4 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts(12 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-10T15:05:11.309Z
Learnt from: madushajg
Repo: wso2/vscode-extensions PR: 868
File: workspaces/bi/bi-extension/src/utils.ts:224-242
Timestamp: 2025-11-10T15:05:11.309Z
Learning: The workspaces/bi/bi-extension and workspaces/ballerina/ballerina-extension are separate VS Code extensions that are packaged and distributed independently, so they cannot share code via imports and must maintain their own implementations of common utilities.
Applied to files:
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
🧬 Code graph analysis (1)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (5)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts (1)
CodeRepairResult(56-59)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (5)
getCustomFunctionsContent(1301-1308)repairAndCheckDiagnostics(1036-1056)getFunctionDefinitionFromSyntaxTree(150-173)generateInlineMappingsSource(1062-1124)buildMappingFileArray(1493-1514)workspaces/ballerina/ballerina-extension/src/utils/modification.ts (1)
writeBallerinaFileDidOpenTemp(69-92)workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
SyntaxTree(504-509)workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/utils.ts (1)
extractVariableDefinitionSource(301-328)
🔇 Additional comments (8)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts (1)
56-59: LGTM!The
CodeRepairResultinterface is well-structured and clearly separates the main file content from custom functions content, which aligns with the repair flow described in the AI summary.workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (7)
20-20: LGTM!The new imports support the BCE3032 error handling and code repair functionality. All imported types and functions are used appropriately in the implementation.
Also applies to: 28-28, 33-33, 42-44
263-264: LGTM!The progress messages align with the PR objectives and provide clear user feedback during the four documented phases: initial scanning, attachment processing (conditional), mapping generation, and code repair.
Also applies to: 370-370, 796-797, 840-840
401-415: BCE3032 handling logic is sound, but depends on fixing the helper function.The diagnostic checking and BCE3032 error detection logic is correct. However, the effectiveness depends on fixing the critical string replacement bug in
addErrorToDataMappingFunctionReturnType(see previous comment). Once that helper is corrected, this flow should work as intended.After applying the fix to
addErrorToDataMappingFunctionReturnType, verify that:
- BCE3032 errors are properly detected
- The return type is correctly modified with
|error- No unrelated code is corrupted
884-907: BCE3032 handling logic is appropriate for inline mappings.The inline mapping BCE3032 detection and handling mirrors the pattern used in the standard mapping flow. The logic correctly identifies the error and attempts to repair it. However, like the mapping flow, this depends on fixing the string replacement bug in
addErrorToInlineVariableContainingFunction.After applying the fix to
addErrorToInlineVariableContainingFunction, test inline mappings with check expressions to ensure:
- The containing function is correctly identified
- The return type is properly modified
- The variable definition extraction still works correctly
434-434: LGTM!The assistant response updates improve user experience by:
- Providing clear, structured output
- Displaying attachment information when present
- Warning users about overriding existing mappings
The messaging is consistent between standard and inline mapping flows.
Also applies to: 442-449, 918-925
347-347: LGTM!The event handler propagation enables the progress messages documented in the PR objectives. Based on the AI summary,
generateMappingsandgenerateInlineMappingsSourceaccept an optionaleventHandlerparameter, so these changes maintain backward compatibility while enabling enhanced progress tracking.Also applies to: 819-819
352-352: Use event-based synchronization instead of fixed delays.The code uses fixed 100ms delays after
writeBallerinaFileDidOpenTempandupdateSourceCodecalls, presumably waiting for the language server to process changes. However,writeBallerinaFileDidOpenTempis synchronous and returns immediately after sendingdidChange/didOpennotifications—it doesn't wait for the language server to parse the file. These arbitrary delays are fragile and can fail under load or on slower systems.A better pattern already exists in this codebase:
writeBallerinaFileDidOpen(lines 94–137 in modification.ts) subscribes toArtifactsUpdatednotifications with a 10-second timeout, ensuring the language server has actually processed the changes. Consider refactoringwriteBallerinaFileDidOpenTempto use the same notification-based approach, or use its async counterpart where applicable.Affected lines: 352, 422, 589–590, 685–686, 822
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
Outdated
Show resolved
Hide resolved
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
Outdated
Show resolved
Hide resolved
VellummyilumVinoth
force-pushed
the
check-error
branch
2 times, most recently
from
d03534e to
512ffdb
Compare
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
Outdated
Show resolved
Hide resolved
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
Outdated
Show resolved
Hide resolved
workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts
Outdated
Show resolved
Hide resolved
VellummyilumVinoth
force-pushed
the
check-error
branch
from
512ffdb to
a5ba21f
Compare
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
Outdated
Show resolved
Hide resolved
VellummyilumVinoth
force-pushed
the
check-error
branch
2 times, most recently
from
ea55e0b to
adc05a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (4)
744-753: String replacement corrupts unrelated code.This function uses naive
fileContent.replace(returnTypeSource, ...)which will replace every occurrence of the return type text in the entire file, not just the function's return type. For example, if the return type is"Person", this will corrupt all variable declarations, comments, and other references toPersonthroughout the file.Use position-based replacement instead:
-function addErrorToReturnType(fileContent: string, returnTypeSource: string): string { - if (returnTypeSource && !returnTypeSource.includes('|error')) { - return fileContent.replace( - returnTypeSource, - `${returnTypeSource.trim()}|error ` - ); - } - return fileContent; -} +// This function should not be used - use position-based replacement instead +// Pass startChar and endChar from the returnType node and splice the file contentBased on past review comments.
755-777: Critical: Use position-based replacement instead of string replacement.This function calls
addErrorToReturnTypewhich has the string replacement bug. Past review comments explicitly flagged this function (lines 567-595 in the old version). ThereturnTypeobject likely contains position information (startChar,endChar) that should be used for precise replacement.Apply position-based replacement:
if (funcDef?.functionSignature.returnTypeDesc) { const returnType = funcDef.functionSignature.returnTypeDesc.type; - fileContent = addErrorToReturnType(fileContent, returnType.source); + if (returnType.source && !returnType.source.includes('|error') && + returnType.startChar !== undefined && returnType.endChar !== undefined) { + const before = fileContent.substring(0, returnType.startChar); + const after = fileContent.substring(returnType.endChar); + fileContent = before + returnType.source.trim() + '|error' + after; + } writeBallerinaFileDidOpenTemp(tempFilePath, fileContent);Based on past review comments.
779-806: Critical: String replacement will corrupt the entire file.This function calls
addErrorToReturnTypein a loop for all functions, compounding the string replacement bug. Each call can corrupt unrelated code, and multiple calls will make the corruption worse.Use position-based replacement for each function:
for (const func of allFunctions) { if (func.functionSignature?.returnTypeDesc) { const returnType = func.functionSignature.returnTypeDesc.type; - customFileContent = addErrorToReturnType(customFileContent, returnType?.source); + if (returnType?.source && !returnType.source.includes('|error') && + returnType.startChar !== undefined && returnType.endChar !== undefined) { + const before = customFileContent.substring(0, returnType.startChar); + const after = customFileContent.substring(returnType.endChar); + customFileContent = before + returnType.source.trim() + '|error' + after; + } } }Based on past review comments.
808-849: Critical: String replacement corrupts code; use position-based replacement.This function also uses
addErrorToReturnTypewith the same string replacement bug, which will corrupt unrelated code in the file.Apply position-based replacement:
if (returnType?.source) { - fileContent = addErrorToReturnType(fileContent, returnType.source); + if (!returnType.source.includes('|error') && + returnType.startChar !== undefined && returnType.endChar !== undefined) { + const before = fileContent.substring(0, returnType.startChar); + const after = fileContent.substring(returnType.endChar); + fileContent = before + returnType.source.trim() + '|error' + after; + } writeBallerinaFileDidOpenTemp(tempFilePath, fileContent);Based on past review comments.
🧹 Nitpick comments (2)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (2)
660-685: Consider extracting syntax tree utilities to a separate module.The
findAllFunctionsInSyntaxTreefunction is correctly implemented but adds to the complexity of this already large file (1100+ lines). Consider extracting syntax tree traversal utilities into a dedicated module for better maintainability and testability.
687-742: High complexity - consider refactoring for maintainability.This function has deep nesting (4-5 levels) with a nested
containsVariablehelper. Past review comments flagged similar complexity issues. Consider extracting the helper function to a top-level function for better testability and readability.Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts(4 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts(16 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts
- workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-10T15:05:11.309Z
Learnt from: madushajg
Repo: wso2/vscode-extensions PR: 868
File: workspaces/bi/bi-extension/src/utils.ts:224-242
Timestamp: 2025-11-10T15:05:11.309Z
Learning: The workspaces/bi/bi-extension and workspaces/ballerina/ballerina-extension are separate VS Code extensions that are packaged and distributed independently, so they cannot share code via imports and must maintain their own implementations of common utilities.
Applied to files:
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.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/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
🧬 Code graph analysis (1)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (8)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts (1)
CodeRepairResult(56-59)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (6)
getCustomFunctionsContent(1359-1366)repairAndCheckDiagnostics(1036-1056)getFunctionDefinitionFromSyntaxTree(150-173)generateInlineMappingsSource(1062-1125)buildMappingFileArray(1551-1572)generateDataMapperModel(327-411)workspaces/ballerina/ballerina-extension/src/features/ai/activator.ts (1)
langClient(33-33)workspaces/ballerina/ballerina-extension/src/core/extended-language-client.ts (1)
ExtendedLangClient(503-1500)workspaces/ballerina/ballerina-extension/src/utils/modification.ts (1)
writeBallerinaFileDidOpenTemp(69-92)workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
SyntaxTree(504-509)workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/utils.ts (1)
extractVariableDefinitionSource(301-328)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts (1)
openChatWindowWithCommand(701-703)
🔇 Additional comments (8)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts (1)
56-59: LGTM!The
CodeRepairResultinterface is well-defined and provides a clear structure for returning repaired code content.workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (6)
260-261: LGTM!The progress messages improve user visibility into the mapping process, as stated in the PR objectives.
462-468: LGTM!The attachment listing provides clear visibility into which attachments were used for mapping generation.
867-868: LGTM!Progress messages are consistent with the regular mapping flow and improve user visibility.
1011-1017: LGTM!Attachment listing is consistent with the regular mapping flow.
1057-1080: LGTM!The updated messaging provides better context by listing the source attachment names and using proper singular/plural grammar.
1104-1124: LGTM!The
openChatWindowWithCommandfunction provides a clean programmatic interface for opening the AI chat window with generated data mapper context.workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts (1)
122-122: All callers have been updated to provide attachments array.Verification confirms the breaking change has been properly addressed. The two locations where
MetadataWithAttachmentsobjects are created both provide theattachmentsfield:
dataMapping.ts(line 1102): passes empty arraydatamapper.ts(line 339): passes attachments from requestNo incomplete instantiations found.
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
Show resolved
Hide resolved
VellummyilumVinoth
force-pushed
the
check-error
branch
from
adc05a1 to
9bdf7eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (3)
743-771: Critical: Multiple functions with identical return types will be corrupted.When
addErrorToAllFunctionReturnTypesprocesses multiple functions with the same return type, successivereplaceAll()calls (line 763) will corrupt code.Example scenario:
- Function 1 returns
string→ FirstreplaceAllchanges it tostring|error- Function 2 also returns
string→ SecondreplaceAllmatches the already-modified first function, corrupting it furtherThe fix requires passing position information to
addErrorToReturnType:async function addErrorToAllFunctionReturnTypes( langClient: ExtendedLangClient, filePath: string ): Promise<string> { let fileContent = fs.readFileSync(filePath, 'utf8'); const st = await langClient.getSyntaxTree({ documentIdentifier: { uri: Uri.file(filePath).toString(), }, }) as SyntaxTree; const modulePart = st.syntaxTree as ModulePart; const allFunctions = findAllFunctionsInSyntaxTree(modulePart); - // Add |error to each function's return type if not already present + // Process functions in reverse order to maintain offsets + const sortedFunctions = allFunctions.sort((a, b) => + (b.functionSignature?.returnTypeDesc?.type?.startChar || 0) - + (a.functionSignature?.returnTypeDesc?.type?.startChar || 0) + ); + - for (const func of allFunctions) { + for (const func of sortedFunctions) { if (func.functionSignature?.returnTypeDesc) { const returnType = func.functionSignature.returnTypeDesc.type; - fileContent = addErrorToReturnType(fileContent, returnType.source); + if (returnType.startChar !== undefined && returnType.endChar !== undefined) { + fileContent = addErrorToReturnType( + fileContent, + returnType.startChar, + returnType.endChar, + returnType.source + ); + } } } writeBallerinaFileDidOpenTemp(filePath, fileContent); await new Promise((resolve) => setTimeout(resolve, 100)); return fs.readFileSync(filePath, 'utf8'); }Note: Process functions in reverse order (end-to-beginning) to preserve character offsets as you modify the file.
732-741: Critical: Global string replacement will corrupt unrelated code.The
addErrorToReturnTypefunction at line 735 usesreplaceAll()which replaces every occurrence ofreturnTypeSourcein the entire file. This will corrupt:
- Variable declarations with the same type name
- Comments mentioning the type
- Other type references throughout the file
Example: If
returnTypeSourceis"string", this replaces ALL occurrences of"string"in the file with"string|error ", breaking every string variable, parameter, and field.Use position-based replacement via the syntax tree node's character offsets:
-function addErrorToReturnType(fileContent: string, returnTypeSource: string): string { +function addErrorToReturnType( + fileContent: string, + returnTypeStartChar: number, + returnTypeEndChar: number, + returnTypeSource: string +): string { if (returnTypeSource && !returnTypeSource.includes('|error')) { - return fileContent.replaceAll( - returnTypeSource, - `${returnTypeSource.trim()}|error ` - ); + const before = fileContent.substring(0, returnTypeStartChar); + const after = fileContent.substring(returnTypeEndChar); + return before + returnTypeSource.trim() + '|error ' + after; } return fileContent; }Then update callers to pass position information from the syntax tree node (available via
returnTypeDesc.type.startCharandreturnTypeDesc.type.endCharor similar position fields).
773-814: Critical: Verify position-based replacement is used.The
addErrorToInlineVariableContainingFunctionat line 797 also callsaddErrorToReturnTypewith only the source string, inheriting the same global replacement issue. Ensure the fix toaddErrorToReturnTypeis also applied here.After fixing
addErrorToReturnTypeto use position-based replacement:if (containingFunc?.functionSignature?.returnTypeDesc) { const returnType = containingFunc.functionSignature.returnTypeDesc.type; - if (returnType?.source) { - fileContent = addErrorToReturnType(fileContent, returnType.source); + if (returnType?.source && returnType.startChar !== undefined && returnType.endChar !== undefined) { + fileContent = addErrorToReturnType( + fileContent, + returnType.startChar, + returnType.endChar, + returnType.source + ); writeBallerinaFileDidOpenTemp(tempFilePath, fileContent);
🧹 Nitpick comments (2)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (2)
452-471: Consider simplifying attachment name extraction.The attachment name extraction at lines 462-466 can be simplified using
map():if (mappingRequest.attachments.length > 0) { - const attachmentNames = []; - for (const att of (mappingRequest.attachments)) { - attachmentNames.push(att.name); - } + const attachmentNames = mappingRequest.attachments.map(att => att.name); assistantResponse += `- **Attachments**: ${attachmentNames.join(", ")}\n`; }
975-981: Consider simplifying attachment name extraction.Same simplification opportunity for inline mapping attachments:
if (inlineMappingRequest.attachments.length > 0) { - const attachmentNames = []; - for (const att of (inlineMappingRequest.attachments)) { - attachmentNames.push(att.name); - } + const attachmentNames = inlineMappingRequest.attachments.map(att => att.name); assistantResponse += `- **Attachments**: ${attachmentNames.join(", ")}\n`; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts(4 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts(16 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts
- workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-10T15:05:11.309Z
Learnt from: madushajg
Repo: wso2/vscode-extensions PR: 868
File: workspaces/bi/bi-extension/src/utils.ts:224-242
Timestamp: 2025-11-10T15:05:11.309Z
Learning: The workspaces/bi/bi-extension and workspaces/ballerina/ballerina-extension are separate VS Code extensions that are packaged and distributed independently, so they cannot share code via imports and must maintain their own implementations of common utilities.
Applied to files:
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.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/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
🧬 Code graph analysis (2)
workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (3)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
AllDataMapperSourceRequest(316-318)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts (1)
generateMappingExpressionsFromModel(156-178)workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
DMModel(130-141)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (6)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts (1)
CodeRepairResult(56-59)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (5)
getCustomFunctionsContent(1359-1366)repairAndCheckDiagnostics(1036-1056)getFunctionDefinitionFromSyntaxTree(150-173)generateInlineMappingsSource(1062-1125)buildMappingFileArray(1551-1572)workspaces/ballerina/ballerina-extension/src/core/extended-language-client.ts (1)
ExtendedLangClient(503-1500)workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
SyntaxTree(504-509)workspaces/ballerina/ballerina-extension/src/utils/modification.ts (1)
writeBallerinaFileDidOpenTemp(69-92)workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/utils.ts (1)
extractVariableDefinitionSource(301-328)
🔇 Additional comments (7)
workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (3)
36-36: LGTM!The import of
CopilotEventHandleris necessary for the event-driven mapping generation flow and is used correctly in the updated function signatures.
475-486: LGTM!The signature update correctly propagates the
eventHandlerthrough the mapping generation pipeline, enabling event-driven progress reporting. The attachment handling is consistent with the updated type requirements.
1062-1125: LGTM!The signature update correctly integrates event handling, and the attachments initialization logic is safe and defensive. The empty array default ensures the payload is always well-formed.
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (4)
20-20: LGTM!The new imports support the BCE3032 error handling and syntax tree traversal features. All imported types and utilities are properly utilized in the implementation.
Also applies to: 28-28, 42-45
260-261: LGTM!The progress messages improve user experience by providing clear visibility into mapping stages, as stated in the PR objectives.
Also applies to: 364-364, 832-833, 874-874
916-964: Conditional on fixing the helper functions.The inline mapping BCE3032 error handling logic is structurally sound, but it depends on
addErrorToInlineVariableContainingFunctionandaddErrorToAllFunctionReturnTypes, which have critical string replacement issues. Once those functions are fixed to use position-based replacement, this code will be safe.
1068-1088: LGTM!The
openChatWindowWithCommandfunction correctly generates the data mapper model and opens the AI panel with appropriate parameters. The command execution sequence (close then open) ensures a clean state.
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
Show resolved
Hide resolved
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
Outdated
Show resolved
Hide resolved
VellummyilumVinoth
force-pushed
the
check-error
branch
from
9bdf7eb to
02711d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts (2)
206-207: Potential NPE: missing null checks before accessingdataMapperMetadata.The code directly accesses
datamapperMetadata.codeDataanddatamapperMetadata.namewithout verifying thatdatamapperMetadataexists or that these properties are defined, which can cause a runtime exception.Apply this diff to add defensive checks:
await writeBallerinaFileDidOpen(balFilePath, req.content); updateView(); const datamapperMetadata = StateMachine.context().dataMapperMetadata; + if (!datamapperMetadata || !datamapperMetadata.codeData || !datamapperMetadata.name) { + return true; + } await refreshDataMapper(balFilePath, datamapperMetadata.codeData, datamapperMetadata.name); return true;
683-686: Clarify error handling comment.The comment on line 685 says "silently fail for timeout issues," but the catch block handles all exceptions, not just timeouts. Consider updating the comment to reflect the actual behavior or add timeout-specific handling if that's the intent.
} catch (error) { console.error(">>> Failed to add files to the project", error); - return false; //silently fail for timeout issues. + return false; // Silently fail to avoid disrupting user flow }
🧹 Nitpick comments (2)
workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts (1)
374-378: Action title matching may be too broad.The filter
action.title.includes("return")at line 377 could match unintended code actions if the title contains "return" in a different context (e.g., "return statement", "early return pattern"). Consider using a more specific pattern match.Apply this diff for more precise matching:
- // Find the action that adds error to return type - // The language server typically provides actions like "Change return type to ..." - const action = codeActions.find( - action => action.title && action.title.includes("return") - ); + // Find the action that adds error to return type + // The language server typically provides actions like "Change return type to ..." + const action = codeActions.find( + action => action.title && + (action.title.toLowerCase().includes("change return type") || + action.title.toLowerCase().includes("return type to")) + );workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (1)
475-486: Remove redundant fallback for required field.At line 484, the
|| []fallback is unnecessary sinceattachmentsis now a required field inMetadataWithAttachments(interface.ts line 122). The defensive coding is fine but creates a minor inconsistency with the type definition.Apply this diff if you want strict consistency with the type:
const generatedMappings = await generateMappingExpressionsFromModel( metadataWithAttachments.metadata.mappingsModel as DMModel, - metadataWithAttachments.attachments || [], + metadataWithAttachments.attachments, eventHandler );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts(4 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts(16 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
🧬 Code graph analysis (4)
workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts (3)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (2)
Diagnostics(148-151)SyntaxTree(504-509)workspaces/ballerina/ballerina-extension/src/core/extended-language-client.ts (1)
ExtendedLangClient(503-1500)workspaces/ballerina/ballerina-extension/src/utils/modification.ts (1)
writeBallerinaFileDidOpenTemp(69-92)
workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (3)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
AllDataMapperSourceRequest(316-318)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts (1)
generateMappingExpressionsFromModel(156-178)workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
DMModel(130-141)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (6)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts (1)
CodeRepairResult(56-59)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (6)
getCustomFunctionsContent(1359-1366)repairAndCheckDiagnostics(1036-1056)getFunctionDefinitionFromSyntaxTree(150-173)generateInlineMappingsSource(1062-1125)buildMappingFileArray(1551-1572)generateDataMapperModel(327-411)workspaces/ballerina/ballerina-extension/src/features/ai/activator.ts (1)
langClient(33-33)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts (1)
addCheckExpressionErrors(334-423)workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/utils.ts (1)
extractVariableDefinitionSource(301-328)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts (1)
openChatWindowWithCommand(701-703)
workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts (2)
workspaces/ballerina/ballerina-extension/src/stateMachine.ts (2)
StateMachine(653-676)updateView(688-768)workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/utils.ts (1)
refreshDataMapper(407-419)
🔇 Additional comments (7)
workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts (1)
665-681: Good defensive checks for data mapper refresh, but apply similar pattern toaddToProject.The conditional refresh logic with fallback to
updateView()is well-structured and prevents NPEs. However, theaddToProjectmethod (lines 206-207) directly accessesdataMapperMetadata.codeDataanddataMapperMetadata.namewithout null checks, creating an inconsistency.Apply similar defensive checks to the
addToProjectmethod:await writeBallerinaFileDidOpen(balFilePath, req.content); updateView(); const datamapperMetadata = StateMachine.context().dataMapperMetadata; - await refreshDataMapper(balFilePath, datamapperMetadata.codeData, datamapperMetadata.name); + if (!datamapperMetadata || !datamapperMetadata.codeData || !datamapperMetadata.name) { + return true; + } + await refreshDataMapper(balFilePath, datamapperMetadata.codeData, datamapperMetadata.name); return true;workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (1)
1102-1115: LGTM: Proper attachments initialization and event handler forwarding.The explicit initialization of attachments as an empty array (line 1104) followed by conditional population (lines 1106-1108) correctly handles the now-required
attachmentsfield. Event handler forwarding is consistent with the updated call chain.workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (5)
258-259: LGTM: Progress messages improve user experience.The added progress messages provide visibility into mapping stages, aligning with the PR objectives to improve user feedback during operations.
391-421: LGTM: BCE3032 repair flow uses code actions approach.This implementation correctly addresses the critical string replacement issues flagged in past reviews. The repair flow:
- Detects BCE3032 diagnostics (check expression errors)
- Calls
addCheckExpressionErrorswhich uses the language server's code action API- Re-reads files after modifications to ensure content is up-to-date
The code action approach is safer and more reliable than string manipulation.
Based on learnings (past review comments requested code action approach).
449-459: Good UX: Conditional attachments display and override warning.The conditional display of attachments (lines 449-456) and the override warning (lines 457-459) improve user experience by:
- Only showing attachment information when relevant
- Warning users about potential data loss before they click "Add to Integration"
749-791: LGTM: Inline mapping BCE3032 repair mirrors main flow.The inline mapping repair flow correctly mirrors the main mapping approach and adds appropriate handling for variable extraction (lines 773-782). The re-extraction of variable definition after modifications ensures the displayed code reflects the repaired state.
896-915: LGTM: Programmatic AI chat integration for mapper.The
openChatWindowWithCommandfunction enables programmatic opening of the AI panel with a generated data mapper model, supporting the PR objective to refresh the visualizer correctly when "Add to Integration" is clicked. The close-then-open pattern ensures a clean state.
workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts
Show resolved
Hide resolved
VellummyilumVinoth
force-pushed
the
check-error
branch
from
02711d7 to
71a027d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (1)
1062-1115: LGTM! Event handler propagation for inline mappings.The updated
generateInlineMappingsSourcecorrectly threads theeventHandlerthrough togenerateMappings(lines 1111-1114), enabling progress messages for inline mapping generation.The attachments initialization at lines 1103-1108 is explicit and correct. If you prefer a more concise approach, consider:
- const mappingRequestPayload: MetadataWithAttachments = { - metadata: tempFileMetadata, - attachments: [] - }; - if (inlineMappingRequest.attachments.length > 0) { - mappingRequestPayload.attachments = inlineMappingRequest.attachments; - } + const mappingRequestPayload: MetadataWithAttachments = { + metadata: tempFileMetadata, + attachments: inlineMappingRequest.attachments || [] + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts(4 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts(16 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts
- workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
🧬 Code graph analysis (2)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (4)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts (1)
CodeRepairResult(56-59)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (6)
getCustomFunctionsContent(1359-1366)repairAndCheckDiagnostics(1036-1056)getFunctionDefinitionFromSyntaxTree(150-173)generateInlineMappingsSource(1062-1125)buildMappingFileArray(1551-1572)generateDataMapperModel(327-411)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts (1)
addCheckExpressionErrors(334-427)workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/utils.ts (1)
extractVariableDefinitionSource(301-328)
workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (3)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
AllDataMapperSourceRequest(316-318)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts (1)
generateMappingExpressionsFromModel(156-178)workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
DMModel(130-141)
🔇 Additional comments (9)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts (1)
56-59: LGTM! Clean interface for repair results.The
CodeRepairResultinterface clearly separates the main file content from custom functions content, supporting the BCE3032 repair flow introduced in this PR.workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (6)
20-43: LGTM! Import additions support new BCE3032 repair flow.The new imports (
CodeRepairResult,repairAndCheckDiagnostics,addCheckExpressionErrors, andfs) are all utilized in the BCE3032 error handling and file I/O operations introduced in this PR.
258-259: LGTM! Progress messages improve user experience.The progress messages align with the PR objectives and provide clear visibility into the mapping generation stages. Consistent implementation across both main and inline mapping flows (also at lines 362, 665-666, 707).
391-421: Excellent! BCE3032 handling uses LSP code actions instead of string manipulation.This implementation correctly addresses the check expression errors (BCE3032) by:
- Using
repairAndCheckDiagnosticsto detect BCE3032 diagnostics- Applying fixes via
addCheckExpressionErrors, which uses LSP code actions rather than naive string replacement- Re-reading files after modifications to capture the updated content
This approach avoids the string replacement pitfalls flagged in previous reviews and follows the recommended pattern of using the language server's code action API.
749-791: LGTM! Inline mapping BCE3032 handling mirrors main flow.The inline mapping flow correctly implements the same BCE3032 error handling pattern as the main mapping flow, with appropriate handling for variable-based inline mappings (lines 773-782). The code action-based approach ensures safe and accurate fixes.
449-459: LGTM! Enhanced assistant response with attachment details and override warnings.The updated assistant response now includes:
- Attachment names when provided (lines 449-455)
- Clear override warning when existing mappings will be replaced (lines 457-459)
This improves transparency and aligns with the PR objectives. Similar implementation for inline mappings at lines 802-812.
895-915: LGTM! Programmatic AI panel control for data mapper.The
openChatWindowWithCommandfunction cleanly generates the data mapper model and refreshes the AI chat interface by closing and reopening the panel with updated metadata. This supports the PR objective of refreshing the visualizer correctly when "Add to Integration" is clicked.workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (2)
36-36: LGTM! Import supports event handler propagation.The
CopilotEventHandlerimport is used to thread progress events through the mapping generation functions (lines 478, 1066).
475-486: LGTM! Event handler propagation enables progress messaging.The updated
generateMappingssignature threads theeventHandlerthrough togenerateMappingExpressionsFromModel, enabling progress messages during mapping generation. The|| []fallback on line 484 safely handles missing attachments.
VellummyilumVinoth
force-pushed
the
check-error
branch
2 times, most recently
from
73a5532 to
71a027d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (1)
770-775: Fix duplicate declaration and remove dead code.The variable
generatedSourceFilesis declared twice:
- First at line 770-775 using initial
codeRepairResult- Again at line 835-840 using potentially modified
codeRepairResultafter BCE3032 repairThe first declaration is dead code (never used before the second declaration), and the second creates a duplicate declaration error. Only the second declaration is used (at line 867).
Apply this fix:
- const generatedSourceFiles = buildMappingFileArray( - targetFilePath, - codeRepairResult.finalContent, - customFunctionsTargetPath, - codeRepairResult.customFunctionsContent, - ); - const variableName = inlineMappingRequest.metadata.name || inlineMappingsResult.tempFileMetadata.name; let codeToDisplay = codeRepairResult.finalContent; ... // BCE3032 repair logic (lines 791-833) ... - const generatedSourceFiles = buildMappingFileArray( + const generatedSourceFiles = buildMappingFileArray( context.documentUri, codeRepairResult.finalContent, customFunctionsTargetPath, codeRepairResult.customFunctionsContent, );Alternatively, declare once and reassign:
- const generatedSourceFiles = buildMappingFileArray( + let generatedSourceFiles = buildMappingFileArray( targetFilePath, codeRepairResult.finalContent, customFunctionsTargetPath, codeRepairResult.customFunctionsContent, ); ... // After BCE3032 repair completes - const generatedSourceFiles = buildMappingFileArray( + generatedSourceFiles = buildMappingFileArray( context.documentUri, codeRepairResult.finalContent, customFunctionsTargetPath, codeRepairResult.customFunctionsContent, );Also applies to: 835-840
🧹 Nitpick comments (1)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (1)
400-430: Approve BCE3032 repair logic; consider extracting duplicated code.The diagnostic repair flow correctly uses the code action approach via
addCheckExpressionErrorsto handle BCE3032 (check expression) errors, addressing past review concerns about unsafe string replacement. The logic properly:
- Collects file paths for both main and custom functions files
- Detects BCE3032 diagnostics
- Applies code actions to add
|errorto return types- Re-reads modified content
However, this logic is duplicated in the inline mapping flow (lines 791-833). Consider extracting this into a shared helper function to reduce code duplication and improve maintainability.
Based on past review comments.
Example refactor:
async function handleBCE3032Errors( langClient: ExtendedLangClient, projectRoot: string, tempDir: string, filePaths: string[], codeRepairResult: CodeRepairResult, isSameFile: boolean ): Promise<void> { const diags = await repairAndCheckDiagnostics(langClient, projectRoot, { tempDir, filePaths }); const hasCheckError = diags.diagnosticsList.some(diagEntry => diagEntry.diagnostics.some(d => d.code === "BCE3032") ); if (hasCheckError) { const isModified = await addCheckExpressionErrors(diags.diagnosticsList, langClient); if (isModified) { const mainFilePath = filePaths[0]; codeRepairResult.finalContent = fs.readFileSync(mainFilePath, 'utf8'); if (filePaths.length > 1 && !isSameFile) { codeRepairResult.customFunctionsContent = fs.readFileSync( filePaths[1], 'utf8' ); } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts(4 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts(16 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
🧬 Code graph analysis (2)
workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (3)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
AllDataMapperSourceRequest(316-318)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts (1)
generateMappingExpressionsFromModel(156-178)workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
DMModel(131-144)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (4)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts (1)
CodeRepairResult(56-59)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (5)
getCustomFunctionsContent(1359-1366)repairAndCheckDiagnostics(1036-1056)getFunctionDefinitionFromSyntaxTree(150-173)generateInlineMappingsSource(1062-1125)buildMappingFileArray(1551-1572)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts (1)
addCheckExpressionErrors(334-427)workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/utils.ts (1)
extractVariableDefinitionSource(301-328)
🪛 Biome (2.1.2)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
[error] 835-835: Shouldn't redeclare 'generatedSourceFiles'. Consider to delete it or rename it.
'generatedSourceFiles' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (8)
workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (4)
36-36: LGTM!The import of
CopilotEventHandleris correctly added to support event-driven progress reporting throughout the mapping workflow.
1111-1114: LGTM!The call to
generateMappingscorrectly passes all required parameters including the neweventHandler.
475-486: All callers properly provide the requiredeventHandlerparameter.Verification confirms that both call sites have been updated:
- Line 1111 in dataMapping.ts passes
eventHandlercorrectly- Line 337 in datamapper.ts passes
eventHandlercorrectlyNo migration issues detected.
1062-1066: All callers already provide theeventHandlerparameter.Verification confirms the single call site at
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts:704correctly passes all required parameters, includingeventHandler.workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (4)
20-20: LGTM! Import additions support diagnostic repair workflow.The new imports for
CodeRepairResult,addCheckExpressionErrors,repairAndCheckDiagnostics, and updatedfsimport properly support the BCE3032 diagnostic repair functionality introduced in this PR.Also applies to: 28-28, 33-34, 43-43
258-259: LGTM! Progress messages improve user visibility.The progress messages ("Reading project files...", "Repairing generated code...") and
eventHandlerpropagation togenerateMappingsandgenerateInlineMappingsSourcealign with the PR objectives to improve user visibility into mapping stages. Additional progress messages (e.g., "Processing mapping hints from attachments...", "Generating data mappings...") are likely emitted within the called functions.Also applies to: 340-340, 371-371, 682-683, 704-704, 733-733
466-476: LGTM! Attachments handling and override warning enhance UX.The assistant response correctly includes:
- Attachment names when present (conditional on length > 0)
- Override warning when existing mappings will be replaced
This provides clear feedback to users about what will happen when they click "Add to Integration".
937-957: LGTM! Visualizer refresh implementation is correct.The
openChatWindowWithCommandfunction properly implements the visualizer refresh requirement by:
- Generating an updated data mapper model
- Closing the AI panel to clear state
- Reopening with the refreshed model and appropriate template (function mappings vs. inline mappings)
This ensures the visualizer displays the latest state when "Add to Integration" is clicked.
VellummyilumVinoth
force-pushed
the
check-error
branch
from
315c894 to
476f79f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (1)
246-475: Guard against optional attachments when building mapping payload and responseIn
generateMappingCodeCorethe mapping request type still declaresattachments?: Attachment[], but the code assumes it’s always defined:
- Line 337:
attachments: mappingRequest.attachments- Line 449:
if (mappingRequest.attachments.length > 0) { ... }If
attachmentsis omitted, the.lengthaccess will throw, andattachments: undefinedviolates theMetadataWithAttachments.attachments: Attachment[]contract even thoughgenerateMappingsdefends withattachments || [].Consider normalizing to a non-undefined array once and reusing it:
- const allMappingsRequest = await generateMappings({ - metadata: tempFileMetadata, - attachments: mappingRequest.attachments - }, context, eventHandler); + const normalizedAttachments = mappingRequest.attachments ?? []; + + const allMappingsRequest = await generateMappings({ + metadata: tempFileMetadata, + attachments: normalizedAttachments + }, context, eventHandler); @@ - if (mappingRequest.attachments.length > 0) { - const attachmentNames = []; - for (const att of (mappingRequest.attachments)) { + if (normalizedAttachments.length > 0) { + const attachmentNames: string[] = []; + for (const att of normalizedAttachments) { attachmentNames.push(att.name); } assistantResponse += `- **Attachments**: ${attachmentNames.join(", ")}\n`; }This keeps runtime behavior safe while preserving the required-array semantics for
MetadataWithAttachments.
🧹 Nitpick comments (2)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (2)
653-828: Inline mapping flow looks good; consider passing imports to repair in the split-file caseThe inline mapping core mirrors the main mapping flow nicely: it adds progress messages, uses
generateInlineMappingsSourcewith the event handler, runsrepairCodeAndGetUpdatedContent, and then appliesrepairAndCheckDiagnostics+addCheckExpressionErrorsbefore building both the files array and the assistant response (including attachments and the override note).One small inconsistency: in the
isSameFilebranch you passimports: uniqueImportStatementsintorepairCodeAndGetUpdatedContent, but in theelsebranch you omitimports. For symmetry and to give the repair LLM the same context regardless of where custom functions are written, it would be cleaner to includeimportsin both branches:- codeRepairResult = await repairCodeAndGetUpdatedContent({ - tempFileMetadata: inlineMappingsResult.tempFileMetadata, - customFunctionsFilePath: inlineMappingsResult.allMappingsRequest.customFunctionsFilePath, - tempDir: inlineMappingsResult.tempDir - }, langClient, projectRoot); + codeRepairResult = await repairCodeAndGetUpdatedContent({ + tempFileMetadata: inlineMappingsResult.tempFileMetadata, + customFunctionsFilePath: inlineMappingsResult.allMappingsRequest.customFunctionsFilePath, + imports: uniqueImportStatements, + tempDir: inlineMappingsResult.tempDir + }, langClient, projectRoot);Behavior today is still correct; this would just make the repair hints more consistent.
895-915: openChatWindowWithCommand assumes data-mapper context; consider a defensive guard
openChatWindowWithCommandcalls:
generateDataMapperModel({}, langClient, context);, which immediately dereferencescontext.dataMapperMetadata.codeDatain the fallback branch, and- then uses
{ identifier, dataMapperMetadata } = contextto build the template payload.If this RPC is ever invoked when
context.dataMapperMetadata(orcontext.identifier) is unset—for example, from the AI panel when no data-mapper view is active—you’ll get a thrown error before any UI feedback.A light-weight guard would make this more robust:
export async function openChatWindowWithCommand(): Promise<void> { const langClient = StateMachine.langClient(); const context = StateMachine.context(); + if (!context.dataMapperMetadata) { + console.warn("openChatWindowWithCommand called without dataMapperMetadata in context"); + return; + } const model = await generateDataMapperModel({}, langClient, context); @@ - const { identifier, dataMapperMetadata } = context; + const { identifier, dataMapperMetadata } = context;This keeps the happy path unchanged while avoiding hard failures if the command is triggered in an unsupported state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts(4 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts(16 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
🧬 Code graph analysis (4)
workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts (2)
workspaces/ballerina/ballerina-extension/src/stateMachine.ts (2)
StateMachine(653-676)updateView(688-768)workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/utils.ts (1)
refreshDataMapper(407-419)
workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (3)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
AllDataMapperSourceRequest(316-318)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts (1)
generateMappingExpressionsFromModel(156-178)workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
DMModel(130-141)
workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts (2)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (2)
Diagnostics(148-151)SyntaxTree(504-509)workspaces/ballerina/ballerina-extension/src/utils/modification.ts (1)
writeBallerinaFileDidOpenTemp(69-92)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (4)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts (1)
CodeRepairResult(56-59)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (6)
getCustomFunctionsContent(1359-1366)repairAndCheckDiagnostics(1036-1056)getFunctionDefinitionFromSyntaxTree(150-173)generateInlineMappingsSource(1062-1125)buildMappingFileArray(1551-1572)generateDataMapperModel(327-411)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts (1)
addCheckExpressionErrors(334-427)workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/utils.ts (1)
extractVariableDefinitionSource(301-328)
🔇 Additional comments (6)
workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts (1)
119-123: MetadataWithAttachments now has a clear, non-optional attachments contractChanging
attachmentsto a requiredAttachment[]matches the way mapping payloads are now constructed (always initializing with[]and overriding when attachments exist). This simplifies downstream null‑checks and looks consistent with the new data-mapping flows.workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (1)
36-37: Event-handler wiring and attachments handling look consistentThreading
CopilotEventHandlerintogenerateMappings/generateInlineMappingsSourceand forwarding it togenerateMappingExpressionsFromModelis consistent with the utils signature, and initializingMetadataWithAttachmentswithattachments: []before conditionally overriding from the request ensures the now-requiredattachmentsfield is always defined. No further changes needed here.Also applies to: 475-487, 1062-1108
workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts (1)
656-687: Guarded data-mapper refresh after addFilesToProjectThe new logic that checks
dataMapperMetadataand itscodeData/fileName/namebefore callingrefreshDataMapper, and otherwise falls back toupdateView(), avoids null/undefined access while still ensuring the visualizer refreshes appropriately when data-mapper context is available. Returningfalseon errors in the catch block also gives callers a clearer signal.workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts (1)
334-427: BCE3032 repair via code actions is a solid replacement for string manipulationUsing
codeAction+stModifyto fix BCE3032 check-expression errors, and then persisting theSyntaxTree.sourceback to disk, aligns with the existing import/required-fields repair helpers and avoids the earlier unsafe string replacements. The only thing to watch is the heuristic on action titles ("change","return","error"); if the LS ever renames these quick-fixes, this helper will silently skip them, so it’s worth validating against the current Ballerina LS behavior.workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (2)
17-44: New imports are consistent with the extended mapping and repair flowBringing in
CodeRepairResult, additional core types,fs, and the helpers from../../dataMappingplusaddCheckExpressionErrorsmatches how this file now delegates repair and diagnostics; I don’t see any inconsistencies between what’s imported here and what’s used in the updated mapping/inline flows.
846-893: Context-type generation messaging and validation are reasonableValidating that
typeCreationRequest.attachments.length === 0and throwing early matches the requirement that at least one attachment be present for context-type creation, and the updated message (“Types generated from the … file(s) shown below”) correctly handles both singular and plural cases. The rest of the flow—delegating togenerateTypesFromContextand surfacing the result as atype_creatorcode block—looks coherent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts (1)
122-125: Breaking change: attachments is now required.Making
attachmentsrequired (removing?) is a breaking API change. Per previous review findings, webview components inballerina-visualizer/src/views/AIPanel/components/AIChat/index.tsx(around lines 1002-1021) pass optionalattachments?: Attachment[]and must be updated to satisfy this contract—either by providing a default empty array (attachments || []) or by updating function signatures to require attachments.workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (1)
1102-1108: Add null-safety check for attachments access.Line 1106 accesses
inlineMappingRequest.attachments.lengthwithout checking ifattachmentsis defined. Despiteattachmentsbeing required in theMetadataWithAttachmentsinterface, defensive coding is necessary to prevent runtime errors from improperly constructed objects.Apply the suggested fix from the previous review:
const mappingRequestPayload: MetadataWithAttachments = { metadata: tempFileMetadata, attachments: [] }; - if (inlineMappingRequest.attachments.length > 0) { + if (inlineMappingRequest.attachments?.length > 0) { mappingRequestPayload.attachments = inlineMappingRequest.attachments; }Alternatively, simplify to:
const mappingRequestPayload: MetadataWithAttachments = { metadata: tempFileMetadata, - attachments: [] - }; - if (inlineMappingRequest.attachments.length > 0) { - mappingRequestPayload.attachments = inlineMappingRequest.attachments; - } + attachments: inlineMappingRequest.attachments || [] + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts(4 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts(16 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
🧬 Code graph analysis (2)
workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (3)
workspaces/ballerina/ballerina-core/src/interfaces/extended-lang-client.ts (1)
AllDataMapperSourceRequest(316-318)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts (1)
generateMappingExpressionsFromModel(156-178)workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
DMModel(131-144)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (4)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts (1)
CodeRepairResult(56-59)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (6)
getCustomFunctionsContent(1359-1366)repairAndCheckDiagnostics(1036-1056)getFunctionDefinitionFromSyntaxTree(150-173)generateInlineMappingsSource(1062-1125)buildMappingFileArray(1551-1572)generateDataMapperModel(327-411)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts (1)
addCheckExpressionErrors(334-427)workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/utils.ts (1)
extractVariableDefinitionSource(301-328)
🔇 Additional comments (11)
workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (4)
36-36: LGTM: Event handler import added.The
CopilotEventHandlerimport supports the new event-driven progress updates throughout the mapping flow.
475-486: LGTM: Event handler properly threaded through mapping generation.The
eventHandlerparameter is correctly added to the function signature and propagated togenerateMappingExpressionsFromModel. The defensive fallback pattern on line 484 (metadataWithAttachments.attachments || []) is good practice for runtime safety.
1062-1066: LGTM: Event handler parameter added to inline mapping source generation.The
eventHandlerparameter addition aligns with the overall change to propagate progress events through the mapping pipeline.
1111-1114: LGTM: Event handler correctly propagated to mapping generation.The
eventHandleris properly passed togenerateMappings, completing the event propagation chain for progress updates during inline mapping generation.workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (7)
20-20: LGTM! Imports align with the new diagnostic repair flow.The new imports support the BCE3032 error handling via code actions (
addCheckExpressionErrors) and theCodeRepairResulttype for managing repaired content. This addresses the string manipulation concerns from previous reviews.Also applies to: 28-28, 33-34, 43-43
258-259: LGTM! Progress messages enhance user visibility.The progress indicators clearly communicate each stage of the mapping process, helping users identify where issues occur if mapping fails.
Also applies to: 371-371, 682-683, 733-733
400-430: LGTM! BCE3032 error handling uses the recommended code action approach.The diagnostic-driven repair flow correctly applies LSP code actions via
addCheckExpressionErrorsto handle check expression errors, avoiding the naive string replacement issues flagged in previous reviews. Files are properly re-read after modifications to capture the updated content.
784-826: LGTM! Inline mapping BCE3032 handling mirrors main flow with proper variable extraction.The error handling correctly re-extracts the variable definition after applying code actions, ensuring the displayed code shows the updated content with error type additions.
883-883: Same concern: verifyattachmentsis always defined.Line 883 accesses
attachments.lengthdirectly. Consider using optional chaining:!typeCreationRequest.attachments?.length.
449-454: LGTM! File array construction correctly uses repair results.The
buildMappingFileArraycalls properly thread through theCodeRepairResultcontent, including custom functions when present.Also applies to: 828-833
466-472: Verify thatattachmentsis always defined by the type contract.The code accesses
.attachments.lengthat lines 466, 837, and 883 without null/undefined checks. While evidence suggests theMetadataWithAttachmentstype from@wso2/ballerina-corerequiresattachmentsas a defined property (the construction pattern at dataMapping.ts:1102 always initializes it asattachments: []), the external type definition cannot be verified from this repository. If the type contract guaranteesattachmentsis always present, the current code is acceptable; otherwise, consider adding defensive checks likemappingRequest.attachments?.length > 0.Also applies to: lines 837–843, 883–905
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
Show resolved
Hide resolved
VellummyilumVinoth
force-pushed
the
check-error
branch
from
1f9a718 to
65511b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (1)
949-961: Addawaitto prevent race condition between close and open commands.Executing
CLOSE_AI_PANEL_COMMANDandOPEN_AI_PANEL_COMMANDwithout awaiting the close operation may cause a race condition where the panel tries to open before the close completes, leading to inconsistent state. This was flagged in a previous review.Apply this diff:
// Automatically close and open AI mapping chat window with the generated model - commands.executeCommand(CLOSE_AI_PANEL_COMMAND); - commands.executeCommand(OPEN_AI_PANEL_COMMAND, { + await commands.executeCommand(CLOSE_AI_PANEL_COMMAND); + await commands.executeCommand(OPEN_AI_PANEL_COMMAND, { type: 'command-template',workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (1)
1102-1108: Fix potential null/undefined reference error.Line 1106 accesses
inlineMappingRequest.attachments.lengthwithout checking ifattachmentsis defined. This was flagged in a previous review but remains unaddressed.Apply this diff to add a null-safety check:
const mappingRequestPayload: MetadataWithAttachments = { metadata: tempFileMetadata, attachments: [] }; - if (inlineMappingRequest.attachments.length > 0) { + if (inlineMappingRequest.attachments?.length > 0) { mappingRequestPayload.attachments = inlineMappingRequest.attachments; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts(4 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts(15 hunks)workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts(1 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts
- workspaces/ballerina/ballerina-core/src/rpc-types/ai-panel/interfaces.ts
- workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/rpc-manager.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-24T14:51:49.267Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 998
File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132
Timestamp: 2025-11-24T14:51:49.267Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.
Applied to files:
workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.tsworkspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
📚 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/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
📚 Learning: 2025-11-26T06:37:07.886Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/dm-data/array-inner/inline/map3.bal.txt:6-8
Timestamp: 2025-11-26T06:37:07.886Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/dm-data/ directories, BAL test data files (such as those in array-inner/inline/, array-root/inline/, basic/inline/, and their reusable counterparts) may intentionally contain type errors and other violations. These are comparison files used to test data mapper functionality and error handling, so such errors should not be flagged as issues.
Applied to files:
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.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/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.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/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.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/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
🧬 Code graph analysis (1)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (3)
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/types.ts (1)
CodeRepairResult(56-59)workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (3)
getCustomFunctionsContent(1359-1366)repairAndCheckDiagnostics(1036-1056)buildMappingFileArray(1551-1572)workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts (1)
addCheckExpressionErrors(334-427)
🔇 Additional comments (6)
workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/utils.ts (1)
156-178: LGTM! Event handler threading implemented correctly.The
eventHandleris now a required parameter, which aligns with the past review feedback that "event handler should always be there." Progress messages are emitted at appropriate stages:
- "Processing mapping hints from attachments..." (conditional)
- "Generating data mappings..."
workspaces/ballerina/ballerina-extension/src/features/ai/dataMapping.ts (1)
475-486: LGTM! Event handler threading implemented correctly.The
generateMappingsfunction properly accepts and forwards theeventHandlertogenerateMappingExpressionsFromModel, enabling progress reporting through the mapping generation pipeline.workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts (3)
400-430: BCE3032 repair logic is well-structured.The implementation correctly:
- Checks for BCE3032 diagnostics after initial repair
- Uses code actions via
addCheckExpressionErrorsinstead of string manipulation- Re-reads files after modifications to get updated content
This addresses the approach suggested in past reviews to use code actions instead of naive string replacement.
258-259: Progress messages improve user experience.The addition of progress messages ("Reading project files and collecting imports...", "Repairing generated code...") aligns with the PR objectives to improve user visibility into mapping stages.
474-477: Add null-safety check for mappingsModel.mappings.Accessing
tempFileMetadata.mappingsModel.mappings.lengthcould throw if the nested properties are undefined.Apply this diff:
- if (tempFileMetadata.mappingsModel.mappings.length > 0) { + if (tempFileMetadata.mappingsModel?.mappings?.length > 0) { assistantResponse += `\n**Note**: When you click **Add to Integration**, it will override your existing mappings.\n`; }⛔ Skipped due to learnings
Learnt from: KCSAbeywickrama Repo: wso2/vscode-extensions PR: 653 File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:128-134 Timestamp: 2025-11-26T06:34:09.752Z Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBreadcrumb() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.Learnt from: KCSAbeywickrama Repo: wso2/vscode-extensions PR: 998 File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132 Timestamp: 2025-11-24T14:51:49.267Z Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.workspaces/ballerina/ballerina-extension/src/rpc-managers/ai-panel/repair-utils.ts (1)
334-427: I'll verify the observation about the BCE3032 code action title matching pattern. Let me search the codebase and gather information about the expected code action titles.
<function_calls>
#!/bin/bashSearch for BCE3032 references and check expression related code
rg "BCE3032|check.*expression.*error|addCheckExpressionErrors" --type=ts --type=js -C 3 | head -150
</function_calls>Now let me search for test files that might show expected code action titles:
<function_calls>
#!/bin/bashFind test files related to repair utilities or code actions
fd "test|spec" --type=f -e ".ts" -e ".js" | grep -i "repair|code.*action" | head -20
</function_calls>Let me search more broadly for code action related patterns in the repository:
<function_calls>
#!/bin/bashSearch for code action implementation patterns
rg "codeAction|code.*action" --type=ts -C 2 | head -200
</function_calls>Let me also check the structure of the repository to better understand context:
<function_calls>
#!/bin/bashGet the file structure of the repair-utils directory
find . -name "repair-utils*" -o -name "repair" | grep -E ".(ts|js)$" | head -20
</function_calls>Let me check the actual file to see if there's any documentation or comments about the expected titles:
<function_calls>
#!/bin/bashRead the repair-utils.ts file around the function in question
find . -name "repair-utils.ts" -type f
</function_calls><function_calls>
#!/bin/bashSearch for the file and read around the mentioned function
rg -l "addCheckExpressionErrors" --type=ts
</function_calls>Let me search for test cases or examples of code action titles in the repository:
<function_calls>
#!/bin/bashSearch for test patterns showing code action titles or BCE error codes
rg "title.*Change.*return|quickfix|code.*action.*title" --type=ts --type=js -i -C 2 | head -150
</function_calls>Let me check if there's any Ballerina language server or LSP related code:
<function_calls>
#!/bin/bashSearch for language server or LSP related files
fd -e ".ts" -e ".js" | xargs grep -l "language.*server|langClient" | head -20
</function_calls><function_calls>
#!/bin/bashGet more context about the codebase structure
git ls-files | grep -E "repair|lang|server" | head -20
</function_calls><function_calls>
#!/bin/bashSearch for documentation files that might explain code actions
find . -name "README*" -o -name "*.md" | xargs grep -l "BCE3032|check.*expression" 2>/dev/null | head -10
</function_calls>Let me check if there are comments or similar functions that show patterns:
<function_calls>
#!/bin/bashSearch for similar code action patterns in the same file
rg ".title\s*.\s*toLowerCase.*includes|codeActions.find" --type=ts -B 5 -A 5 | head -200
</function_calls><function_calls>
#!/bin/bashLet's look for any test or fixture files that might document code action behavior
find . -type f ( -name ".test.ts" -o -name ".spec.ts" -o -name "fixture" ) | xargs grep -l "change.*return|BCE" 2>/dev/null | head -10
</function_calls><function_calls>
Ballerina language BCE3032 check expression error code action title
</function_calls>
| if (mappingRequest.attachments.length > 0) { | ||
| const attachmentNames = []; | ||
| for (const att of (mappingRequest.attachments)) { | ||
| attachmentNames.push(att.name); | ||
| } | ||
| assistantResponse += `- **Attachments**: ${attachmentNames.join(", ")}\n`; | ||
| } |
There was a problem hiding this comment.
Add null-safety check for attachments access.
Accessing mappingRequest.attachments.length without verifying attachments is defined could throw a runtime error if attachments is undefined or null.
Apply this diff:
- if (mappingRequest.attachments.length > 0) {
+ if (mappingRequest.attachments?.length > 0) {
const attachmentNames = [];
for (const att of (mappingRequest.attachments)) {
attachmentNames.push(att.name);
}
assistantResponse += `- **Attachments**: ${attachmentNames.join(", ")}\n`;
}📝 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.
| if (mappingRequest.attachments.length > 0) { | |
| const attachmentNames = []; | |
| for (const att of (mappingRequest.attachments)) { | |
| attachmentNames.push(att.name); | |
| } | |
| assistantResponse += `- **Attachments**: ${attachmentNames.join(", ")}\n`; | |
| } | |
| if (mappingRequest.attachments?.length > 0) { | |
| const attachmentNames = []; | |
| for (const att of (mappingRequest.attachments)) { | |
| attachmentNames.push(att.name); | |
| } | |
| assistantResponse += `- **Attachments**: ${attachmentNames.join(", ")}\n`; | |
| } |
🤖 Prompt for AI Agents
In
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
around lines 466 to 472, the code directly accesses
mappingRequest.attachments.length which can throw if attachments is
null/undefined; change the guard to check that attachments exists and is an
array before using length (e.g., use Array.isArray(mappingRequest.attachments)
&& mappingRequest.attachments.length > 0 or mappingRequest.attachments?.length >
0), then iterate over mappingRequest.attachments safely to build
attachmentNames.
| if (inlineMappingRequest.attachments.length > 0) { | ||
| const attachmentNames = []; | ||
| for (const att of (inlineMappingRequest.attachments)) { | ||
| attachmentNames.push(att.name); | ||
| } | ||
| assistantResponse += `- **Attachments**: ${attachmentNames.join(", ")}\n`; | ||
| } | ||
|
|
||
| if (inlineMappingRequest.metadata.mappingsModel.mappings.length > 0) { | ||
| assistantResponse += `\n**Note**: When you click **Add to Integration**, it will override your existing mappings.\n`; | ||
| } |
There was a problem hiding this comment.
Add null-safety checks for attachments access.
Similar to the main mapping flow, accessing inlineMappingRequest.attachments.length and inlineMappingRequest.metadata.mappingsModel.mappings.length without null checks could cause runtime errors.
Apply this diff:
- if (inlineMappingRequest.attachments.length > 0) {
+ if (inlineMappingRequest.attachments?.length > 0) {
const attachmentNames = [];
for (const att of (inlineMappingRequest.attachments)) {
attachmentNames.push(att.name);
}
assistantResponse += `- **Attachments**: ${attachmentNames.join(", ")}\n`;
}
- if (inlineMappingRequest.metadata.mappingsModel.mappings.length > 0) {
+ if (inlineMappingRequest.metadata?.mappingsModel?.mappings?.length > 0) {
assistantResponse += `\n**Note**: When you click **Add to Integration**, it will override your existing mappings.\n`;
}📝 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.
| if (inlineMappingRequest.attachments.length > 0) { | |
| const attachmentNames = []; | |
| for (const att of (inlineMappingRequest.attachments)) { | |
| attachmentNames.push(att.name); | |
| } | |
| assistantResponse += `- **Attachments**: ${attachmentNames.join(", ")}\n`; | |
| } | |
| if (inlineMappingRequest.metadata.mappingsModel.mappings.length > 0) { | |
| assistantResponse += `\n**Note**: When you click **Add to Integration**, it will override your existing mappings.\n`; | |
| } | |
| if (inlineMappingRequest.attachments?.length > 0) { | |
| const attachmentNames = []; | |
| for (const att of (inlineMappingRequest.attachments)) { | |
| attachmentNames.push(att.name); | |
| } | |
| assistantResponse += `- **Attachments**: ${attachmentNames.join(", ")}\n`; | |
| } | |
| if (inlineMappingRequest.metadata?.mappingsModel?.mappings?.length > 0) { | |
| assistantResponse += `\n**Note**: When you click **Add to Integration**, it will override your existing mappings.\n`; | |
| } |
workspaces/ballerina/ballerina-extension/src/features/ai/service/datamapper/datamapper.ts
Show resolved
Hide resolved
2 participants
Description
Fixes AI Datamapper issues identified in #1753
Changes
Add
|errorwhen check exists in codeProgress Messages Added:
Visualizer updated correctly
Impact
Add to Integrationis clickedResolves #1753
Goals
Approach
UI Component Development
npm run storybookfrom the root directory to view current components.Manage Icons
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.