Add support for Devant connections in BI#1459
Merged
kaje94 merged 128 commits intorelease/bi-1.8.xfrom Feb 19, 2026
Merged
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
workspaces/ballerina/ballerina-visualizer/src/components/TopNavigationBar/index.tsx (1)
121-121:⚠️ Potential issue | 🟡 MinorRemove debug
console.logbefore mergingThe
>>>prefix is a clear debug artifact and should not land in production.🐛 Proposed fix
- console.log(">>> history", history);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/components/TopNavigationBar/index.tsx` at line 121, Remove the debug console.log statement that prints the history (the line with console.log(">>> history", history)) from the TopNavigationBar component (index.tsx); locate the console.log inside the TopNavigationBar render or effect where the history variable is referenced and delete it so no debug output remains in production.workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx (2)
54-64:⚠️ Potential issue | 🟡 MinorDuplicate
flex: 1inStepContent— unconditional declaration on line 57 makes the conditional one on line 60 a no-op.Line 57 sets
flex: 1unconditionally. The conditional block (lines 59–63) also setsflex: 1whenfillHeightis true, which is redundant. Either the unconditionalflex: 1should be removed (if it should only apply whenfillHeightis true), or the duplicate inside the conditional should be removed.Proposed fix (assuming flex: 1 should only apply when fillHeight)
const StepContent = styled.div<{ fillHeight?: boolean }>` display: flex; flex-direction: column; - flex: 1; gap: 20px; ${(props: { fillHeight?: boolean }) => props.fillHeight && ` flex: 1; min-height: 0; height: 100%; `} `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx` around lines 54 - 64, The styled component StepContent currently declares flex: 1 unconditionally and again inside the fillHeight conditional, making the conditional redundant; update StepContent so flex: 1 is only applied when fillHeight is true by removing the unconditional "flex: 1" (leave the conditional block that sets flex: 1, min-height: 0, and height: 100% when props.fillHeight is true) so the layout only grows to fill space when fillHeight is provided.
449-480:⚠️ Potential issue | 🟡 MinorBrittle retry pattern using hardcoded
setTimeoutdelays.The flow sleeps 1 second (line 451), then searches, then sleeps another 2 seconds (line 478) if the connector isn't found yet. This is fragile — the connector may take longer on slow systems, or the delay wastes time when the connector is already available. Also, the comment on line 475 says "retry after 2 second" while line 477 says "retrying after 1 second" (actual delay is 2000ms).
Consider a polling loop with a bounded number of retries and exponential/linear backoff, or use an event-based mechanism if available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx` around lines 449 - 480, Replace the brittle fixed delays around the initial search and retry by implementing a bounded polling loop that calls the existing searchForConnector logic repeatedly with a configurable maxRetries and an exponential (or linear) backoff between attempts; use the same rpcClient.getBIDiagramRpcClient().search call inside searchForConnector, preserve defaultPosition/fileName/connectorName usage, update the console.warn to report the current attempt number and actual delay, and fail deterministically after maxRetries (returning null or throwing) instead of relying on fixed setTimeouts scattered around the code.workspaces/wso2-platform/wso2-platform-extension/src/git/util.ts (1)
599-621:⚠️ Potential issue | 🟠 MajorNon-trailing optional parameter with
directoryPath = ""causes CWD-dependent behaviour and usability trapTwo confirmed issues:
Non-trailing optional parameter:
directoryPathhas a default value but is not the last parameter. Callers cannot skip it; they must explicitly passundefinedto use the default (e.g.,hasDirtyRepo(undefined, context)). This defeats the intent to makedirectoryPathoptional.
path.relative("", ...)resolves to CWD: Node'spath.relative(from, to)resolves""viapath.resolve(""), which returnsprocess.cwd(). Thusrelative(repoRoot, "")produces a relative path from repo root to the process working directory (e.g.,"../../jailuser/git"), not an empty subdirectory filter. This CWD-dependent value is then passed togetStatus({ subDirectory: subPath })andgetUnPushedCommits(...), yielding incorrect results.All three existing call sites (PlatformExtensionApi.ts:37, commit-and-push-to-git-cmd.ts:87, WebviewRPC.ts:484) pass explicit non-empty values, so the default is not currently triggered. However, the parameter design is fundamentally flawed.
Recommended fix: Move
contextbeforedirectoryPathto make it a genuine trailing optional parameter, then update the three call sites to reorder arguments. Alternatively, add an explicit guard forundefinedand passdirectoryPathcorrectly:🔧 Parameter reordering (preferred)
-export const hasDirtyRepo = async (directoryPath: string = "", context: ExtensionContext, ignoredFileNames: string[] = []): Promise<boolean> => { +export const hasDirtyRepo = async (context: ExtensionContext, directoryPath: string = "", ignoredFileNames: string[] = []): Promise<boolean> => {Update call sites to pass context first:
hasDirtyRepo(fsPath, ext.context, ...)→hasDirtyRepo(ext.context, fsPath, ...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/wso2-platform/wso2-platform-extension/src/git/util.ts` around lines 599 - 621, The hasDirtyRepo function currently declares parameters as (directoryPath: string = "", context: ExtensionContext, ...) which makes directoryPath non-trailing-optional and causes path.relative(repoRoot, directoryPath) to treat "" as process.cwd(), producing incorrect subPath; fix by reordering parameters to (context: ExtensionContext, directoryPath?: string, ignoredFileNames: string[] = []) in the hasDirtyRepo declaration (and update internal uses like subPath = directoryPath ? relative(repoRoot, directoryPath) : ""), then update all call sites (PlatformExtensionApi.ts, commit-and-push-to-git-cmd.ts, WebviewRPC.ts) to pass context first and directoryPath second; alternatively, if you cannot change call sites now, add an explicit guard in hasDirtyRepo so that if directoryPath is "" or undefined you treat subPath as "" (not relative(...)) before calling git.getStatus and git.getUnPushedCommits.workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx (1)
773-784:⚠️ Potential issue | 🟠 Major
customValidatorerrors display but don't block form submission.
setDiagnosticsToFields(line 777) runs thecustomValidatorand setsseverity: "ERROR"diagnostics on fields, buthandleFormValidation's return value is gated solely onnodeWithDiagnostics?.diagnostics?.hasDiagnostics(LS diagnostics). A form with only custom-validator errors will returntrueand submit successfully.🐛 Proposed fix
const handleFormValidation = async (data: FormValues, dirtyFields?: any): Promise<boolean> => { if (node && targetLineRange && !skipFormValidation) { const updatedNode = mergeFormDataWithFlowNode(data, targetLineRange, dirtyFields); const nodeWithDiagnostics = await getFormWithDiagnostics(updatedNode); setDiagnosticsToFields(data, nodeWithDiagnostics!); if (nodeWithDiagnostics?.diagnostics?.hasDiagnostics) { return false; } + + if (customValidator) { + const hasCustomErrors = fields.some( + (field) => !!customValidator(field.key, data[field.key], data) + ); + if (hasCustomErrors) { + return false; + } + } } return true; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx` around lines 773 - 784, handleFormValidation currently only blocks on language-server diagnostics (nodeWithDiagnostics?.diagnostics?.hasDiagnostics) and ignores custom-validator errors set by setDiagnosticsToFields; change setDiagnosticsToFields to return a boolean (or a diagnostics summary) indicating if any field-level diagnostics with severity "ERROR" were applied, then in handleFormValidation call it like const hasFieldErrors = setDiagnosticsToFields(data, nodeWithDiagnostics!) and if hasFieldErrors return false; alternatively, if you prefer not to change the signature, after calling setDiagnosticsToFields inspect the form's field diagnostic state (e.g., a returned fieldDiagnostics map or the updated nodeWithDiagnostics.fieldDiagnostics) for any severity === "ERROR" and use that to block submission.workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/index.tsx (1)
103-106:⚠️ Potential issue | 🟠 MajorFix keyboard navigation indices after inserting Devant item.
menuItemRefsindices are now non‑sequential andcurrentMenuItemCountdoesn’t include the Devant entry, so Ctrl+Up/Down/Enter will skip items or stop early. Please align indices with the visual order and include the Devant entry in the count.🛠️ Suggested fix
- const currentMenuItemCount = types ? - (forcedValueTypeConstraint?.includes(AI_PROMPT_TYPE) ? 6 : 5) : - (forcedValueTypeConstraint?.includes(AI_PROMPT_TYPE) ? 5 : 4) + const baseMenuItemCount = types ? + (forcedValueTypeConstraint?.includes(AI_PROMPT_TYPE) ? 6 : 5) : + (forcedValueTypeConstraint?.includes(AI_PROMPT_TYPE) ? 5 : 4); + const menuIndexOffset = devantExpressionEditor ? 1 : 0; + const currentMenuItemCount = baseMenuItemCount + menuIndexOffset;- <SlidingPaneNavContainer - ref={el => menuItemRefs.current[3] = el} + <SlidingPaneNavContainer + ref={el => menuItemRefs.current[menuIndexOffset + 0] = el} to="INPUTS" > ... - <SlidingPaneNavContainer - ref={el => menuItemRefs.current[2] = el} + <SlidingPaneNavContainer + ref={el => menuItemRefs.current[menuIndexOffset + 1] = el} to="VARIABLES" > ... - <SlidingPaneNavContainer - ref={el => menuItemRefs.current[4] = el} + <SlidingPaneNavContainer + ref={el => menuItemRefs.current[menuIndexOffset + 2] = el} to="CONFIGURABLES" > ... - <SlidingPaneNavContainer - ref={el => menuItemRefs.current[5] = el} + <SlidingPaneNavContainer + ref={el => menuItemRefs.current[menuIndexOffset + 3] = el} to="FUNCTIONS" > ... - <SlidingPaneNavContainer - ref={el => menuItemRefs.current[6] = el} + <SlidingPaneNavContainer + ref={el => menuItemRefs.current[menuIndexOffset + 4] = el} to="DOCUMENTS" >Also applies to: 262-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/index.tsx` around lines 103 - 106, currentMenuItemCount and menuItemRefs are out of sync after inserting the "Devant" menu item causing keyboard navigation (Ctrl+Up/Down/Enter) to skip or stop early; update the logic that computes currentMenuItemCount to include the Devant entry and adjust the ordering/indexing of menuItemRefs to match the visual order. Specifically, modify the expression that defines currentMenuItemCount (and the equivalent logic in the other occurrence between lines 262-324) to account for the Devant item when forcedValueTypeConstraint includes AI_PROMPT_TYPE and when types is present or absent, and then renumber/shift the menuItemRefs population so indices are sequential and reflect the rendered item order used by the keyboard handlers (ensure functions that reference menuItemRefs use the new sequential indices).workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/Configurables.tsx (1)
93-113:⚠️ Potential issue | 🟠 Major
excludedConfigsarray reference in deps will cause excessive re-fetching.
excludedConfigsis an array prop. Since arrays are compared by reference in the dependency array, this effect will re-run on every parent render (unless the parent memoizes the array). Each run resetsconfigVariablesto[](line 94) and re-fetches, causing a flash of empty content.Consider stabilizing the dependency — e.g., by joining the array into a string key or using
JSON.stringify(excludedConfigs)as the dep:🐛 Proposed fix
- }, [excludedConfigs]) + }, [JSON.stringify(excludedConfigs)])Or better, memoize
excludedConfigsin the parent (DevantConfigurables).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/Configurables.tsx` around lines 93 - 113, The effect watching excludedConfigs (useEffect) currently uses the array reference directly which causes reruns on every render and resets configVariables via setConfigVariables([]); fix by stabilizing the dependency: replace excludedConfigs in the dependency array with a stable key such as JSON.stringify(excludedConfigs) or a memoized value from the parent, e.g., memoize excludedConfigs in DevantConfigurables and pass that; ensure getConfigVariables and getProjectInfo/toml fetch still run only when the stable key changes and remove unnecessary immediate resets of configVariables if you want to avoid the empty-flash behavior.workspaces/ballerina/ballerina-visualizer/src/views/BI/PackageOverview/index.tsx (2)
685-689:⚠️ Potential issue | 🔴 CriticalEvent subscription outside
useEffectcauses memory leaks.
onProjectContentUpdatedis called directly in the component body, re-registering a new listener on every render. This will accumulate duplicate subscriptions and leak memory.Move this into a
useEffectwith proper cleanup:🐛 Proposed fix
- rpcClient?.onProjectContentUpdated((state: boolean) => { - if (state) { - fetchContext(); - } - }); - - useEffect(() => { - fetchContext(); + useEffect(() => { + const dispose = rpcClient?.onProjectContentUpdated((state: boolean) => { + if (state) { + fetchContext(); + } + }); + fetchContext(); showLoginAlert().then((status) => { setShowAlert(status); }); + return () => { + dispose?.(); + }; }, [projectPath]);Note: If
onProjectContentUpdateddoes not return a disposable, you'll need another mechanism to prevent duplicate registrations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/PackageOverview/index.tsx` around lines 685 - 689, The RPC event subscription rpcClient?.onProjectContentUpdated((state: boolean) => { if (state) fetchContext(); }) is being registered on every render causing memory leaks; move this registration into a React useEffect that depends on rpcClient (and fetchContext if not stable) and ensure you unsubscribe/cleanup the listener when the component unmounts or rpcClient changes (use the disposable returned by onProjectContentUpdated if available, or call an unsubscribe method/flag to prevent duplicate registrations).
650-683:⚠️ Potential issue | 🟡 MinorRemove redundant README fetch —
getReadmeContentoverwriteshandleReadmeContent.Both
handleReadmeContent(line 665) andgetReadmeContent(line 679) fetch and setreadmeContentwith the same state variable. SincegetReadmeContentis asynchronous and completes after the synchronoushandleReadmeContentoperation, its result will overwrite the first fetch, making one call unnecessary. Remove the duplicate call or consolidate to use a single method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/PackageOverview/index.tsx` around lines 650 - 683, The fetchContext function is calling both rpcClient.getBIDiagramRpcClient().handleReadmeContent(...) and rpcClient.getBIDiagramRpcClient().getReadmeContent(...) which both set the same state via setReadmeContent and cause one to overwrite the other; remove the redundant call by keeping only the intended method (either handleReadmeContent or getReadmeContent) inside fetchContext, ensuring setReadmeContent is invoked once for readme updates, and update any related logic (e.g., parameters passed to handleReadmeContent/getReadmeContent) so the remaining call provides the required readme content.
workspaces/ballerina/ballerina-extension/src/rpc-managers/platform-ext/rpc-manager.ts
Show resolved
Hide resolved
workspaces/ballerina/ballerina-visualizer/src/providers/react-query-provider.tsx
Show resolved
Hide resolved
...allerina-visualizer/src/views/BI/Connection/AddConnectionPopup/AddConnectionPopupContent.tsx
Show resolved
Hide resolved
workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx
Outdated
Show resolved
Hide resolved
...ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantBIConnectorInitForm.tsx
Show resolved
Hide resolved
...aces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx
Show resolved
Hide resolved
Contributor
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/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js (1)
258-289:⚠️ Potential issue | 🟠 MajorBearer token is forwarded to redirect targets (potential token leak to S3/CDN domains)
makeRequestcalls itself recursively on redirects, passing the same headers — includingAuthorization: Bearer <token>— to whatever URLLocationpoints to. GitHub's/releases/assets/{id}endpoint redirects to AWS S3 presigned URLs (orobjects.githubusercontent.com). Sending the token there is unnecessary (S3 authenticates via query params) and leaks the credential to a third-party domain. A MITM or compromisedLocationheader could redirect to an arbitrary host.Fix: detect a cross-origin redirect and strip the
Authorizationheader before following it.🔒 Proposed fix — strip auth on cross-origin redirects
+ function isSameOrigin(urlA, urlB) { + try { + return new URL(urlA).origin === new URL(urlB).origin; + } catch { + return false; + } + } + const makeRequest = (requestUrl, redirectCount = 0) => { + const isInitial = redirectCount === 0; const req = https.request(requestUrl, { headers: { 'User-Agent': 'Choreo-CLI-Downloader', 'Accept': 'application/octet-stream', - ...getAuthHeaders() + ...(isInitial ? getAuthHeaders() : {}) } }, (res) => { if (isRedirect(res.statusCode) && res.headers.location) { if (redirectCount >= maxRedirects) { cleanupAndReject(new Error(`Too many redirects (${redirectCount})`)); return; } makeRequest(res.headers.location, redirectCount + 1);Alternatively, keep auth only when the redirect stays on the same origin:
- ...getAuthHeaders() + ...(isSameOrigin(url, requestUrl) ? getAuthHeaders() : {})where
urlis the outerdownloadFileparameter captured via closure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js` around lines 258 - 289, The makeRequest function currently reuses the same headers (including Authorization from getAuthHeaders) when following redirects, which leaks bearer tokens to third-party redirect targets; update makeRequest so that before calling itself on a redirect (where isRedirect(res.statusCode) && res.headers.location), it computes whether the redirect target is cross-origin relative to the original downloadFile URL (captured by downloadFile) and if so removes the Authorization header (or uses no auth headers) when calling makeRequest(res.headers.location, redirectCount + 1); keep existing redirect limit logic (maxRedirects) and preserve auth for same-origin redirects only, using the existing helper functions isRedirect and getAuthHeaders to construct headers appropriately.
🧹 Nitpick comments (6)
workspaces/wso2-platform/wso2-platform-extension/src/utils.ts (2)
529-542: File is read twice;originalContentsnapshot is taken after in-place mutation.Line 530 reads and parses the file into
componentYamlFileContent, which is then mutated at lines 531-538. Line 539 reads the file a second time intooriginalContent. This is both wasteful (two synchronous reads) and fragile — the "original" should be captured from the first parse, not from a re-read that bypasses any in-memory changes.♻️ Proposed fix: parse once, snapshot before mutation
if (existsSync(componentYamlPath)) { - const componentYamlFileContent: ComponentYamlContent = yaml.load(readFileSync(componentYamlPath, "utf8")) as any; + const fileText = readFileSync(componentYamlPath, "utf8"); + const originalContent: ComponentYamlContent = yaml.load(fileText) as any; + const componentYamlFileContent: ComponentYamlContent = yaml.load(fileText) as any; const schemaVersion = Number(componentYamlFileContent.schemaVersion); if (schemaVersion < 1.2) { componentYamlFileContent.schemaVersion = "1.2"; } componentYamlFileContent.dependencies = { ...componentYamlFileContent.dependencies, connectionReferences: [...(componentYamlFileContent.dependencies?.connectionReferences ?? []), { name: params?.name, resourceRef }], }; - const originalContent: ComponentYamlContent = yaml.load(readFileSync(componentYamlPath, "utf8")) as any; if (!deepEqual(originalContent, componentYamlFileContent)) { writeFileSync(componentYamlPath, yaml.dump(componentYamlFileContent)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/wso2-platform/wso2-platform-extension/src/utils.ts` around lines 529 - 542, The code reads componentYamlPath twice and takes originalContent after mutating componentYamlFileContent; to fix, read and parse the file once into componentYamlFileContent, create an immutable snapshot (e.g., clone or assign originalContent = deepClone(componentYamlFileContent)) before performing mutations to schemaVersion and dependencies, then perform the deepEqual(originalContent, componentYamlFileContent) comparison and call writeFileSync only if different; update references to existsSync, componentYamlPath, componentYamlFileContent, originalContent, deepEqual, and writeFileSync accordingly.
452-468: Unconditional write even when nothing is removed.
writeFileSyncat line 466 is called every time the file exists, even when neitherconnectionReferencesnorserviceReferencescontained an entry matchingparams.connectionName. This triggers an unnecessary filesystem write and an unintended git diff.createConnectionConfig(line 540) already guards writes withdeepEqual— apply the same pattern here.♻️ Proposed fix: guard the write
export const deleteLocalConnectionConfig = (params: DeleteLocalConnectionsConfigReq) => { const componentYamlPath = join(params.componentDir, ".choreo", "component.yaml"); if (existsSync(componentYamlPath)) { - const componentYamlFileContent: ComponentYamlContent = yaml.load(readFileSync(componentYamlPath, "utf8")) as any; + const fileText = readFileSync(componentYamlPath, "utf8"); + const originalContent: ComponentYamlContent = yaml.load(fileText) as any; + const componentYamlFileContent: ComponentYamlContent = yaml.load(fileText) as any; if (componentYamlFileContent.dependencies?.connectionReferences) { componentYamlFileContent.dependencies.connectionReferences = componentYamlFileContent.dependencies.connectionReferences.filter( (item) => item.name !== params.connectionName, ); } if (componentYamlFileContent.dependencies?.serviceReferences) { componentYamlFileContent.dependencies.serviceReferences = componentYamlFileContent.dependencies.serviceReferences.filter( (item) => item.name !== params.connectionName, ); } - writeFileSync(componentYamlPath, yaml.dump(componentYamlFileContent)); + if (!deepEqual(originalContent, componentYamlFileContent)) { + writeFileSync(componentYamlPath, yaml.dump(componentYamlFileContent)); + } } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/wso2-platform/wso2-platform-extension/src/utils.ts` around lines 452 - 468, deleteLocalConnectionConfig currently always calls writeFileSync when the YAML exists, causing pointless writes; change it to only write back when the content actually changed by comparing the original dependency arrays to the filtered ones (e.g., capture original componentYamlFileContent.dependencies.connectionReferences and .serviceReferences, produce filtered versions, then only call writeFileSync if at least one filtered array differs from its original using the same deepEqual helper/pattern used by createConnectionConfig). Ensure you still handle cases where dependencies or the specific arrays are undefined (treat as no-op) and only perform yaml.dump/write when a real removal occurred.workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantBIConnectorInitForm.tsx (4)
162-163: Hardcoded"PUBLIC"string literal is inconsistent with theServiceInfoVisibilityEnumused elsewhere.Lines 162 and 195 pass
"PUBLIC"as a raw string forvisibility, while every other call-site in this file and inDevantConnectorCreateForm.tsxusesServiceInfoVisibilityEnum.Public. If the parameter type is typed as the enum (or narrows to a union of enum members), this could silently drift.♻️ Proposed fix (applies to lines 162 and 195)
- visibility: "PUBLIC", + visibility: ServiceInfoVisibilityEnum.Public,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantBIConnectorInitForm.tsx` around lines 162 - 163, The visibility field in DevantBIConnectorInitForm is being set with the hardcoded string "PUBLIC" (at the object where componentDir: projectPath is set) which is inconsistent with the rest of the codebase; change that value to use the enum member ServiceInfoVisibilityEnum.Public so the type matches other call-sites (and repeat the same replacement at the other occurrence noted in this file), ensuring imports reference ServiceInfoVisibilityEnum if not already present and updating any object literals passed to functions like the Devant connector initializer to use the enum instead of the string.
279-293:overrideFlowNodemutates its argument in place.The callback directly writes to
node.properties.variable.valueandnode.properties.variable.editablebefore returning the same reference. IfConnectionConfigurationFormpasses the canonical node object (rather than a disposable clone), this mutation propagates upstream unexpectedly. Consider returning a cloned node instead.♻️ Proposed fix
overrideFlowNode={(node) => { if (node.properties.variable) { - node.properties.variable.value = importedConnection - ? initialNameCandidate - : generateInitialConnectionName( - biConnectionNames, - existingDevantConnNames, - initialNameCandidate, - ); - if (importedConnection) { - node.properties.variable.editable = false; - } + const variable = { + ...node.properties.variable, + value: importedConnection + ? initialNameCandidate + : generateInitialConnectionName( + biConnectionNames, + existingDevantConnNames, + initialNameCandidate, + ), + ...(importedConnection ? { editable: false } : {}), + }; + return { ...node, properties: { ...node.properties, variable } }; } return node; }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantBIConnectorInitForm.tsx` around lines 279 - 293, The overrideFlowNode callback currently mutates the passed-in node (writing to node.properties.variable.value and .editable) which can leak changes upstream; update overrideFlowNode to operate on and return a cloned node instead: create a new node object (shallow clone), clone its properties and the variable object (e.g., copy node, node.properties, node.properties.variable), then set the .value using importedConnection ? initialNameCandidate : generateInitialConnectionName(...) and set .editable when importedConnection is true, and finally return the cloned node; apply this change to the overrideFlowNode block that references generateInitialConnectionName, initialNameCandidate, importedConnection, and biConnectionNames/existingDevantConnNames.
79-95:useEffectis missing dependencies —onFlowChange,selectedMarketplaceItem, andimportedConnection.All three are used inside the effect body but absent from the dependency array. Particularly,
onFlowChangeis a callback prop whose reference can change between renders, meaning the effect could invoke a stale version of the function after a parent re-render. Even if the intent is to trigger only onselectedConnectorchanges, the ESLintreact-hooks/exhaustive-depsrule will flag this, and it is a genuine source of subtle bugs if props change concurrently.♻️ Proposed fix
-}, [props.selectedConnector]); +}, [props.selectedConnector, selectedMarketplaceItem, importedConnection, onFlowChange]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantBIConnectorInitForm.tsx` around lines 79 - 95, The useEffect that updates the flow is missing dependencies and should include onFlowChange, selectedMarketplaceItem, and importedConnection in its dependency array so it always uses current values; update the dependency array from [props.selectedConnector] to [props.selectedConnector, onFlowChange, selectedMarketplaceItem, importedConnection] (or ensure onFlowChange is stable by memoizing it with useCallback in the parent) and keep the existing DevantConnectionFlow logic intact to avoid stale callbacks or state.
138-138:!!!triple negation is redundant — use single!.
!!!xis always equivalent to!x. The double-negation idiom (!!) is used to coerce to boolean; adding a third!then negates it, exactly like a plain!. The same pattern repeats on lines 172 and 204.♻️ Proposed fix (applies to lines 138, 172, 204)
- const isProjectLevel = !!!platformExtState?.selectedComponent?.metadata?.id; + const isProjectLevel = !platformExtState?.selectedComponent?.metadata?.id;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantBIConnectorInitForm.tsx` at line 138, The triple negation used to compute isProjectLevel (const isProjectLevel = !!!platformExtState?.selectedComponent?.metadata?.id) is redundant and should be replaced with a single logical NOT to preserve intent and readability; update this occurrence and the same pattern used elsewhere (the other isProjectLevel assignments on lines with similar expressions) to use a single ! (or explicitly compare to undefined/null if you need boolean coercion) so the expression becomes !platformExtState?.selectedComponent?.metadata?.id while keeping the variable name isProjectLevel unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workspaces/wso2-platform/wso2-platform-extension/src/utils.ts`:
- Line 527: The constructed resourceRef may embed "undefined" because
component?.metadata?.handler and params?.marketplaceItem?.component?.endpointId
are not validated; update the logic that assigns resourceRef so it first checks
that component.metadata.handler and params.marketplaceItem.component.endpointId
are present (or explicitly assert and narrow their types) and if any are missing
return an empty string (or throw/report an error) instead of building the
template literal; specifically, validate those fields before using them in the
resourceRef assignment and only construct resourceRef when both are non-null,
referencing the resourceRef variable and the component.metadata.handler and
params.marketplaceItem.component.endpointId symbols in your changes.
---
Outside diff comments:
In
`@workspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js`:
- Around line 258-289: The makeRequest function currently reuses the same
headers (including Authorization from getAuthHeaders) when following redirects,
which leaks bearer tokens to third-party redirect targets; update makeRequest so
that before calling itself on a redirect (where isRedirect(res.statusCode) &&
res.headers.location), it computes whether the redirect target is cross-origin
relative to the original downloadFile URL (captured by downloadFile) and if so
removes the Authorization header (or uses no auth headers) when calling
makeRequest(res.headers.location, redirectCount + 1); keep existing redirect
limit logic (maxRedirects) and preserve auth for same-origin redirects only,
using the existing helper functions isRedirect and getAuthHeaders to construct
headers appropriately.
---
Duplicate comments:
In `@workspaces/wso2-platform/wso2-platform-extension/src/utils.ts`:
- Around line 476-481: The deletion of .choreo/endpoints.yaml and
.choreo/component-config.yaml happens too early and can cause data loss if
project or component lookups fail; update the logic in utils.ts so that the
rmSync calls that use join(params.componentDir, ".choreo", "endpoints.yaml") and
join(params.componentDir, ".choreo", "component-config.yaml") are deferred until
after the validation checks that return on !project and !component (i.e., move
or re-run the deletions only after the project and component existence guards
complete successfully), or add explicit checks that project and component are
truthy before calling rmSync to ensure deletions only occur when replacements
will be written.
---
Nitpick comments:
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantBIConnectorInitForm.tsx`:
- Around line 162-163: The visibility field in DevantBIConnectorInitForm is
being set with the hardcoded string "PUBLIC" (at the object where componentDir:
projectPath is set) which is inconsistent with the rest of the codebase; change
that value to use the enum member ServiceInfoVisibilityEnum.Public so the type
matches other call-sites (and repeat the same replacement at the other
occurrence noted in this file), ensuring imports reference
ServiceInfoVisibilityEnum if not already present and updating any object
literals passed to functions like the Devant connector initializer to use the
enum instead of the string.
- Around line 279-293: The overrideFlowNode callback currently mutates the
passed-in node (writing to node.properties.variable.value and .editable) which
can leak changes upstream; update overrideFlowNode to operate on and return a
cloned node instead: create a new node object (shallow clone), clone its
properties and the variable object (e.g., copy node, node.properties,
node.properties.variable), then set the .value using importedConnection ?
initialNameCandidate : generateInitialConnectionName(...) and set .editable when
importedConnection is true, and finally return the cloned node; apply this
change to the overrideFlowNode block that references
generateInitialConnectionName, initialNameCandidate, importedConnection, and
biConnectionNames/existingDevantConnNames.
- Around line 79-95: The useEffect that updates the flow is missing dependencies
and should include onFlowChange, selectedMarketplaceItem, and importedConnection
in its dependency array so it always uses current values; update the dependency
array from [props.selectedConnector] to [props.selectedConnector, onFlowChange,
selectedMarketplaceItem, importedConnection] (or ensure onFlowChange is stable
by memoizing it with useCallback in the parent) and keep the existing
DevantConnectionFlow logic intact to avoid stale callbacks or state.
- Line 138: The triple negation used to compute isProjectLevel (const
isProjectLevel = !!!platformExtState?.selectedComponent?.metadata?.id) is
redundant and should be replaced with a single logical NOT to preserve intent
and readability; update this occurrence and the same pattern used elsewhere (the
other isProjectLevel assignments on lines with similar expressions) to use a
single ! (or explicitly compare to undefined/null if you need boolean coercion)
so the expression becomes !platformExtState?.selectedComponent?.metadata?.id
while keeping the variable name isProjectLevel unchanged.
In `@workspaces/wso2-platform/wso2-platform-extension/src/utils.ts`:
- Around line 529-542: The code reads componentYamlPath twice and takes
originalContent after mutating componentYamlFileContent; to fix, read and parse
the file once into componentYamlFileContent, create an immutable snapshot (e.g.,
clone or assign originalContent = deepClone(componentYamlFileContent)) before
performing mutations to schemaVersion and dependencies, then perform the
deepEqual(originalContent, componentYamlFileContent) comparison and call
writeFileSync only if different; update references to existsSync,
componentYamlPath, componentYamlFileContent, originalContent, deepEqual, and
writeFileSync accordingly.
- Around line 452-468: deleteLocalConnectionConfig currently always calls
writeFileSync when the YAML exists, causing pointless writes; change it to only
write back when the content actually changed by comparing the original
dependency arrays to the filtered ones (e.g., capture original
componentYamlFileContent.dependencies.connectionReferences and
.serviceReferences, produce filtered versions, then only call writeFileSync if
at least one filtered array differs from its original using the same deepEqual
helper/pattern used by createConnectionConfig). Ensure you still handle cases
where dependencies or the specific arrays are undefined (treat as no-op) and
only perform yaml.dump/write when a real removal occurred.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.vscode/tasks.json (1)
102-142:⚠️ Potential issue | 🔴 CriticalMissing
"label"field will break thewatch-alldependency resolution.The new
watch-wso2-platformtask has no explicit"label"field. VS Code auto-generates the label for npm tasks (typically"npm: watch-wso2-platform"), so the"watch-wso2-platform"string inwatch-all'sdependsOn(Line 16) won't resolve to this task, causingwatch-allto error out or silently skip it.Compare with the
watch-ballerinaandwatch-bitasks (Lines 19 and 61), which both carry explicit"label"fields matching what's independsOn.🐛 Proposed fix — add the missing label
{ "type": "npm", + "label": "watch-wso2-platform", "script": "watch-wso2-platform", "path": "workspaces/wso2-platform/wso2-platform-extension",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vscode/tasks.json around lines 102 - 142, The new task "watch-wso2-platform" is missing an explicit "label", which prevents the "watch-all" task's dependsOn reference from resolving; add a "label": "watch-wso2-platform" property to that task (same place where "type", "script", and "path" are defined) so it matches the string used in the "watch-all" dependsOn, following the pattern used by "watch-ballerina" and "watch-bi".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.vscode/tasks.json:
- Around line 102-142: The new task "watch-wso2-platform" is missing an explicit
"label", which prevents the "watch-all" task's dependsOn reference from
resolving; add a "label": "watch-wso2-platform" property to that task (same
place where "type", "script", and "path" are defined) so it matches the string
used in the "watch-all" dependsOn, following the pattern used by
"watch-ballerina" and "watch-bi".
…ensions into devant-connections-v3
tharindulak
reviewed
Feb 18, 2026
workspaces/ballerina/ballerina-core/src/rpc-types/platform-ext/index.ts
Outdated
Show resolved
Hide resolved
workspaces/ballerina/ballerina-core/src/rpc-types/platform-ext/index.ts
Outdated
Show resolved
Hide resolved
workspaces/ballerina/ballerina-core/src/rpc-types/platform-ext/interfaces.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
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-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx (2)
476-480:⚠️ Potential issue | 🟡 MinorComment says "1 second" but the retry delay is 2000 ms; also a debug
console.warnartifact.The comment at line 477 is stale and the
console.warnshould be removed for production.🐛 Proposed fix
- console.warn(">>> Connector not found on first attempt, retrying after 1 second..."); await new Promise(resolve => setTimeout(resolve, 2000));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx` around lines 476 - 480, The inline log and stale comment are inconsistent: remove the debug console.warn call and make the retry delay comment match the actual timeout; in the block that checks createdConnector and calls await new Promise(resolve => setTimeout(resolve, 2000)) (and then calls searchForConnector()), delete the console.warn(">>> Connector not found on first attempt, retrying after 1 second...") and update any surrounding comment text to indicate a 2 second retry delay (or change the timeout to 1000 ms if you prefer a 1 second retry) so the message and setTimeout value in the createdConnector / searchForConnector retry logic are consistent.
484-489:⚠️ Potential issue | 🟡 MinorAdd null guard on
nodeTemplateResponse.flowNodebefore passing toonSave.The
getNodeTemplate()RPC method's catch block resolves toundefinedwhen an error occurs, but the return type declaresflowNodeas required. If the request fails, accessingnodeTemplateResponse.flowNodethrows a TypeError. Although the surrounding try-catch handles this, the user is left stuck in the "Loading connector configuration..." state with no error feedback.Suggested fix
const nodeTemplateResponse = await rpcClient.getBIDiagramRpcClient().getNodeTemplate({ position: target || null, filePath: fileName, id: createdConnector.codedata, }); - onSave(createdConnector, nodeTemplateResponse.flowNode, specType, connectorName, selectedFilePath); + if (nodeTemplateResponse?.flowNode) { + onSave(createdConnector, nodeTemplateResponse.flowNode, specType, connectorName, selectedFilePath); + } else { + console.error(">>> Failed to retrieve node template for connector"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx` around lines 484 - 489, The call to rpcClient.getBIDiagramRpcClient().getNodeTemplate can return undefined on error, so guard nodeTemplateResponse.flowNode before calling onSave: check that nodeTemplateResponse is defined and that nodeTemplateResponse.flowNode exists (or provide a sensible fallback) and only then call onSave(createdConnector, nodeTemplateResponse.flowNode, specType, connectorName, selectedFilePath); otherwise handle the error path (e.g., call onSave with undefined/empty config or surface an error/cleanup) so the UI doesn't stay stuck; update the code around getNodeTemplate and the onSave invocation to perform this null check.
🧹 Nitpick comments (12)
workspaces/ballerina/ballerina-visualizer/src/utils/bi.tsx (1)
159-171: Consider defining a typed extension for the Devant-enriched category items.The function attaches ad-hoc properties (
devant,unusedDevantConn,isLoading) toPanelCategoryobjects via object spread, bypassing the type system. Consumers must use type assertions oranyto access these fields, which is fragile and error-prone.A small interface extending
PanelCategory(e.g.,DevantEnrichedCategory) would make the contract explicit and let TypeScript catch misuse at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/utils/bi.tsx` around lines 159 - 171, Define a typed extension for the ad-hoc fields: add an interface like DevantEnrichedCategory extends PanelCategory { devant: DevantConnectionType; unusedDevantConn: boolean; isLoading?: boolean } (use the actual type for devant from your connections array). Replace the implicit any/spread usages where you create enriched category objects — e.g., the map that returns { ...categoryItem, devant: matchingDevantConn, unusedDevantConn: false } and the unusedCategoryItems const — with explicit annotations/returns of DevantEnrichedCategory[] and ensure the created objects conform to that interface; update any function signatures or local variables (such as the mapped result and unusedCategoryItems) to use DevantEnrichedCategory so consumers can access devant, unusedDevantConn, and isLoading without type assertions.workspaces/ballerina/ballerina-side-panel/src/components/NodeList/index.tsx (3)
681-687: Identicalhandlersmaps defined twice; extract once.The same five-entry map is built from scratch on every loop iteration in two separate places. With the new Devant entries it now appears identically at two locations. Hoist it once (e.g. at the top of
getCategoryContainer, or at component scope alongside the other handlers) to eliminate the duplication and avoid per-iteration allocations.♻️ Proposed refactor
const getCategoryContainer = (groups: Category[], isSubCategory = false, parentCategoryTitle?: string) => { + const handlers = { + onAddConnection: handleAddConnection, + onAddFunction: handleAddFunction, + onAdd: handleAdd, + onLinkDevantProject: handleOnLinkDevantProject, + onRefreshDevantConnections: handleOnRefreshDevantConnections, + }; + // ... rest of the function {categoryActions.map((action, actionIndex) => { - const handlers = { - onAddConnection: handleAddConnection, - onAddFunction: handleAddFunction, - onAdd: handleAdd, - onLinkDevantProject: handleOnLinkDevantProject, - onRefreshDevantConnections: handleOnRefreshDevantConnections - }; // ... })} // ... {categoryActions.map((action, actionIndex) => { - const handlers = { - onAddConnection: handleAddConnection, - onAddFunction: handleAddFunction, - onAdd: handleAdd, - onLinkDevantProject: handleOnLinkDevantProject, - onRefreshDevantConnections: handleOnRefreshDevantConnections - }; // ... })}Also applies to: 736-742
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-side-panel/src/components/NodeList/index.tsx` around lines 681 - 687, The duplicated creation of the identical handlers map (onAddConnection: handleAddConnection, onAddFunction: handleAddFunction, onAdd: handleAdd, onLinkDevantProject: handleOnLinkDevantProject, onRefreshDevantConnections: handleOnRefreshDevantConnections) should be hoisted out of the loop and defined once — move the handlers object to component scope or to the top of getCategoryContainer and reuse that single reference in both places (instead of rebuilding it at lines where handlers is currently created), ensuring any closures still have access to the same handler functions.
706-706: Unnecessary optional chaining onaction.
actionis the.map()callback parameter and can never beundefined. The access at line 689 (handlers[action.handlerKey]) already uses plain property access.action?.codeIconis inconsistent; preferaction.codeIcon.♻️ Proposed fix
- <Codicon name={action?.codeIcon || "add"} /> + <Codicon name={action.codeIcon || "add"} />- <Codicon name={action?.codeIcon || "add"} iconSx={{ fontSize: 12 }} /> + <Codicon name={action.codeIcon || "add"} iconSx={{ fontSize: 12 }} />Also applies to: 758-758
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-side-panel/src/components/NodeList/index.tsx` at line 706, Replace the unnecessary optional chaining for the map callback parameter by using the direct property access: in the map where you render <Codicon ... />, change the prop to use action.codeIcon || "add" (the callback parameter is never undefined; earlier access uses handlers[action.handlerKey] without ?), and make the same change for the other occurrence around the Codicon usage at the later line.
478-488: Wrapper handlers add a redundant null guard.Both
handleOnLinkDevantProjectandhandleOnRefreshDevantConnectionscheckif (onLinkDevantProject)/if (onRefreshDevantConnections)before calling, but the call site already bails out withif (!propsHandler || !handler) return null;(lines 693, 747–748), so the inner checks are never reached when the prop is absent. They can be simplified to direct calls:♻️ Proposed simplification
- const handleOnLinkDevantProject = () => { - if (onLinkDevantProject){ - onLinkDevantProject(); - } - } - - const handleOnRefreshDevantConnections = () => { - if (onRefreshDevantConnections){ - onRefreshDevantConnections(); - } - } + const handleOnLinkDevantProject = () => onLinkDevantProject?.(); + const handleOnRefreshDevantConnections = () => onRefreshDevantConnections?.();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-side-panel/src/components/NodeList/index.tsx` around lines 478 - 488, The wrapper handlers handleOnLinkDevantProject and handleOnRefreshDevantConnections contain redundant null guards before invoking their props; remove the inner conditional checks and call onLinkDevantProject() and onRefreshDevantConnections() directly inside those functions, since the component already returns early when handlers are missing (so the props are guaranteed present at call time). Ensure you only modify the bodies of handleOnLinkDevantProject and handleOnRefreshDevantConnections to eliminate the if-checks and directly invoke the prop functions.workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx (1)
40-58:queryKeylacks distinguishing parameters — risk of stale or shared cache.The query key
["config-variables"]does not include any project-specific identifier (e.g.,projectPath). If the visualizer location or project changes, react-query will return the cached result from the previous project instead of refetching. Include the project path (or another distinguishing value) in the key.♻️ Suggested fix
You could hoist the project path or use a compound key:
queryKey: ["config-variables"], + // Consider: queryKey: ["config-variables", projectPath],Alternatively, call
queryClient.invalidateQuerieson project changes, but a parameterized key is more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx` around lines 40 - 58, The queryKey for the useQuery call in DevantConfigurables.tsx is too generic and can return cached config names from another project; fetch the visualizer/project identifier first (via rpcClient.getVisualizerLocation() or the derived projectPath) and include that identifier in the queryKey (e.g., ["config-variables", projectPath]) so that useQuery (and functions like getConfigVariablesV2) are scoped per project and will refetch when the project/visualizer location changes; ensure you compute the projectPath before calling useQuery or derive it in a surrounding hook and pass it into useQuery's queryKey.workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx (2)
263-271: Unnecessary fragment around<StepperContainer>.
renderStepperwraps a single element in a fragment that serves no purpose.♻️ Proposed fix
const renderStepper = () => { return ( - <> - <StepperContainer> - <Stepper steps={steps} currentStep={currentStep} alignment="center" /> - </StepperContainer> - </> + <StepperContainer> + <Stepper steps={steps} currentStep={currentStep} alignment="center" /> + </StepperContainer> ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx` around lines 263 - 271, The renderStepper function returns a React fragment that wraps a single StepperContainer element; remove the unnecessary fragment so renderStepper directly returns <StepperContainer> (keep the existing Stepper component, props, and StepperContainer wrapper) to simplify the JSX and avoid redundant nodes in renderStepper.
54-64: Redundantflex: 1declaration inside thefillHeightblock.
flex: 1is already declared unconditionally at line 57, making the repetition inside thefillHeightconditional at line 60 a no-op.♻️ Proposed fix
const StepContent = styled.div<{ fillHeight?: boolean }>` display: flex; flex-direction: column; flex: 1; gap: 20px; ${(props: { fillHeight?: boolean }) => props.fillHeight && ` - flex: 1; min-height: 0; height: 100%; `} `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx` around lines 54 - 64, The styled component StepContent redundantly sets flex: 1 both unconditionally and again inside the fillHeight conditional; remove the duplicate flex: 1 from the conditional block in StepContent (the styled.div definition) and keep only the min-height: 0 and height: 100% lines when props.fillHeight is true so the fillHeight behavior is preserved without repeating flex: 1.workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/AddConnectionPopup/AddConnectionPopupContent.tsx (2)
85-85:filterparameter infetchConnectorsis dead code.The
filter?: booleanparameter (line 85) overrides thefilterByCurrentOrglogic at line 99, but it is never supplied at any call site — all callers invokefetchConnectors()with no arguments.Remove it or wire it up if it's still needed:
-const fetchConnectors = useCallback((filter?: boolean) => { +const fetchConnectors = useCallback(() => { ... - filterByCurrentOrg: filter ?? filterType === "Organization", + filterByCurrentOrg: filterType === "Organization",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/AddConnectionPopup/AddConnectionPopupContent.tsx` at line 85, The fetchConnectors function includes an unused parameter filter (signature: fetchConnectors(filter?: boolean)) which never gets passed by callers and overrides the intended filterByCurrentOrg behavior; remove the dead parameter from the function signature and any internal branching that reads it so the function consistently uses filterByCurrentOrg (or, if the original intent was to allow an override, update all call sites to pass the intended boolean). Specifically, update the fetchConnectors declaration to drop the filter param, remove references to that param inside fetchConnectors, and ensure the logic uses the existing filterByCurrentOrg flag to compute filters for connector retrieval.
223-252:filteredCategoriesderivation runs on every render — wrap inuseMemo.
cloneDeep(connectors)plus the category mapping/filtering pipeline executes on each render cycle, even whenconnectors,searchText, andfilterTypehaven't changed.♻️ Proposed fix
- const filteredCategories = cloneDeep(connectors).map((category) => { + const filteredCategories = useMemo(() => cloneDeep(connectors).map((category) => { ... - }).filter((category) => { + }).filter((category) => { ... - }); + }), [connectors, searchText, filterType]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/AddConnectionPopup/AddConnectionPopupContent.tsx` around lines 223 - 252, The filteredCategories computation (using cloneDeep(connectors).map(...).filter(...)) is expensive and currently runs every render; wrap it in React.useMemo to memoize results and only recompute when its inputs change—useMemo(() => { /* existing cloneDeep + map + filter pipeline using filterItems */ }, [connectors, searchText, filterType]); ensure you reference the same identifiers (filteredCategories, cloneDeep, filterItems, connectors, searchText, filterType) inside the memo callback so behavior is unchanged but recomputation only happens when those dependencies change.workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/AddConnectionPopup/styles.ts (3)
22-29:PopupContentandBackButtonduplicate exports from the parentConnection/styles.ts.Both components are byte-for-byte copies of exports already present in
workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/styles.ts(lines 83–90 and 53–56 respectively). Re-export or import from the parent module instead.♻️ Proposed fix
Remove both declarations from this file and re-export from the parent:
-export const PopupContent = styled.div` - flex: 1; - overflow-y: auto; - padding: 16px 20px; - display: flex; - flex-direction: column; - gap: 16px; -`;-export const BackButton = styled(Button)` - min-width: auto; - padding: 4px; -`;Add at the top of the imports:
+export { PopupContent, BackButton } from "../styles";Also applies to: 196-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/AddConnectionPopup/styles.ts` around lines 22 - 29, Remove the duplicate styled exports PopupContent and BackButton from this file and instead import and re-export them from the parent module where they are originally defined (the sibling Connection/styles.ts); update the top of this file to import { PopupContent, BackButton } from the parent module and then export them (or use direct re-exports) so the local file no longer contains duplicate declarations for PopupContent and BackButton.
167-187: Redundant inline type annotations inFilterButtoninterpolations.The generic
styled.button<{ active?: boolean }>already typesprops; the repeated(props: { active?: boolean })annotations in each interpolation are unnecessary noise.♻️ Proposed fix
export const FilterButton = styled.button<{ active?: boolean }>` font-size: 12px; padding: 6px 12px; height: 28px; border-radius: 4px; border: none; cursor: pointer; - font-weight: ${(props: { active?: boolean }) => (props.active ? 600 : 400)}; - background-color: ${(props: { active?: boolean }) => - props.active ? ThemeColors.PRIMARY : "transparent"}; - color: ${(props: { active?: boolean }) => - props.active ? ThemeColors.ON_PRIMARY : ThemeColors.ON_SURFACE_VARIANT}; + font-weight: ${(props) => (props.active ? 600 : 400)}; + background-color: ${(props) => props.active ? ThemeColors.PRIMARY : "transparent"}; + color: ${(props) => props.active ? ThemeColors.ON_PRIMARY : ThemeColors.ON_SURFACE_VARIANT}; transition: all 0.2s ease; &:hover { - background-color: ${(props: { active?: boolean }) => - props.active ? ThemeColors.PRIMARY : ThemeColors.SURFACE_CONTAINER}; - color: ${(props: { active?: boolean }) => - props.active ? ThemeColors.ON_PRIMARY : ThemeColors.ON_SURFACE}; + background-color: ${(props) => props.active ? ThemeColors.PRIMARY : ThemeColors.SURFACE_CONTAINER}; + color: ${(props) => props.active ? ThemeColors.ON_PRIMARY : ThemeColors.ON_SURFACE}; } `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/AddConnectionPopup/styles.ts` around lines 167 - 187, The interpolations inside the FilterButton styled component duplicate prop typings—remove the inline `(props: { active?: boolean })` annotations in all interpolations (font-weight, background-color, color, and &:hover) and use the already-declared generic props from `styled.button<{ active?: boolean }>` by referencing `props => props.active` (or simply `({ active }) => active`) so the component `FilterButton` relies on the single generic typing and avoids redundant annotations while keeping ThemeColors references intact.
206-217:ConnectorDetailCardshould declaredisabledvia the styled-component generic, not an inline cast.Without the generic type parameter, TypeScript doesn't validate
disabledas a prop, anddisabledmay also be forwarded as an unknown HTML attribute to the DOM element.♻️ Proposed fix
-export const ConnectorDetailCard = styled.div` +export const ConnectorDetailCard = styled.div<{ disabled?: boolean }>` position: relative; display: flex; align-items: center; gap: 12px; padding: 12px; border: 1px solid ${ThemeColors.OUTLINE_VARIANT}; border-radius: 8px; background-color: ${ThemeColors.SURFACE_DIM}; transition: all 0.2s ease; - opacity: ${(props: { disabled?: boolean }) => (props.disabled ? 0.5 : 1)}; + opacity: ${(props) => (props.disabled ? 0.5 : 1)}; `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/AddConnectionPopup/styles.ts` around lines 206 - 217, ConnectorDetailCard currently types its props via an inline cast inside the template which doesn't provide proper TS checking or prevent unknown HTML attributes from being forwarded; fix by declaring the prop type on the styled-component generic (e.g. change styled.div`...` to styled.div<{ disabled?: boolean }>`...` and update the opacity line to use props.disabled without a cast) or alternatively use a transient prop (e.g. $disabled) in the generic and template to ensure the boolean is typed and not forwarded to the DOM; update the ConnectorDetailCard declaration and remove the inline (props: { disabled?: boolean }) cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workspaces/ballerina/ballerina-visualizer/src/utils/bi.tsx`:
- Around line 152-176: The code can spread undefined because mappedCategoryItems
may be undefined when category.items is nullish; update the logic in the
panelCategories mapping so mappedCategoryItems is always an array (e.g., use
(category.items ?? []) before calling .map or default mappedCategoryItems to []
when category.items is falsy) so that the final return spreads arrays: items:
[...mappedCategoryItems, ...unusedCategoryItems]; adjust the creation of
mappedCategoryItems in the block that defines usedConnIds and maps connections
(references: panelCategories, category.items, mappedCategoryItems,
unusedCategoryItems, connections, usedConnIds).
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/AddConnectionPopup/AddConnectionPopupContent.tsx`:
- Around line 104-105: Remove the debug console.log traces in the
AddConnectionPopupContent React component: delete the console.log(">>> bi
connectors", model), console.log(">>> bi filtered connectors", model.categories)
and the other similar console.log(">>> ...") calls (the occurrences around lines
shown) inside AddConnectionPopupContent (or replace them with the project's
logger/debug utility if one exists). Ensure no stray console.log calls remain in
functions/methods within AddConnectionPopupContent.tsx before merging.
- Line 46: The effect that sets hasPersistConnection (useState
hasPersistConnection / setHasPersistConnection and the effect that calls
getModuleNodes) is computing whether a Persist database connection exists but
that state is never used; update the database card rendering logic (the JSX
gated by connectorOptions.showDatabase) to also hide or disable the database
option when hasPersistConnection is true (e.g., skip rendering or render
disabled UI), and wire the imported Tooltip and ExperimentalBadge into that UI
to explain why the database option is disabled; alternatively, if you prefer to
postpone the feature, remove the hasPersistConnection state, its effect, and the
unused Tooltip/ExperimentalBadge imports to avoid dead code.
- Around line 85-111: fetchConnectors and handleSearch call
rpcClient.getBIDiagramRpcClient() without guarding for a null/undefined
rpcClient and they also lack .catch() handlers, causing throws and swallowed
errors; update both functions (fetchConnectors and handleSearch) to use optional
chaining when accessing rpcClient (e.g., rpcClient?.getBIDiagramRpcClient()) or
early-return if rpcClient is falsy, and attach a .catch(err => { /* log error,
set user-facing error state if present */ setIsSearching(false);
setFetchingInfo(false); }) to the RPC promise chain so errors are logged/handled
and loading flags are cleared reliably.
- Around line 114-117: Remove the redundant mount-only effect that calls
setIsSearching(true) and fetchConnectors(); the existing effect dependent on
[filterType, fetchConnectors] already runs on initial render. Delete the
useEffect block that invokes setIsSearching and fetchConnectors with an empty
dependency array so only the effect referencing filterType and fetchConnectors
triggers the initial fetch.
- Around line 187-193: The listener registration accumulates because the effect
depends on fetchConnectors; instead, store the latest fetchConnectors in a ref
and register the rpcClient.onProjectContentUpdated listener only once so it
calls fetchConnectorsRef.current(), avoiding re-registration on
fetchConnectors/filterType changes: add a ref like fetchConnectorsRef =
useRef(fetchConnectors), update fetchConnectorsRef.current whenever
fetchConnectors changes (useEffect with [fetchConnectors]), and change the
existing effect to depend only on [rpcClient] and call
rpcClient.onProjectContentUpdated((state) => { if (state)
fetchConnectorsRef.current?.(); }); ensure you reference the existing symbols
fetchConnectors, rpcClient, and onProjectContentUpdated when making these
changes.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx`:
- Around line 367-370: The specType state is hardcoded to "OpenAPI" which
ignores the apiSpecOptions prop and causes the Dropdown and downstream logic
(handleOnGenerateSubmit, supportedFileFormats) to be out of sync; change the
initialization to derive from the prop (e.g., use apiSpecOptions?.[0]?.value or
fallback to defaultOptions[0].value/"OpenAPI") and add a useEffect that updates
setSpecType whenever apiSpecOptions changes so the Dropdown selection and
subsequent computations always reflect the provided apiSpecOptions.
- Around line 208-261: Remove the debug console.log calls in handleOnFormSubmit
(the initial ">>> on form submit" and ">>> Updated source code" logs), and
change the getSourceCode call (rpcClient.getBIDiagramRpcClient().getSourceCode)
to be awaited so handleOnFormSubmit only resolves after save completes; wrap
that await in a try/catch/finally block that sets setIsSavingConnection(true)
before the call, sets setIsSavingConnection(false) in finally, on success finds
the new artifact and calls onClose with recentIdentifier and
DIRECTORY_MAP.CONNECTION, and on error logs via console.error (or rethrows) so
callers can await and react to failures. Ensure you still handle the local scope
lineRange and isConnector determination before the awaited call.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx`:
- Around line 498-506: setDiagnosticsToFields currently appends customValidator
errors to updatedField.diagnostics (via setBaseFields) but handleFormValidation
only checks nodeWithDiagnostics?.diagnostics?.hasDiagnostics (LS-driven flag),
so custom errors don't block submit; update handleFormValidation to also scan
the current baseFields (or the updatedField.diagnostics you create) for any
diagnostic with severity "ERROR" (or for any non-empty customValidationMessage)
and return false if any exist, or alternatively set
nodeWithDiagnostics.diagnostics.hasDiagnostics when adding custom validator
errors in setDiagnosticsToFields; reference setDiagnosticsToFields,
handleFormValidation, customValidator, updatedField.diagnostics,
nodeWithDiagnostics?.diagnostics?.hasDiagnostics, and setBaseFields when making
the change.
- Around line 501-504: The synthetic diagnostic currently sets severity as the
string "ERROR" which doesn't match the Ballerina LSP Diagnostic type; update the
construction that appends to updatedField.diagnostics (the object created where
message: customValidationMessage is set) to use the numeric LSP severity enum
for Error (use 1) or import/resolve the DiagnosticSeverity enum from the
ballerina LSP types and use that constant instead so the severity value matches
the expected numeric format.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx`:
- Around line 60-76: The mutationFn passed to useMutation can return undefined
when devantExpressionEditor?.onAddDevantConfig is falsy; update mutationFn (in
the useMutation call) to always return a Promise by either returning
Promise.reject(new Error("onAddDevantConfig not available")) or throwing an
error when devantExpressionEditor?.onAddDevantConfig is absent, so callers of
mutate and react-query internals never receive undefined; reference the
mutationFn, useMutation, and devantExpressionEditor?.onAddDevantConfig symbols
when making the change.
- Around line 78-93: The modal is capturing a JSX snapshot so
isSaving={isPending} won’t update; change the pattern by moving the mutation and
saving state into DevantNewConfigurableForm: remove passing isSaving and
refreshConfigVariables into the JSX snapshot, instead pass a simple onSubmit
callback (e.g., onSubmit={(data) => { /* call mutate inside form */ }}), then
inside DevantNewConfigurableForm perform the mutate call and manage local saving
state (useTransition or useState) to set the Save button disabled/label while
the mutation runs; update references to mutate, onAddNewConfigurable, addModal
and POPUP_IDS.CONFIGURABLES accordingly so the modal only mounts the form
component and the form owns isSaving.
---
Outside diff comments:
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx`:
- Around line 476-480: The inline log and stale comment are inconsistent: remove
the debug console.warn call and make the retry delay comment match the actual
timeout; in the block that checks createdConnector and calls await new
Promise(resolve => setTimeout(resolve, 2000)) (and then calls
searchForConnector()), delete the console.warn(">>> Connector not found on first
attempt, retrying after 1 second...") and update any surrounding comment text to
indicate a 2 second retry delay (or change the timeout to 1000 ms if you prefer
a 1 second retry) so the message and setTimeout value in the createdConnector /
searchForConnector retry logic are consistent.
- Around line 484-489: The call to
rpcClient.getBIDiagramRpcClient().getNodeTemplate can return undefined on error,
so guard nodeTemplateResponse.flowNode before calling onSave: check that
nodeTemplateResponse is defined and that nodeTemplateResponse.flowNode exists
(or provide a sensible fallback) and only then call onSave(createdConnector,
nodeTemplateResponse.flowNode, specType, connectorName, selectedFilePath);
otherwise handle the error path (e.g., call onSave with undefined/empty config
or surface an error/cleanup) so the UI doesn't stay stuck; update the code
around getNodeTemplate and the onSave invocation to perform this null check.
---
Duplicate comments:
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx`:
- Around line 246-249: The previous crash risk from accessing newConnection.name
is already addressed by optional chaining; to be robust, ensure
setIsSavingConnection(false) always runs (move it out of the if-block or use
finally-style flow) and when invoking onClose use a clear fallback for
recentIdentifier (e.g., recentIdentifier: newConnection?.name ?? undefined) or
skip calling onClose when newConnection is undefined; update the logic around
response.artifacts, newConnection, setIsSavingConnection, and onClose to reflect
this.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx`:
- Around line 49-54: The reviewer flagged a duplicate comment; to tidy the code,
ensure the config variable handling in DevantConfigurables.tsx is deterministic
by giving configVars a safe default and removing the unnecessary optional
chaining: change the declaration of configVars (the variable named configVars
computed from data.configVariables[...] as ConfigVariable[]) to default to an
empty array (e.g., const configVars = (... ) as ConfigVariable[] || []) and then
call configVars.forEach(...) with configNames.push(...) (referencing the
existing push call and properties.variable.value access), and remove the
redundant reviewer note so the PR no longer contains duplicate comments.
---
Nitpick comments:
In `@workspaces/ballerina/ballerina-side-panel/src/components/NodeList/index.tsx`:
- Around line 681-687: The duplicated creation of the identical handlers map
(onAddConnection: handleAddConnection, onAddFunction: handleAddFunction, onAdd:
handleAdd, onLinkDevantProject: handleOnLinkDevantProject,
onRefreshDevantConnections: handleOnRefreshDevantConnections) should be hoisted
out of the loop and defined once — move the handlers object to component scope
or to the top of getCategoryContainer and reuse that single reference in both
places (instead of rebuilding it at lines where handlers is currently created),
ensuring any closures still have access to the same handler functions.
- Line 706: Replace the unnecessary optional chaining for the map callback
parameter by using the direct property access: in the map where you render
<Codicon ... />, change the prop to use action.codeIcon || "add" (the callback
parameter is never undefined; earlier access uses handlers[action.handlerKey]
without ?), and make the same change for the other occurrence around the Codicon
usage at the later line.
- Around line 478-488: The wrapper handlers handleOnLinkDevantProject and
handleOnRefreshDevantConnections contain redundant null guards before invoking
their props; remove the inner conditional checks and call onLinkDevantProject()
and onRefreshDevantConnections() directly inside those functions, since the
component already returns early when handlers are missing (so the props are
guaranteed present at call time). Ensure you only modify the bodies of
handleOnLinkDevantProject and handleOnRefreshDevantConnections to eliminate the
if-checks and directly invoke the prop functions.
In `@workspaces/ballerina/ballerina-visualizer/src/utils/bi.tsx`:
- Around line 159-171: Define a typed extension for the ad-hoc fields: add an
interface like DevantEnrichedCategory extends PanelCategory { devant:
DevantConnectionType; unusedDevantConn: boolean; isLoading?: boolean } (use the
actual type for devant from your connections array). Replace the implicit
any/spread usages where you create enriched category objects — e.g., the map
that returns { ...categoryItem, devant: matchingDevantConn, unusedDevantConn:
false } and the unusedCategoryItems const — with explicit annotations/returns of
DevantEnrichedCategory[] and ensure the created objects conform to that
interface; update any function signatures or local variables (such as the mapped
result and unusedCategoryItems) to use DevantEnrichedCategory so consumers can
access devant, unusedDevantConn, and isLoading without type assertions.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/AddConnectionPopup/AddConnectionPopupContent.tsx`:
- Line 85: The fetchConnectors function includes an unused parameter filter
(signature: fetchConnectors(filter?: boolean)) which never gets passed by
callers and overrides the intended filterByCurrentOrg behavior; remove the dead
parameter from the function signature and any internal branching that reads it
so the function consistently uses filterByCurrentOrg (or, if the original intent
was to allow an override, update all call sites to pass the intended boolean).
Specifically, update the fetchConnectors declaration to drop the filter param,
remove references to that param inside fetchConnectors, and ensure the logic
uses the existing filterByCurrentOrg flag to compute filters for connector
retrieval.
- Around line 223-252: The filteredCategories computation (using
cloneDeep(connectors).map(...).filter(...)) is expensive and currently runs
every render; wrap it in React.useMemo to memoize results and only recompute
when its inputs change—useMemo(() => { /* existing cloneDeep + map + filter
pipeline using filterItems */ }, [connectors, searchText, filterType]); ensure
you reference the same identifiers (filteredCategories, cloneDeep, filterItems,
connectors, searchText, filterType) inside the memo callback so behavior is
unchanged but recomputation only happens when those dependencies change.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/AddConnectionPopup/styles.ts`:
- Around line 22-29: Remove the duplicate styled exports PopupContent and
BackButton from this file and instead import and re-export them from the parent
module where they are originally defined (the sibling Connection/styles.ts);
update the top of this file to import { PopupContent, BackButton } from the
parent module and then export them (or use direct re-exports) so the local file
no longer contains duplicate declarations for PopupContent and BackButton.
- Around line 167-187: The interpolations inside the FilterButton styled
component duplicate prop typings—remove the inline `(props: { active?: boolean
})` annotations in all interpolations (font-weight, background-color, color, and
&:hover) and use the already-declared generic props from `styled.button<{
active?: boolean }>` by referencing `props => props.active` (or simply `({
active }) => active`) so the component `FilterButton` relies on the single
generic typing and avoids redundant annotations while keeping ThemeColors
references intact.
- Around line 206-217: ConnectorDetailCard currently types its props via an
inline cast inside the template which doesn't provide proper TS checking or
prevent unknown HTML attributes from being forwarded; fix by declaring the prop
type on the styled-component generic (e.g. change styled.div`...` to
styled.div<{ disabled?: boolean }>`...` and update the opacity line to use
props.disabled without a cast) or alternatively use a transient prop (e.g.
$disabled) in the generic and template to ensure the boolean is typed and not
forwarded to the DOM; update the ConnectorDetailCard declaration and remove the
inline (props: { disabled?: boolean }) cast.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx`:
- Around line 263-271: The renderStepper function returns a React fragment that
wraps a single StepperContainer element; remove the unnecessary fragment so
renderStepper directly returns <StepperContainer> (keep the existing Stepper
component, props, and StepperContainer wrapper) to simplify the JSX and avoid
redundant nodes in renderStepper.
- Around line 54-64: The styled component StepContent redundantly sets flex: 1
both unconditionally and again inside the fillHeight conditional; remove the
duplicate flex: 1 from the conditional block in StepContent (the styled.div
definition) and keep only the min-height: 0 and height: 100% lines when
props.fillHeight is true so the fillHeight behavior is preserved without
repeating flex: 1.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx`:
- Around line 40-58: The queryKey for the useQuery call in
DevantConfigurables.tsx is too generic and can return cached config names from
another project; fetch the visualizer/project identifier first (via
rpcClient.getVisualizerLocation() or the derived projectPath) and include that
identifier in the queryKey (e.g., ["config-variables", projectPath]) so that
useQuery (and functions like getConfigVariablesV2) are scoped per project and
will refetch when the project/visualizer location changes; ensure you compute
the projectPath before calling useQuery or derive it in a surrounding hook and
pass it into useQuery's queryKey.
...allerina-visualizer/src/views/BI/Connection/AddConnectionPopup/AddConnectionPopupContent.tsx
Show resolved
Hide resolved
...allerina-visualizer/src/views/BI/Connection/AddConnectionPopup/AddConnectionPopupContent.tsx
Show resolved
Hide resolved
...allerina-visualizer/src/views/BI/Connection/AddConnectionPopup/AddConnectionPopupContent.tsx
Show resolved
Hide resolved
...allerina-visualizer/src/views/BI/Connection/AddConnectionPopup/AddConnectionPopupContent.tsx
Show resolved
Hide resolved
workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/APIConnectionPopup/index.tsx
Show resolved
Hide resolved
workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx
Show resolved
Hide resolved
workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx
Show resolved
Hide resolved
...aces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx
Outdated
Show resolved
Hide resolved
...aces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx
Show resolved
Hide resolved
axewilledge
approved these changes
Feb 18, 2026
kanushka
approved these changes
Feb 18, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/utils.ts-149-155 (1)
149-155:⚠️ Potential issue | 🟡 MinorMisleading comment: lookup is case-sensitive, not case-insensitive.
The comment on Line 150 says "case-insensitive" but
Set.has()performs case-sensitive comparison. No lowercasing is applied when adding names or checking. Either update the comment or implement actual case-insensitive comparison if that's the intended behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/utils.ts` around lines 149 - 155, The comment claims a case-insensitive lookup but the Set existingNames is populated and used case-sensitively; fix by normalizing names to a canonical case (e.g., .toLowerCase()) when building existingNames (apply to biConnectorNames, devantConnectorNames and the replaceAll mapped entries) and ensure any lookup against existingNames also lowercases the candidate name; alternatively, if you prefer to keep case-sensitive behavior, update the misleading comment instead—refer to the existingNames variable and the places that populate it (biConnectorNames, devantConnectorNames, and the map with replaceAll).workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorCreateForm.tsx-362-362 (1)
362-362:⚠️ Potential issue | 🟡 MinorTypo: "Collapsed" should be "Collapse".
The button label should use the imperative form "Collapse" to indicate an action the user can take, not a past state.
✏️ Fix typo
- Collapsed + Collapse🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorCreateForm.tsx` at line 362, The button label in the DevantConnectorCreateForm component is using the past-tense string "Collapsed" instead of the imperative "Collapse"; locate the JSX/TSX element inside DevantConnectorCreateForm (search for the text "Collapsed" or the button that toggles collapse state) and change the displayed label to "Collapse" so the button reads as an action rather than a state.workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorMarketplaceInfo.tsx-152-178 (1)
152-178:⚠️ Potential issue | 🟡 Minor
itemaccessed without null-check; effect dependency list is incomplete.Inside this
useEffect,item.isThirdParty(Lines 156, 166) is accessed without a guard, butitemis typed asMarketplaceItem | undefined. Although currently safe because the query is disabled whenitemis falsy, staleserviceIdldata from a prior fetch could trigger the effect afteritembecomesundefined.Additionally, the dependency array
[serviceIdl, serviceIdlError]omitsitem,importedConnection, andonFlowChange, which are all read inside the effect.🛡️ Proposed fix
useEffect(() => { + if (!item) return; if (serviceIdlError || (serviceIdl && (serviceIdl?.idlType !== "OpenAPI" || !serviceIdl?.content))) { let newFlow: DevantConnectionFlow; // ... rest of logic onFlowChange(newFlow); } - }, [serviceIdl, serviceIdlError]); + }, [serviceIdl, serviceIdlError, item, importedConnection, onFlowChange]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorMarketplaceInfo.tsx` around lines 152 - 178, The effect reads item.isThirdParty, importedConnection and calls onFlowChange but doesn't guard against item being undefined and its dependency array is incomplete; update the useEffect in DevantConnectorMarketplaceInfo.tsx to first return early if item is falsy (or otherwise null-check item before accessing item.isThirdParty) and include item, importedConnection, and onFlowChange in the dependency array alongside serviceIdl and serviceIdlError; keep the existing DevantConnectionFlow logic and serviceIdl/idType checks but ensure the guard prevents accessing item when undefined and triggers correctly when any of the referenced values change.workspaces/ballerina/ballerina-extension/src/rpc-managers/platform-ext/rpc-manager.ts-548-551 (1)
548-551:⚠️ Potential issue | 🟡 MinorTypo in user-facing error message: "Pease" → "Please".
Fix
- "Pease associate your directory with Devant project in order to connect to Devant while running or debugging", + "Please associate your directory with Devant project in order to connect to Devant while running or debugging",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/rpc-managers/platform-ext/rpc-manager.ts` around lines 548 - 551, Update the user-facing error string passed to window.showErrorMessage in rpc-manager.ts: change the typo "Pease" to "Please" in the message ("Pease associate your directory..." → "Please associate your directory...") where window.showErrorMessage is called (the call that also includes the "Manage Project" action) so the displayed prompt reads correctly.workspaces/ballerina/ballerina-extension/src/rpc-managers/platform-ext/rpc-manager.ts-332-339 (1)
332-339:⚠️ Potential issue | 🟡 MinorCopy-paste error in log messages across multiple methods.
Line 337 logs "Failed to delete connection config" but the method is
getDevantConsoleUrl. The same incorrect message appears instopProxyServer(Line 373) andstartProxyServer(Line 600).Proposed fix for all three
async getDevantConsoleUrl(): Promise<string> { try { const platformExt = await this.getPlatformExt(); return await platformExt?.getDevantConsoleUrl(); } catch (err) { - log(`Failed to delete connection config: ${err}`); + log(`Failed to get Devant console URL: ${err}`); } }async stopProxyServer(params: StopProxyServerReq): Promise<void> { try { const platformExt = await this.getPlatformExt(); return platformExt?.stopProxyServer(params); } catch (err) { - log(`Failed to delete connection config: ${err}`); + log(`Failed to stop proxy server: ${err}`); } }} catch (err) { - log(`Failed to delete connection config: ${err}`); + log(`Failed to start proxy server: ${err}`); return { envVars: {}, proxyServerPort: 0, requiresProxy: false }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/rpc-managers/platform-ext/rpc-manager.ts` around lines 332 - 339, The three methods getDevantConsoleUrl, stopProxyServer, and startProxyServer contain a copy-paste log message "Failed to delete connection config"; update each catch block to log the correct context-specific message (e.g., "Failed to get Devant console URL" in getDevantConsoleUrl, "Failed to stop proxy server" in stopProxyServer, "Failed to start proxy server" in startProxyServer) and include the caught error details (err) in the process/log call; ensure the error logging uses the same logger variable used elsewhere in the file and keep the method signatures/returns intact (include the error in the log string or pass err as a second argument to the logger).workspaces/ballerina/ballerina-visualizer/src/views/BI/PlatformExtPopover/PlatformExtPopover.tsx-327-331 (1)
327-331:⚠️ Potential issue | 🟡 MinorTriple negation
!!!is likely a typo — equivalent to single!.
!!!platformExtState?.devantConns?.connectedToDevantis functionally identical to!platformExtState?.devantConns?.connectedToDevant, but the triple!suggests a mistake.Fix
platformRpcClient.setConnectedToDevant( - !!!platformExtState?.devantConns?.connectedToDevant, + !platformExtState?.devantConns?.connectedToDevant, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/PlatformExtPopover/PlatformExtPopover.tsx` around lines 327 - 331, Replace the accidental triple negation in the onChange handler: change the argument passed to platformRpcClient.setConnectedToDevant from !!!platformExtState?.devantConns?.connectedToDevant to a single logical negation (!platformExtState?.devantConns?.connectedToDevant) so the boolean flip is clear and not confusing; verify this occurs inside the onChange callback near the call to platformRpcClient.setConnectedToDevant.workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx-136-139 (1)
136-139:⚠️ Potential issue | 🟡 MinorCase-insensitive duplicate check is overly restrictive for Ballerina's case-sensitive identifiers.
myConfigandMyConfigare distinct Ballerina identifiers, but the current.toLowerCase()comparison would rejectMyConfigifmyConfigalready exists. Change to a case-sensitive comparison:🐛 Proposed fix
- if (existingNames.some((existing) => existing.toLowerCase() === nameValue.toLowerCase())) { + if (existingNames.some((existing) => existing === nameValue)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx` around lines 136 - 139, The duplicate-name check in DevantConfigurables.tsx is using a case-insensitive comparison (existing.toLowerCase() === nameValue.toLowerCase()) which incorrectly rejects distinct Ballerina identifiers; update the check in the block that uses existingNames.some(...) to perform a case-sensitive comparison (e.g., existing === nameValue) so "myConfig" and "MyConfig" are treated as different identifiers, leaving the error string unchanged.
🧹 Nitpick comments (8)
common/config/rush/.pnpmfile.cjs (1)
52-63: Remaining duplicate override entries fordiff,eslint, andlodash.This PR already cleaned up the redundant
fast-xml-parserblock from both sections. The same deduplication was not applied todiff,eslint, andlodash, which each appear twice in both thedependenciesanddevDependenciesblocks with identical values. While functionally harmless today, diverging values in a future edit on only one copy would silently produce an inconsistency.♻️ Proposed cleanup — remove second occurrence of each duplicate in both sections
if (pkg.dependencies['diff']) { pkg.dependencies['diff'] = '^8.0.3'; } if (pkg.dependencies['eslint']) { pkg.dependencies['eslint'] = '^9.27.0'; } if (pkg.dependencies['fast-xml-parser']) { pkg.dependencies['fast-xml-parser'] = '5.3.6'; } if (pkg.dependencies['lodash']) { pkg.dependencies['lodash'] = '4.17.23'; } if (pkg.dependencies['qs']) { pkg.dependencies['qs'] = '6.14.2'; } - if (pkg.dependencies['diff']) { - pkg.dependencies['diff'] = '^8.0.3'; - } - if (pkg.dependencies['eslint']) { - pkg.dependencies['eslint'] = '^9.27.0'; - } if (pkg.dependencies['hono']) { pkg.dependencies['hono'] = '^4.11.7'; } - if (pkg.dependencies['lodash']) { - pkg.dependencies['lodash'] = '4.17.23'; - }Apply the same deduplication symmetrically to the
devDependenciesblock (lines 100–111).Also applies to: 100-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/config/rush/.pnpmfile.cjs` around lines 52 - 63, The dependencies block contains duplicate override assignments for pkg.dependencies['diff'], pkg.dependencies['eslint'], and pkg.dependencies['lodash'] (and the same duplicates exist in pkg.devDependencies); remove the redundant second occurrence of each of these three keys in both the dependencies and devDependencies sections so each package override is defined only once (leave the existing single assignment with the intended version and delete the duplicate assignments to avoid future divergence).workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorPopup.tsx (1)
56-56: Unused import:setfrom lodash.
setis imported but never used in this file.♻️ Remove unused import
-import { set } from "lodash";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorPopup.tsx` at line 56, Remove the unused import "set" from lodash in DevantConnectorPopup.tsx by deleting the import specifier (import { set } from "lodash";) so the file no longer imports an unused symbol; ensure no other code in the file references "set" before removing.workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorList.tsx (1)
68-258: Service-type-to-available-node mapping is duplicated.The
handleMarketplaceItemClicklogic (Lines 87-118) that mapsitem.serviceTypeto anavailableNodeand selects aDevantConnectionFlowis nearly identical to thehandleInitImportConnectormutation inDevantConnectorPopup.tsx(Lines 104-145). Consider extracting a shared helper to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorList.tsx` around lines 68 - 258, The mapping logic that turns a MarketplaceItem.serviceType into an AvailableNode and chooses a DevantConnectionFlow is duplicated between DevantConnectorList.handleMarketplaceItemClick and DevantConnectorPopup.handleInitImportConnector; extract a shared helper (e.g., resolveConnectorFlow) that accepts (item: MarketplaceItem, type: DevantConnectionType, balOrgCategories: typeof balOrgConnectors?.categories) and returns { availableNode?: AvailableNode, flow: DevantConnectionFlow }; implement the helper using getKnownAvailableNode and the same REST/GRAPHQL/SOAP/GRPC cases and flow selection rules, place it in a shared utility module, and replace the inline logic in both functions to call this helper and then call onItemSelect or dispatch the flow with the returned values.workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/platform-ext/platform-ext-client.ts (1)
124-126: Minor inconsistency: missingundefinedparameter.Other void-request methods (e.g.,
refreshConnectionListat Line 109,getDevantConsoleUrlat Line 97) explicitly passundefinedas the third argument, butdeployIntegrationInDevantomits it.Fix for consistency
deployIntegrationInDevant(): Promise<void> { - return this._messenger.sendRequest(deployIntegrationInDevant, HOST_EXTENSION); + return this._messenger.sendRequest(deployIntegrationInDevant, HOST_EXTENSION, undefined); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/platform-ext/platform-ext-client.ts` around lines 124 - 126, The deployIntegrationInDevant method is inconsistent with other void-request methods by omitting the explicit undefined third argument; update deployIntegrationInDevant to call this._messenger.sendRequest(deployIntegrationInDevant, HOST_EXTENSION, undefined) so it matches the other methods like refreshConnectionList and getDevantConsoleUrl and preserves the explicit third-argument pattern used for void requests.workspaces/ballerina/ballerina-visualizer/src/views/BI/PlatformExtPopover/PlatformExtPopover.tsx (2)
192-203: Magic number-1for separator kind — prefer the enum for clarity.
{ kind: -1, label: "..." }works becauseQuickPickItemKind.Separatoris-1, but using the enum value directly would be more readable and less fragile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/PlatformExtPopover/PlatformExtPopover.tsx` around lines 192 - 203, Replace the magic number -1 used for the separator in quickPickOptions with the QuickPickItemKind enum to improve readability and robustness: locate the quickPickOptions array in PlatformExtPopover.tsx and change the separator item from { kind: -1, label: "Associated Integrations" } to use QuickPickItemKind.Separator (ensure QuickPickItemKind is imported/available) so the separator is expressed as QuickPickItemKind.Separator while leaving the surrounding items and mapping of platformExtState?.components unchanged.
92-97: Interface nameDiagnosticsPopUpPropsseems misnamed for this component.This is a
PlatformExtPopover, not a diagnostics popup. The name appears to be a leftover from copy-paste.Suggested rename
-export interface DiagnosticsPopUpProps { +export interface PlatformExtPopoverProps { isVisible: boolean; anchorEl: HTMLElement; onClose: () => void; projectPath?: string; } -export function PlatformExtPopover(props: DiagnosticsPopUpProps) { +export function PlatformExtPopover(props: PlatformExtPopoverProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/PlatformExtPopover/PlatformExtPopover.tsx` around lines 92 - 97, The interface name DiagnosticsPopUpProps is a leftover and should be renamed to reflect this component (e.g., PlatformExtPopoverProps); update the interface declaration name and all usages (the PlatformExtPopover component props type, any exports or imports referencing DiagnosticsPopUpProps, and any prop-typed variables) to the new name so the type matches the component purpose and avoids confusion.workspaces/ballerina/ballerina-extension/src/rpc-managers/platform-ext/rpc-manager.ts (1)
269-321: Silently returningundefinedon error for typed return promises.Methods like
getMarketplaceItems,getMarketplaceItem,getMarketplaceIdl,getConnections,getConnection, andgetComponentListall catch errors, log them, and implicitly returnundefined. But their return types promise specific objects (e.g.,Promise<MarketplaceListResp>). Callers may not guard againstundefined, leading to downstream NPEs.Consider either re-throwing (to let the caller handle it) or returning a safe default value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-extension/src/rpc-managers/platform-ext/rpc-manager.ts` around lines 269 - 321, These RPC wrapper methods (getMarketplaceItems, getMarketplaceItem, getMarketplaceIdl, getConnections, getConnection, getComponentList) log caught errors and then implicitly return undefined which violates their declared Promise<T> signatures; update each catch block to either re-throw the error (e.g., throw err) so callers receive the failure, or return a safe default matching the declared type (e.g., empty list or null object) depending on intended behavior — pick one consistent strategy and apply it to platformExt?.getMarketplaceItems, platformExt?.getMarketplaceItem, platformExt?.getMarketplaceIdl, platformExt?.getConnections, platformExt?.getConnection, and platformExt?.getComponentList so the functions never resolve to undefined.workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx (1)
40-57: StaticqueryKeymay serve stale data when the project context changes.
queryKey: ["config-variables"]does not include any project-specific identifier (e.g.,projectPath, ororg/name). If the active project changes without remounting this component, the cached result from a prior project will be returned. Include the project path in the key:♻️ Proposed refactor
+ const visualizerLocation = await rpcClient.getVisualizerLocation(); const { data: existingConfigVariables = [] } = useQuery({ queryFn: async () => { - const visualizerLocation = await rpcClient.getVisualizerLocation(); const data = await rpcClient.getBIDiagramRpcClient().getConfigVariablesV2({ projectPath: visualizerLocation?.projectPath || "", includeLibraries: false, }); const configNames: string[] = []; const projectToml = await rpcClient.getCommonRpcClient().getCurrentProjectTomlValues(); const configVars = (data.configVariables as any)?.[ `${projectToml?.package?.org}/${projectToml?.package?.name}` ]?.[""] as ConfigVariable[]; configVars?.forEach((configVar) => configNames.push(configVar?.properties?.variable?.value?.toString() || ""), ); return configNames; }, - queryKey: ["config-variables"], + queryKey: ["config-variables", visualizerLocation?.projectPath ?? ""], });Note:
visualizerLocationwould need to be resolved outside theuseQuerycall (e.g. fetched once on mount) so it can be referenced in both the key and thequeryFn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx` around lines 40 - 57, Resolve the visualizer/project context before calling useQuery (e.g., call rpcClient.getVisualizerLocation() or obtain projectPath/projectToml on mount) and then include a project-specific identifier in the useQuery key (replace queryKey: ["config-variables"] with something like ["config-variables", projectPath or `${projectToml.package.org}/${projectToml.package.name}`]) so the cache is scoped per project; update the queryFn in useQuery to use the already-resolved visualizerLocation/projectToml and keep the same logic that calls rpcClient.getBIDiagramRpcClient().getConfigVariablesV2 and extracts configNames.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.trivyignore:
- Around line 11-12: Update the .trivyignore comment for CVE-2025-69873 to
correct the false claim that no fix exists: state that ajv was patched in
v8.18.0 (see GHSA-2g4f-4pwh-qvx6 / release v8.18.0) and that the vulnerability
affects ajv ≤ 8.17.1 via unchecked $data passed to RegExp; keep the current
rationale that ajv is only a nested build-time dependency and $data is not
enabled in our context, and add a tracking-issue reference (mirror the pattern
used for CVE-2020-36851) so maintainers can revisit upgrading transitive deps to
ajv ≥ 8.18.0.
In
`@workspaces/ballerina/ballerina-extension/src/rpc-managers/platform-ext/rpc-manager.ts`:
- Around line 719-723: The code assigns keys[entry.id].envName from
connectionKeys[entry.id].envVariableName but doesn't guard against
connectionKeys[entry.id] being undefined; update the assignment in
rpc-manager.ts to first check that connectionKeys[entry.id] exists (or use
optional chaining) and provide a safe fallback (e.g., null, empty string, or a
default name) for envName when connectionKeys[entry.id] is undefined so
accessing envVariableName cannot throw a TypeError.
- Around line 962-1010: The method replaceDevantTempConfigValues currently lacks
error handling — wrap its entire implementation in a try/catch, catch any
exceptions thrown by calls like
StateMachine.context().langClient.getSyntaxTree(...) and updateSourceCode(...),
log a clear contextual error (including the caught error) using the existing
logger (e.g., StateMachine.context().processLogger or the class logger), and
rethrow or propagate a controlled error consistent with other public RPC methods
in this class so failures don't become unhandled rejections.
In
`@workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/platform-ext/platform-ext-client.ts`:
- Around line 104-106: The method onPlatformExtStoreStateChange currently
discards the Disposable returned by this._messenger.onNotification, causing
listener leaks; change onPlatformExtStoreStateChange to return the Disposable
(e.g., update its signature to return Disposable) and return the result of
this._messenger.onNotification(onPlatformExtStoreStateChange, callback); also
ensure the PlatformExtAPI interface (and any other similar notification methods)
is updated for consistency so callers can unsubscribe.
In
`@workspaces/ballerina/ballerina-visualizer/src/providers/platform-ext-ctx-provider.tsx`:
- Around line 64-68: The effect currently subscribes via
platformRpcClient.onPlatformExtStoreStateChange but never unsubscribes, leaking
listeners on remount; update the useEffect in platform-ext-ctx-provider.tsx to
capture the disposable/unsubscribe returned from
platformRpcClient.onPlatformExtStoreStateChange and return a cleanup function
that calls it (e.g., const dispose =
platformRpcClient?.onPlatformExtStoreStateChange(...); return () =>
dispose?.();), and ensure the PlatformExtRpcClient.onPlatformExtStoreStateChange
implementation is changed to return that disposable/unsubscribe function so the
cleanup can remove the listener; keep the queryClient.setQueryData call as-is
inside the subscription callback.
- Around line 75-91: The click handler onLinkDevantProject has inverted and
unreachable logic: ensure the showInformationModal items include both "Login"
and "Associate Project" and swap the response handlers so "Login" triggers the
sign-in flow (call rpcClient.getCommonRpcClient().executeCommand with
PlatformExtCommandIds.SignIn and { extName: "Devant" }) and "Associate Project"
triggers deployment (call platformRpcClient.deployIntegrationInDevant()); update
the items array and the then(resp => { ... }) branches in onLinkDevantProject
accordingly.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantBIConnectorSelect.tsx`:
- Line 109: The map over connectorList in the DevantBIConnectorSelect component
can throw if connectorList is undefined; ensure connectorList is always an array
before mapping by either providing a default when destructuring data (e.g., set
connectorList = [] in the const { ... } = data) or by changing the map site to
use a safe fallback (e.g., (connectorList || [])). Update the place where data
is destructured or the JSX that uses connectorList.map to guarantee an array so
connectorList.map(...) cannot throw.
- Line 97: The render check in DevantBIConnectorSelect uses connectorList.length
which can throw if connectorList is undefined; guard it by treating
connectorList as an array (e.g., use connectorList ?? [] or
connectorList?.length with a null coalescing fallback) when checking emptiness,
or provide a default empty array when destructuring the query result so the
condition becomes safe (refer to connectorList and the render branch that
currently does connectorList.length === 0).
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorCreateForm.tsx`:
- Line 266: The code accesses marketplaceItem.isThirdParty without guarding for
undefined in the DevantConnectorCreateForm component; add a null check (or
optional chaining) before reading isThirdParty so the render path handles
marketplaceItem === undefined safely — e.g., update the conditional around
marketplaceItem.isThirdParty (referencing marketplaceItem and the
DevantConnectorCreateForm render/JSX) to check marketplaceItem != null (or use
marketplaceItem?.isThirdParty) and handle the undefined case (early return,
loading state, or default behavior) to prevent the TypeError.
- Around line 198-214: The mutation's mutationFn currently ignores the user's
selected auth scheme by hardcoding serviceSchemaId to
marketplaceItem?.connectionSchemas[0]?.id; update the createInternalConnection
call inside mutationFn (the useMutation handler) to use the form payload first:
set serviceSchemaId to the user's selection (data.schemaId) falling back to
marketplaceItem?.connectionSchemas[0]?.id and then to empty string (i.e.
serviceSchemaId: data.schemaId || marketplaceItem?.connectionSchemas[0]?.id ||
""); keep all other fields and names (createInternalConnection, mutationFn,
marketplaceItem, data) unchanged.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorList.tsx`:
- Around line 87-99: The handler handleMarketplaceItemClick currently calls
getKnownAvailableNode with balOrgConnectors?.categories which can be undefined
while the query is loading; guard against this by checking
loadingBalOrgConnectors or the presence of balOrgConnectors before performing
lookups (or early-return) and disable/ignore clicks until data loads. Update the
click wiring so marketplace items are disabled when loadingBalOrgConnectors is
true (or add an immediate if (!balOrgConnectors) return at the top of
handleMarketplaceItemClick) to avoid passing undefined categories into
getKnownAvailableNode.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorPopup.tsx`:
- Around line 244-252: The loop iterates over (stResp?.syntaxTree as
ModulePart)?.members which may be undefined and causes a TypeError; guard the
iteration by first extracting members into a local variable (e.g., const members
= (stResp?.syntaxTree as ModulePart)?.members ?? []) or check if members is
truthy before the for...of, then iterate over members and keep the existing
checks that call STKindChecker and update biConnectionNames; this prevents
for...of from running on undefined while preserving the current logic in
DevantConnectorPopup.
- Around line 334-354: The mutation's mutationFn can return undefined when
selectedFlow === DevantConnectionFlow.IMPORT_INTERNAL_OAS because
importInternalOASConnection()'s result isn't returned, causing onSuccess to
dereference data and throw; fix by returning the awaited result from
importInternalOASConnection() inside generateCustomConnectorFromOAS's mutationFn
when that flow is selected (so the mutation resolves with the connection data),
and also defensively guard onSuccess (the onSuccess handler in the useMutation
for generateCustomConnectorFromOAS) by checking if data is truthy before calling
setAvailableNode(data.connectionNode) and goToNextStep() to avoid NPEs if data
is absent.
- Around line 165-170: configVars can be undefined before calling .forEach,
causing an NPE; update the logic around configVars (the extraction from
configResp.configVariables into the const configVars) to guard against undefined
by defaulting to an empty array or checking truthiness before iterating, e.g.,
ensure the variable derived from configResp.configVariables (configVars) is set
to [] when missing and then iterate over it, referencing the same identifiers
configResp.configVariables, configVars, and existingConfigs so
existingConfigs.add(...) only runs for actual items.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/utils.ts`:
- Around line 203-209: getKnownAvailableNode can throw when categories or the
"Network" category is missing; guard against undefined before accessing items.
Modify getKnownAvailableNode to first check categories and the result of
categories.find(...) (networkConnectors) and return undefined if either is
falsy, or use safe chaining (e.g., networkConnectors?.items?.find(...)) so you
never access .items on undefined; keep the returned type as AvailableNode |
undefined and reference the existing function name getKnownAvailableNode and the
variable networkConnectors when applying the null checks.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx`:
- Line 21: The file imports only useState but later references the React
namespace via React.FC<DevantNewConfigurableFormProps> (on the
DevantNewConfigurableForm component); fix by either adding the React namespace
import (import React from "react") alongside useState so React.FC resolves, or
remove the React.FC annotation and type the component as a plain function (e.g.,
declare DevantNewConfigurableForm as (props: DevantNewConfigurableFormProps) =>
JSX.Element) ensuring DevantNewConfigurableFormProps and any JSX types remain
correctly imported/available.
---
Duplicate comments:
In
`@workspaces/ballerina/ballerina-extension/src/rpc-managers/platform-ext/rpc-manager.ts`:
- Around line 251-263: The debounced wrapper currently discards the return value
of initProjectPathWatcher so disposeProjectPathWatcher never gets the cleanup
function; change the debounce usage so debouncedInitProjectPathWatcher returns
the Promise<() => void> from initProjectPathWatcher (or avoid debouncing the
call that produces the disposable). Concretely, update the
debouncedInitProjectPathWatcher factory around initProjectPathWatcher so the
inner debounce callback captures and forwards the result (resolve the Promise
with the returned disposable) or replace it with a debounce implementation that
preserves return values; then keep the assignment disposeProjectPathWatcher =
await debouncedInitProjectPathWatcher(projectPath) inside the
StateMachine.service().subscribe handler so previous watchers are properly
disposed.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx`:
- Around line 49-54: The nested lookup for config variables can still produce
undefined and you should normalize it to an array instead of relying on optional
chaining on iteration; change the assignment to set configVars to an empty array
when the lookup fails (e.g. use the nullish coalescing fallback on the computed
lookup from data.configVariables and projectToml package keys) so you can
iterate without optional chaining, then iterate over configVars and push values
into configNames (referencing configVars, projectToml, data.configVariables, and
configNames).
---
Nitpick comments:
In `@common/config/rush/.pnpmfile.cjs`:
- Around line 52-63: The dependencies block contains duplicate override
assignments for pkg.dependencies['diff'], pkg.dependencies['eslint'], and
pkg.dependencies['lodash'] (and the same duplicates exist in
pkg.devDependencies); remove the redundant second occurrence of each of these
three keys in both the dependencies and devDependencies sections so each package
override is defined only once (leave the existing single assignment with the
intended version and delete the duplicate assignments to avoid future
divergence).
In
`@workspaces/ballerina/ballerina-extension/src/rpc-managers/platform-ext/rpc-manager.ts`:
- Around line 269-321: These RPC wrapper methods (getMarketplaceItems,
getMarketplaceItem, getMarketplaceIdl, getConnections, getConnection,
getComponentList) log caught errors and then implicitly return undefined which
violates their declared Promise<T> signatures; update each catch block to either
re-throw the error (e.g., throw err) so callers receive the failure, or return a
safe default matching the declared type (e.g., empty list or null object)
depending on intended behavior — pick one consistent strategy and apply it to
platformExt?.getMarketplaceItems, platformExt?.getMarketplaceItem,
platformExt?.getMarketplaceIdl, platformExt?.getConnections,
platformExt?.getConnection, and platformExt?.getComponentList so the functions
never resolve to undefined.
In
`@workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/platform-ext/platform-ext-client.ts`:
- Around line 124-126: The deployIntegrationInDevant method is inconsistent with
other void-request methods by omitting the explicit undefined third argument;
update deployIntegrationInDevant to call
this._messenger.sendRequest(deployIntegrationInDevant, HOST_EXTENSION,
undefined) so it matches the other methods like refreshConnectionList and
getDevantConsoleUrl and preserves the explicit third-argument pattern used for
void requests.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorList.tsx`:
- Around line 68-258: The mapping logic that turns a MarketplaceItem.serviceType
into an AvailableNode and chooses a DevantConnectionFlow is duplicated between
DevantConnectorList.handleMarketplaceItemClick and
DevantConnectorPopup.handleInitImportConnector; extract a shared helper (e.g.,
resolveConnectorFlow) that accepts (item: MarketplaceItem, type:
DevantConnectionType, balOrgCategories: typeof balOrgConnectors?.categories) and
returns { availableNode?: AvailableNode, flow: DevantConnectionFlow }; implement
the helper using getKnownAvailableNode and the same REST/GRAPHQL/SOAP/GRPC cases
and flow selection rules, place it in a shared utility module, and replace the
inline logic in both functions to call this helper and then call onItemSelect or
dispatch the flow with the returned values.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorPopup.tsx`:
- Line 56: Remove the unused import "set" from lodash in
DevantConnectorPopup.tsx by deleting the import specifier (import { set } from
"lodash";) so the file no longer imports an unused symbol; ensure no other code
in the file references "set" before removing.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx`:
- Around line 40-57: Resolve the visualizer/project context before calling
useQuery (e.g., call rpcClient.getVisualizerLocation() or obtain
projectPath/projectToml on mount) and then include a project-specific identifier
in the useQuery key (replace queryKey: ["config-variables"] with something like
["config-variables", projectPath or
`${projectToml.package.org}/${projectToml.package.name}`]) so the cache is
scoped per project; update the queryFn in useQuery to use the already-resolved
visualizerLocation/projectToml and keep the same logic that calls
rpcClient.getBIDiagramRpcClient().getConfigVariablesV2 and extracts configNames.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/PlatformExtPopover/PlatformExtPopover.tsx`:
- Around line 192-203: Replace the magic number -1 used for the separator in
quickPickOptions with the QuickPickItemKind enum to improve readability and
robustness: locate the quickPickOptions array in PlatformExtPopover.tsx and
change the separator item from { kind: -1, label: "Associated Integrations" } to
use QuickPickItemKind.Separator (ensure QuickPickItemKind is imported/available)
so the separator is expressed as QuickPickItemKind.Separator while leaving the
surrounding items and mapping of platformExtState?.components unchanged.
- Around line 92-97: The interface name DiagnosticsPopUpProps is a leftover and
should be renamed to reflect this component (e.g., PlatformExtPopoverProps);
update the interface declaration name and all usages (the PlatformExtPopover
component props type, any exports or imports referencing DiagnosticsPopUpProps,
and any prop-typed variables) to the new name so the type matches the component
purpose and avoids confusion.
workspaces/ballerina/ballerina-extension/src/rpc-managers/platform-ext/rpc-manager.ts
Show resolved
Hide resolved
workspaces/ballerina/ballerina-extension/src/rpc-managers/platform-ext/rpc-manager.ts
Show resolved
Hide resolved
workspaces/ballerina/ballerina-rpc-client/src/rpc-clients/platform-ext/platform-ext-client.ts
Show resolved
Hide resolved
workspaces/ballerina/ballerina-visualizer/src/providers/platform-ext-ctx-provider.tsx
Show resolved
Hide resolved
...rina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorPopup.tsx
Show resolved
Hide resolved
...rina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorPopup.tsx
Show resolved
Hide resolved
...rina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/DevantConnectorPopup.tsx
Show resolved
Hide resolved
workspaces/ballerina/ballerina-visualizer/src/views/BI/Connection/DevantConnections/utils.ts
Show resolved
Hide resolved
...aces/ballerina/ballerina-visualizer/src/views/BI/HelperPaneNew/Views/DevantConfigurables.tsx
Show resolved
Hide resolved
96 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
$subject
Related issue: https://github.com/wso2-enterprise/devant/issues/1938
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