Add VSCode Extension Support for RDBMS Connector #999
Conversation
|
|
|
Warning Rate limit exceeded@imedyaosandi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds dynamic-field and driver-management APIs and UI: new RPC types/endpoints, RPC client/manager/handler wiring, a DynamicFieldsHandler for form-driven SQL, driver configuration UI components, and FormGenerator integration to surface driver parameters and dynamic fields. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as FormGenerator/Driver UI
participant Handler as DynamicFieldsHandler
participant Client as ExtendedLanguageClient
participant Server as RPC_Server
User->>UI: change connection/driver or dynamic field
UI->>Handler: handleValueChange(field, value, element)
Handler->>Client: getDynamicFields / getStoredProcedures / loadDriverAndTestConnection
Client->>Server: synapse/* RPC request
Server-->>Client: RPC response
Client-->>Handler: response payload
Handler->>UI: update combos, field values, build queries
UI-->>User: render updated form
sequenceDiagram
participant User
participant DriverUI as DriverConfiguration
participant Form as FormGenerator
participant Client as ExtendedLanguageClient
participant Server as RPC_Server
User->>DriverUI: select mode / enter Maven coords / choose file
DriverUI->>Client: downloadDriverForConnector / getDriverMavenCoordinates / loadDriverAndTestConnection
Client->>Server: synapse/* RPC request
Server-->>Client: driver responses / test connection result
Client-->>DriverUI: results
DriverUI->>Form: update driver fields (groupId, artifactId, version, driverPath)
Form->>Form: include driver params in saved connection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
workspaces/mi/mi-diagram/src/index.tsx (1)
23-23: Remove or clarify commented-out exportThe commented export for
DatabaseDriverFormlooks like leftover code. To keep the public surface clear, either remove it or add a short TODO explaining when it should be re-enabled.workspaces/mi/mi-visualizer/src/views/Forms/ConnectionForm/ConnectionFormGenerator.tsx (1)
129-158: Tighten edit-flow parameter handling and avoid potential staleconnectionTypeThe added driver-parameter handling is useful, but a couple of tweaks would make this more robust:
connectionFound.parametersis assumed to exist; if an older/irregular connection lacks aparametersarray,parameters.filter(...)will throw. Consider defaulting to an empty array:const parameters = connectionFound.parameters ?? []; const driverParams = parameters.filter(/* ... */);
- When calling
reset, you use the statefulconnectionType, which may still have its previous value becausesetConnectionTypeis async. Using the found value avoids any race:- reset({ - name: props.connectionName, - connectionType: connectionType - }); + reset({ + name: props.connectionName, + connectionType: connectionFound.connectionType, + });These changes preserve the current behavior while making the edit path safer.
workspaces/mi/mi-diagram/src/components/sidePanel/connectors/AddConnector.tsx (2)
70-80: Clean up duplicatesetComboValuesand rely on the shared helperYou now define
setComboValuestwice: once at component scope (used and passed toFormGenerator) and once insidefetchConnectionsForTemplateConnection(never used). The inner definition shadows the outer one and is dead code.Recommend removing the inner definition and, if you intended to seed combo values when loading connections, explicitly call the shared helper instead, e.g.:
- const connectionsNames = connectionsData.connections.map(connection => connection.name); - setConnections(connectionsNames); + const connectionsNames = connectionsData.connections.map(connection => connection.name); + setConnections(connectionsNames); + // Optionally expose these in comboValuesMap if needed by DynamicFieldsHandler: + // setComboValues('configKey', connectionsNames);Also applies to: 88-93, 383-391
127-162: Side-panel parameter rehydration is good; consider avoiding mutation and hardening value handlingThe new effect that pulls
sidePanelContext.formValues.parametersback into the form and intoparamsis a nice improvement. A few small adjustments would make it safer:
- You mutate each
paramin-place (param.name = ...,param.namespaces = []). IfsidePanelContextreuses these objects elsewhere, this can have surprising side effects. Consider cloning instead:sidePanelContext.formValues.parameters.forEach((param: any) => { const normalizedName = getNameForController(param.name); if (param.isExpression) { // ... setValue(normalizedName, { isExpression: true, value, namespaces }); } else { setValue(normalizedName, { ...param, namespaces: [] }); } });
- For expression parameters you assume
param.valueis a string and call.replace(/[{}]/g, ''). If older data stores expressions under a different property (e.g.expression) orvalueis missing, this will throw. You already handle both ingenerateParamsviaparam.value ?? param.expression; consider doing the same when callingsetValue:const raw = param.value ?? param.expression ?? ''; const cleaned = String(raw).replace(/[{}]/g, ''); setValue(normalizedName, { isExpression: true, value: cleaned, namespaces: namespacesArray });
generateParams(sidePanelContext.formValues.parameters)is unguarded but you already check forsidePanelContext.formValues?.parametersin the outerif, so that part is fine.These tweaks keep the behavior but make the code more future-proof to changes in the shape of
formValues.parameters.Also applies to: 269-286
workspaces/mi/mi-diagram/src/components/Form/DBConnector/DriverConfiguration.tsx (1)
53-59: Tidy minor issues: reuse sharedcardStyleand avoid unused propsTwo small cleanups:
cardStyleis already exported fromFormGenerator.tsxwith the same definition. To avoid duplication (and keep layout changes in one place), you can import and reuse it here instead of redefining it.In
CustomDriverConfig,onConfigChangeis destructured but never used, which may tripno-unused-vars:-export const CustomDriverConfig: React.FC<DriverConfigProps> = ({ - config, - onConfigChange, +export const CustomDriverConfig: React.FC<DriverConfigProps> = ({ + config, onSave, onClear, onSelectLocation, isLoading, error }) => {Keeping the prop in the interface but not destructuring it is fine if this component doesn’t need to modify the config.
Also applies to: 111-119
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (1)
418-438: Double-check dynamic-field initialisation and error-precedence behavior.The dynamic-field plumbing looks coherent overall, but a couple of subtleties are worth confirming:
DynamicFieldsHandleris initialized only once (Lines 418–438) with the then-currentparametersanddynamicFields. Subsequent prop updates toparametersorconnectionNamewon’t be reflected unless a new handler is constructed. If those props can change during the form’s lifetime (e.g., when editing an existing connector vs. creating a new one), consider either:
- Re-instantiating the handler when those specific props change, or
- Passing them in via methods/refs rather than capturing them in the constructor.
handleValueChange(Lines 469–475) is only invoked ifelement.onValueChange?.functionis present, which is good. However, some initialisation paths rely on theuseEffectcalls that are currently flagged by Biome (e.g., Lines 980–986 for expression fields, and thecombo/connectionblocks), and those are likely to be refactored. When you move those hooks, ensure you preserve the “call once on mount with existing value” semantics so dynamic fields still hydrate correctly in edit flows.In
renderFormElement, custom errors take precedence (customErrors[name] ?? standardErrorMsg) for bothFormExpressionFieldComponentand standard fields (Lines ~980–986, 1048–1050, 1519–1520, 1566–1569). That matches the intended behavior withsetCustomError, but it also means any existing RHF validation messages will be fully hidden while a custom error is set. Confirm this is acceptable UX; otherwise, you may want to concatenate or otherwise surface both sources.No code changes strictly required here, but please revalidate the initialization flows once you address the hook-rule fixes.
Also applies to: 469-475, 980-986, 1048-1050, 1212-1264, 1519-1525, 1566-1569
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (2)
190-221: AddressnoSwitchDeclarationslint errors by scopingconstdeclarations per case.Biome reports multiple
noSwitchDeclarationserrors whereconstdeclarations live directly inswitchcases (e.g., inhandleValueChangeand the query-building/parsing switches). While the current code is functionally OK, the lint rule is there to prevent future accidental cross-case scope issues.Pattern to fix (example in
_buildQueryFromDynamicFields):- switch (operationType) { - case QUERY_TYPES.INSERT: - const insertColumns = Object.values(activeFields); - if (insertColumns.length > 0) { - // ... - } - break; - case QUERY_TYPES.DELETE: - query = `DELETE FROM ${encodedTableName}`; - // ... - break; - // ... - } + switch (operationType) { + case QUERY_TYPES.INSERT: { + const insertColumns = Object.values(activeFields); + if (insertColumns.length > 0) { + // ... + } + break; + } + case QUERY_TYPES.DELETE: { + query = `DELETE FROM ${encodedTableName}`; + // ... + break; + } + // ... + }Apply the same
{ ... }wrapping pattern to:
switch (config.function)inhandleValueChange.- The
switch (operationType)blocks in_buildQueryFromDynamicFieldsand_handleManualQueryChange.- Any other switch where you declare
const/letat case level.This will clear the Biome errors and make future modifications safer.
Also applies to: 1026-1154, 1358-1373, 1581-1603
10-11: Minor cleanup: remove unused import and consider reducing console noise.A few small polish items:
import { config } from 'process';(Line 10) is unused in this file and can be safely removed.- There are multiple
console.logcalls throughout (_getConnectorName,_getValidConnectionDetails,_buildQueryFromDynamicFields,_handleManualQueryChange, etc.). These are helpful during development but may clutter the extension’s output channel in production.Suggested cleanup:
-import { config } from 'process'; -import { Element, getNameForController, DynamicField, DynamicFieldGroup } from '../FormGenerator'; +import { Element, getNameForController, DynamicField, DynamicFieldGroup } from '../FormGenerator'; @@ - console.log("Validating connection:", connection.name); + // Optional: use a structured logger or remove in production @@ - console.log("Query and fields populated from existing parameters."); - console.log("Query:", query); - console.log("queryBuilt. ----",queryBuilt); - console.log("Element:", element); + // Consider removing or gating debug logs in production builds.Not functionally blocking, but cleaning this up will make diagnostics easier to read.
Also applies to: 552-555, 595-605, 782-785, 1184-1192, 1353-1381
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
workspaces/mi/mi-core/src/rpc-types/mi-diagram/index.ts(2 hunks)workspaces/mi/mi-core/src/rpc-types/mi-diagram/rpc-type.ts(2 hunks)workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts(2 hunks)workspaces/mi/mi-diagram/src/components/Form/DBConnector/DriverConfiguration.tsx(1 hunks)workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx(1 hunks)workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx(22 hunks)workspaces/mi/mi-diagram/src/components/Form/RadioButtonGroup/GenericRadioGroup.tsx(1 hunks)workspaces/mi/mi-diagram/src/components/sidePanel/connectors/AddConnector.tsx(5 hunks)workspaces/mi/mi-diagram/src/index.tsx(1 hunks)workspaces/mi/mi-extension/src/lang-client/ExtendedLanguageClient.ts(2 hunks)workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-handler.ts(2 hunks)workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts(2 hunks)workspaces/mi/mi-rpc-client/src/rpc-clients/mi-diagram/rpc-client.ts(2 hunks)workspaces/mi/mi-visualizer/src/views/Forms/ConnectionForm/ConnectionFormGenerator.tsx(3 hunks)workspaces/mi/mi-visualizer/src/views/Forms/DataServiceForm/SidePanelForms/GenerateResourcesForm.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-24T14:51:49.207Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 998
File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132
Timestamp: 2025-11-24T14:51:49.207Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx
📚 Learning: 2025-11-20T11:04:33.712Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 898
File: workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx:38-38
Timestamp: 2025-11-20T11:04:33.712Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx, the use of `useDMQueryClausesPanelStore.getState()` to access `clauseToAdd` and `setClauseToAdd` (instead of the hook subscription pattern) is intentional to prevent re-renders when these state values change.
Applied to files:
workspaces/mi/mi-diagram/src/components/sidePanel/connectors/AddConnector.tsx
🧬 Code graph analysis (9)
workspaces/mi/mi-core/src/rpc-types/mi-diagram/index.ts (1)
workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (10)
LoadDriverAndTestConnectionRequest(2306-2316)TestDbConnectionResponse(1875-1877)GetDynamicFieldsRequest(2268-2274)GetDynamicFieldsResponse(2276-2278)DSSFetchTablesRequest(1923-1929)GetStoredProceduresResponse(2280-2282)DriverDownloadRequest(2284-2288)DriverDownloadResponse(2290-2292)DriverMavenCoordinatesRequest(2293-2297)DriverMavenCoordinatesResponse(2299-2304)
workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (1)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (1)
DynamicField(160-172)
workspaces/mi/mi-diagram/src/components/Form/DBConnector/DriverConfiguration.tsx (3)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (1)
cardStyle(87-93)workspaces/common-libs/ui-toolkit/src/components/Codicon/Codicon.tsx (1)
Codicon(44-56)workspaces/common-libs/ui-toolkit/src/components/Button/Button.tsx (1)
Button(54-65)
workspaces/mi/mi-rpc-client/src/rpc-clients/mi-diagram/rpc-client.ts (3)
workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (10)
GetDynamicFieldsRequest(2268-2274)GetDynamicFieldsResponse(2276-2278)DSSFetchTablesRequest(1923-1929)GetStoredProceduresResponse(2280-2282)DriverDownloadRequest(2284-2288)DriverDownloadResponse(2290-2292)DriverMavenCoordinatesRequest(2293-2297)DriverMavenCoordinatesResponse(2299-2304)LoadDriverAndTestConnectionRequest(2306-2316)TestDbConnectionResponse(1875-1877)workspaces/mi/mi-core/src/rpc-types/mi-diagram/rpc-type.ts (5)
getDynamicFields(465-465)getStoredProcedures(466-466)downloadDriverForConnector(467-467)getDriverMavenCoordinates(468-468)loadDriverAndTestConnection(464-464)workspaces/mi/mi-extension/src/lang-client/ExtendedLanguageClient.ts (5)
getDynamicFields(487-489)getStoredProcedures(491-493)downloadDriverForConnector(495-497)getDriverMavenCoordinates(499-501)loadDriverAndTestConnection(483-485)
workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts (2)
workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (10)
GetDynamicFieldsRequest(2268-2274)GetDynamicFieldsResponse(2276-2278)DSSFetchTablesRequest(1923-1929)GetStoredProceduresResponse(2280-2282)DriverDownloadRequest(2284-2288)DriverDownloadResponse(2290-2292)LoadDriverAndTestConnectionRequest(2306-2316)TestDbConnectionResponse(1875-1877)DriverMavenCoordinatesRequest(2293-2297)DriverMavenCoordinatesResponse(2299-2304)workspaces/mi/mi-extension/src/stateMachine.ts (1)
getStateMachine(681-714)
workspaces/mi/mi-diagram/src/components/sidePanel/connectors/AddConnector.tsx (3)
workspaces/ballerina/ballerina-side-panel/src/components/ParamManager/ParamManager.tsx (1)
ParamConfig(46-50)workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (1)
getNameForController(179-184)workspaces/common-libs/ui-toolkit/src/components/ExpressionEditor/utils/token.ts (1)
setValue(103-145)
workspaces/mi/mi-core/src/rpc-types/mi-diagram/rpc-type.ts (1)
workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (10)
LoadDriverAndTestConnectionRequest(2306-2316)TestDbConnectionResponse(1875-1877)GetDynamicFieldsRequest(2268-2274)GetDynamicFieldsResponse(2276-2278)DSSFetchTablesRequest(1923-1929)GetStoredProceduresResponse(2280-2282)DriverDownloadRequest(2284-2288)DriverDownloadResponse(2290-2292)DriverMavenCoordinatesRequest(2293-2297)DriverMavenCoordinatesResponse(2299-2304)
workspaces/mi/mi-extension/src/lang-client/ExtendedLanguageClient.ts (1)
workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (7)
LoadDriverAndTestConnectionRequest(2306-2316)TestDbConnectionResponse(1875-1877)GetDynamicFieldsRequest(2268-2274)GetDynamicFieldsResponse(2276-2278)DriverDownloadRequest(2284-2288)DriverDownloadResponse(2290-2292)DriverMavenCoordinatesRequest(2293-2297)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (2)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (4)
DynamicFieldGroup(174-177)Element(119-153)getNameForController(179-184)DynamicField(160-172)workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (1)
DynamicField(2255-2266)
🪛 Biome (2.1.2)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx
[error] 895-896: This hook is being called from a nested function, but all hooks must be called unconditionally from the top-level component.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 928-929: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 983-984: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 1212-1213: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1213-1214: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1214-1215: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 1469-1470: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts
[error] 6086-6107: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
[error] 6111-6117: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
[error] 6121-6125: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
[error] 6130-6134: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
[error] 6138-6144: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
[error] 206-206: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1028-1028: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1095-1095: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1096-1096: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1097-1097: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1119-1119: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1138-1142: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1144-1144: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1259-1259: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1264-1264: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1269-1269: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1274-1274: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (12)
workspaces/mi/mi-visualizer/src/views/Forms/DataServiceForm/SidePanelForms/GenerateResourcesForm.tsx (1)
174-180: Confirm that sending an emptydriverPathis the intended contract
DSSFetchTablesRequestnow requiresdriverPath, and this call always sends an empty string. If the backend treats empty string as “no explicit driver path, use default”, this is fine; if it expects a valid path ornull/omitted value, this may cause confusing failures.Please confirm the server-side expectation and, if needed, either:
- derive
driverPathfrom available config, or- make
driverPathoptional in the contract and omit it when not applicable.workspaces/mi/mi-visualizer/src/views/Forms/ConnectionForm/ConnectionFormGenerator.tsx (2)
212-214: Good change: preserves falsy-but-valid values in XML generationSwitching the guard to
values[key] != null && values[key] !== undefinedmeansfalse,0, and empty strings will now be serialized instead of being dropped. This is a correctness improvement for boolean/numeric fields.
445-455: Passingparameters={params}intoFormGeneratormatches the new dynamic-field APIWiring the local
paramsstate intoFormGeneratorvia theparametersprop is consistent with the extended form API and the edit-flow parameter population above. As long asparamsfollows the expected shape for dynamic fields, this looks correct.workspaces/mi/mi-diagram/src/components/Form/RadioButtonGroup/GenericRadioGroup.tsx (1)
24-59: Generic radio group wrapper looks solidThe component cleanly wraps
RadioButtonGroupwith label, required marker, and optional help tooltip. Props are minimal and focused, andorientationhas a sensible default. No issues from my side.workspaces/mi/mi-core/src/rpc-types/mi-diagram/rpc-type.ts (1)
269-276: New RPC endpoints are consistent with the shared type contractsThe added RequestTypes for:
loadDriverAndTestConnectiongetDynamicFieldsgetStoredProceduresdownloadDriverForConnectorgetDriverMavenCoordinatesall correctly use the corresponding request/response interfaces from
types.ts, and the method names follow the existing${_preFix}/...convention. Looks good.Also applies to: 464-468
workspaces/mi/mi-core/src/rpc-types/mi-diagram/index.ts (1)
267-275: API surface for new driver/dynamic-field methods looks correct; ensure implementations are updatedThe
MiDiagramAPImethods:
loadDriverAndTestConnection(LoadDriverAndTestConnectionRequest): Promise<TestDbConnectionResponse>getDynamicFields(GetDynamicFieldsRequest): Promise<GetDynamicFieldsResponse>getStoredProcedures(DSSFetchTablesRequest): Promise<GetStoredProceduresResponse>downloadDriverForConnector(DriverDownloadRequest): Promise<DriverDownloadResponse>getDriverMavenCoordinates(DriverMavenCoordinatesRequest): Promise<DriverMavenCoordinatesResponse>are consistent with the request/response types and the RequestTypes declared in
rpc-type.ts.Please double-check that
ExtendedLanguageClient, the RPC manager, and RPC handler layers all implement and register these methods; otherwise consumers ofMiDiagramAPIwill see runtime errors despite the typings compiling.Also applies to: 455-459
workspaces/mi/mi-diagram/src/components/sidePanel/connectors/AddConnector.tsx (1)
213-243: DB connector value filtering and dynamic-form wiring look correctThe new
valuesForSynapseConfighelper and its use inonClickfor the"db"connector nicely ensure that transient dynamic fields (dyn_param*andpreparedStmt) are not serialized back into the Synapse config, while still passing the fullvaluesobject toupdateMediatorfor other connectors.Passing:
parameters={params},setComboValues,comboValuesMap,connections,connectionName,- and
ignoreFields/disableFieldsinto
FormGeneratorlines up with the dynamic-field and driver configuration work in the rest of the PR. I don’t see functional issues here.Also applies to: 259-267, 383-400
workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (2)
1923-1929: LGTM! Driver path addition supports new functionality.The addition of the
driverPathfield toDSSFetchTablesRequestproperly extends the interface to support driver-specific table fetching operations.
2268-2316: LGTM! New interface definitions are well-structured.The new interfaces for dynamic fields, stored procedures, and driver operations are properly defined with appropriate types and follow established naming conventions. The request/response pairs are consistent and the field selections make sense for their intended operations.
workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-handler.ts (1)
321-333: LGTM! RPC handler registrations follow established patterns.The new RPC handlers are properly registered using the messenger pattern consistent with existing handlers. The imports match the newly defined types and the handler wiring correctly delegates to the RpcManager methods.
Also applies to: 521-525
workspaces/mi/mi-rpc-client/src/rpc-clients/mi-diagram/rpc-client.ts (1)
452-465: LGTM! RPC client methods properly implement the messenger pattern.The new client methods are correctly typed with appropriate request/response pairs and consistently use the
sendRequestpattern withHOST_EXTENSION. The implementations align with the established codebase patterns.Also applies to: 1201-1218
workspaces/mi/mi-extension/src/lang-client/ExtendedLanguageClient.ts (1)
82-90: LGTM! Language client methods properly delegate to synapse endpoints.The new language client methods are correctly typed and follow the established pattern of delegating to
synapse/*endpoints viasendRequest. The implementations are consistent with existing methods in the class.Also applies to: 483-501
workspaces/mi/mi-diagram/src/components/Form/DBConnector/DriverConfiguration.tsx
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (1)
662-676: Unbounded retry loop persists - previously flagged.This unbounded
whileloop was flagged in the previous review. IfdownloadDriverForConnectorcontinues returning a falsy value, this will hang the UI indefinitely.The suggested fix from the previous review remains valid:
- let isDriverDownloaded = false; - if (!driverPath) { - while (!isDriverDownloaded) { - const args = { - groupId: groupId, - artifactId: artifactId, - version: version, - }; - this.setCustomError(getNameForController(FIELD_NAMES.CONFIG_KEY), "Checking DB Driver..."); - driverPath = await this.rpcClient.getMiDiagramRpcClient().downloadDriverForConnector(args); - if (driverPath) { - isDriverDownloaded = true; - } - } - } + if (!driverPath) { + const args = { groupId, artifactId, version }; + this.setCustomError( + getNameForController(FIELD_NAMES.CONFIG_KEY), + "Checking DB Driver..." + ); + driverPath = await this.rpcClient + .getMiDiagramRpcClient() + .downloadDriverForConnector(args); + + if (!driverPath) { + this.setCustomError( + getNameForController(FIELD_NAMES.CONFIG_KEY), + "Failed to download DB driver. Please check Maven coordinates or network connectivity." + ); + return undefined; + } + }
🧹 Nitpick comments (4)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (4)
19-19: Remove unused import.The
configimport from'process'is not used anywhere in this file.Apply this diff:
-import { config } from 'process'; import { Element, getNameForController, DynamicField, DynamicFieldGroup } from '../FormGenerator';
456-481: ReplaceforEachwithfor...ofto properly await async operations.Using
forEachwith an async callback doesn't wait for the promises to complete, which can lead to race conditions and unhandled promise rejections.Apply this diff:
- targetCombos.forEach(async (comboItem: string) => { + for (const comboItem of targetCombos) { this.setComboValues(comboItem, comboOptions); // if combooptions is empty trigger the onDynamicFieldChange if (comboOptions.length === 0) { // dummy empty element to trigger the onDynamicFieldChange const dummyElement = { name: comboItem, value: { name: comboItem, displayName: comboItem, value: "", inputType: 'stringOrExpression', hidden: false, onValueChange: element.onValueChange?.params?.[0]?.onDynamicFieldChange, parentField: fieldName, }, }; const tempElem = this.findElementByName(this.formData.elements, FIELD_NAMES.COLUMNS); if (tempElem) { await this.onDynamicFieldChange(dummyElement.value, dummyElement, fieldName); } } - }); + }
625-634: Remove commented-out code.Lines 625-634 contain commented-out code that should be removed if no longer needed. Keeping commented code reduces readability and can cause confusion.
Apply this diff:
this.setValue(FIELD_NAMES.ASSISTANCE_MODE, true); } - // if (this.getValues(FIELD_NAMES.ASSISTANCE_MODE) === false) { - // console.log("Assistance mode disabled, skipping connection validation."); - // this.setCustomError(getNameForController(FIELD_NAMES.CONFIG_KEY), null); - // this.setValue(FIELD_NAMES.ASSISTANCE_MODE, true); - // //return undefined; - // } - // if (this.getValues(FIELD_NAMES.QUERY_TYPE) && this.getValues(FIELD_NAMES.QUERY_TYPE) === UI_MODES.OFFLINE) { - // this.setCustomError(getNameForController(FIELD_NAMES.CONFIG_KEY), null); - // return undefined; - // } // 2. Check Required Parameters exist
1016-1019: Remove or reduce debug console.log statements.Multiple
console.logstatements appear throughout the code (e.g., lines 534, 604, 613, 818, 1016-1019, 1649). Consider removing them or replacing with a proper logging framework for production code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workspaces/mi/mi-diagram/src/components/Form/DBConnector/DriverConfiguration.tsx(1 hunks)workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workspaces/mi/mi-diagram/src/components/Form/DBConnector/DriverConfiguration.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (2)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (4)
DynamicFieldGroup(174-177)Element(119-153)getNameForController(179-184)DynamicField(160-172)workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (1)
DynamicField(2255-2266)
🪛 Biome (2.1.2)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
[error] 215-215: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1041-1041: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1108-1108: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1109-1109: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1110-1110: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1132-1132: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1151-1155: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1157-1157: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1272-1272: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1277-1277: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1282-1282: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1287-1287: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (1)
777-792: Good fix for theresponseColumnsguard!The guard condition now properly checks that
operationType === QUERY_TYPES.SELECTand thatformValues[FIELD_NAMES.RESPONSE_COLUMNS]is a non-empty string before calling.split(). This addresses the issue flagged in the previous review.
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (5)
199-229: Wrap switch case declarations in blocks.Variable declarations in switch cases should be wrapped in blocks to prevent them from being accessible in other cases. This issue was previously identified.
Based on learnings, static analysis hints from Biome should be addressed.
648-662: Avoid unbounded retry loop when downloading DB driver.This unbounded
while (!isDriverDownloaded)loop was previously identified as a critical issue. IfdownloadDriverForConnectorkeeps returning a falsy value, the loop will never terminate and will hang the UI.
1019-1147: Wrap switch case declarations in blocks.Variable declarations in switch cases (INSERT, DELETE, SELECT, CALL) should be wrapped in blocks to prevent them from being accessible in other cases. This issue was previously identified.
Based on learnings, static analysis hints from Biome should be addressed.
1243-1271: Wrap switch case declarations in blocks.Variable declarations in switch cases (SELECT, DELETE, INSERT, CALL parsing) should be wrapped in blocks. This issue was previously identified.
Based on learnings, static analysis hints from Biome should be addressed.
1625-1625: Use strict equality operator.Line 1625 uses loose equality (
==) which can lead to unexpected type coercion. This was previously identified.Apply this diff:
- if (value == true) { + if (value === true) {
🧹 Nitpick comments (3)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (3)
19-19: Remove unused import.The
configimport from'process'is not used anywhere in the file.Apply this diff:
-import { config } from 'process'; import { Element, getNameForController, DynamicField, DynamicFieldGroup } from '../FormGenerator';
243-246: Remove commented-out code.Commented-out code should be removed to improve maintainability. If this logic might be needed in the future, consider documenting the reasoning in a comment or tracking it in an issue.
Apply this diff:
try { - // //if offline mode do not fetch dynamic fields - // if(element.inputType === 'string' && element.name === FIELD_NAMES.TABLE_NAME) { - // return undefined; - // } const connectionInfo = await this._getValidConnectionDetails();
1540-1543: Remove commented-out code.The commented code for SELECT * handling should either be implemented or removed to improve code clarity.
Apply this diff:
if (!columnsStr) return { success: false, fields: {}, errorMessage: "No columns specified for SELECT." }; - // Handle SELECT * case - // if (columnsStr.trim() === '*') { - // return { success: true, fields: availableFields }; - // } const columns = columnsStr.split(',').map(col => col.trim());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (1)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (4)
DynamicFieldGroup(174-177)Element(119-153)getNameForController(179-184)DynamicField(160-172)
🪛 Biome (2.1.2)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
[error] 215-215: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1021-1021: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1088-1088: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1089-1089: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1090-1090: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1112-1112: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1131-1135: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1137-1137: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1252-1252: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1257-1257: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1262-1262: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1267-1267: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive VSCode extension support for RDBMS connectors, enabling form-driven SQL query building with both online (database-connected) and offline modes. The implementation includes dynamic field management, driver configuration UI with three options (Default, Custom, Maven), and several new RPC endpoints for database operations.
Key Changes:
- Introduction of dynamic field handling system that fetches table schemas and generates form fields based on database connections
- Driver configuration management with support for default drivers, custom local drivers, and Maven-based dependency resolution
- New RPC endpoints for fetching dynamic fields, stored procedures, downloading drivers, and validating database connections
- Enhanced form generator with support for radio button groups, combo value propagation, and custom error handling
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 28 comments.
Show a summary per file
| File | Description |
|---|---|
| GenerateResourcesForm.tsx | Added driverPath parameter initialization for database connections |
| ConnectionFormGenerator.tsx | Enhanced to populate driver parameters and handle parameter generation for connections |
| rpc-client.ts | Added client methods for dynamic fields, stored procedures, and driver operations |
| rpc-manager.ts | Implemented RPC manager methods for new database and driver-related operations |
| rpc-handler.ts | Registered new RPC handlers for driver and dynamic field operations |
| ExtendedLanguageClient.ts | Extended language client with new request methods for database operations |
| index.tsx | Minor export comment (potentially incomplete) |
| AddConnector.tsx | Enhanced connector form with combo values management and parameter generation |
| GenericRadioGroup.tsx | New reusable radio button group component with tooltip support |
| FormGenerator.tsx | Major updates including driver configuration UI, dynamic field handling, and value change propagation |
| DynamicFieldsHandler.tsx | New comprehensive handler for dynamic fields, query building/parsing, and connection management (1634 lines) |
| DriverConfiguration.tsx | New component providing three driver configuration options with validation |
| types.ts | Added type definitions for dynamic fields, driver operations, and stored procedures |
| rpc-type.ts | Defined RPC request types for new operations |
| index.ts (mi-core) | Extended API interface with new method signatures |
Comments suppressed due to low confidence (2)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx:19
- Unused import config.
import { config } from 'process';
workspaces/mi/mi-diagram/src/components/sidePanel/connectors/AddConnector.tsx:88
- Unused variable setComboValues.
const setComboValues = (elementName: string, newValues: string[]) => {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
workspaces/mi/mi-diagram/src/components/Form/RadioButtonGroup/GenericRadioGroup.tsx
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Outdated
Show resolved
Hide resolved
| config: OnValueChangeParams, | ||
| element: Element | ||
| ): Promise<void> { | ||
| let parseSuccess = true; |
There was a problem hiding this comment.
The initial value of parseSuccess is unused, since it is always overwritten.
| const match = currentRegex?.exec(userQuery); | ||
|
|
||
| if (!match?.groups) { | ||
| parseSuccess = false; |
There was a problem hiding this comment.
The value assigned to parseSuccess here is unused.
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Outdated
Show resolved
Hide resolved
…amicFieldsHandler.tsx Co-authored-by: Copilot <[email protected]>
…amicFieldsHandler.tsx Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (2)
242-245: Remove commented-out code.Lines 242-245 contain commented-out logic that appears to be old implementation. Dead code should be removed to improve maintainability.
Apply this diff:
): Promise<DynamicField[] | null> => { try { - // //if offline mode do not fetch dynamic fields - // if(element.inputType === 'string' && element.name === FIELD_NAMES.TABLE_NAME) { - // return undefined; - // } const connectionInfo = await this._getValidConnectionDetails();
1389-1390: Fix typo in comment.Line 1389 contains
"complexicity"which should be"complexity".Apply this diff:
- // conditions within parenthesis are considered out of scope since it introduces unwanted complexicity + // conditions within parenthesis are considered out of scope since it introduces unwanted complexity const conditions = whereClause.trim().split(/\s+AND\s+/i);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/**/*.tsx : Use React 18.2.0 features including concurrent rendering and automatic batching; avoid class components in favor of functional components with hooks
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-24T14:51:49.267Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 998
File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132
Timestamp: 2025-11-24T14:51:49.267Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
🧬 Code graph analysis (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (2)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (4)
DynamicFieldGroup(174-177)Element(119-153)getNameForController(179-184)DynamicField(160-172)workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (1)
DynamicField(2255-2266)
🪛 Biome (2.1.2)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
[error] 214-214: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1020-1020: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1087-1087: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1088-1088: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1089-1089: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1111-1111: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1130-1134: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1136-1136: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1251-1251: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1256-1256: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1261-1261: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1266-1266: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (1)
19-19: Unable to verify the import statement due to repository access failure.The verification process requires access to the repository files to confirm:
- Whether
DynamicFieldGroupis exported fromFormGenerator- Whether
DynamicFieldGroupis defined locally inDynamicFieldsHandlerat lines 173-176- The exact nature of the import on line 19
Without access to the codebase, the claim about a potential circular dependency or incorrect import path cannot be definitively validated or refuted.
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (5)
242-245: Remove commented-out code.Lines 242-245 contain commented-out code that should be removed to improve readability. If this logic might be needed in the future, document the decision in a comment or track it in an issue.
try { - // //if offline mode do not fetch dynamic fields - // if(element.inputType === 'string' && element.name === FIELD_NAMES.TABLE_NAME) { - // return undefined; - // } const connectionInfo = await this._getValidConnectionDetails();
330-335: Consider more robust error identification.The error check on line 332 uses string matching (
error.message.startsWith('Query parsing error:')) to avoid double banners. This is brittle and error-prone if the error message format changes. Consider using custom error types or error codes for better reliability.// Define a custom error class class QueryParsingError extends Error { constructor(message: string) { super(message); this.name = 'QueryParsingError'; } } // Then in catch block: if (!(error instanceof QueryParsingError)) { this.setCustomError(getNameForController(element.name), "Error processing field change."); }
609-618: Simplify complex nested conditionals.The nested conditionals in lines 609-618 are difficult to follow. The logic checks
findElementByName, accessescurrentValue, compares withOFFLINE, and setsASSISTANCE_MODE. Consider extracting this logic into a helper method with clear naming to improve readability.private _shouldEnableAssistanceMode(): boolean { const queryTypeElement = this.findElementByName(this.formData.elements, FIELD_NAMES.QUERY_TYPE); if (!queryTypeElement?.currentValue) { return true; } return !(queryTypeElement.currentValue === UI_MODES.OFFLINE && this.getValues(FIELD_NAMES.QUERY_TYPE) === UI_MODES.OFFLINE); } // Then in _getValidConnectionDetails: const enableAssistance = this._shouldEnableAssistanceMode(); this.setValue(FIELD_NAMES.ASSISTANCE_MODE, enableAssistance); if (!enableAssistance) { return undefined; }
744-758: Direct mutation of column object may cause side effects.Line 746 directly mutates the
columnobject (column.columnName = ...) which is fromparamManagerValues. This could cause unintended side effects if the original data is used elsewhere. Consider creating a new object or documenting that this mutation is intentional.if (Array.isArray(paramManagerValues)) { let count = 0; for (const column of paramManagerValues) { - if (operationType === QUERY_TYPES.CALL) { column.columnName = `param_${++count}`; } + const columnName = operationType === QUERY_TYPES.CALL + ? `param_${++count}` + : column.columnName; - if (!column.columnName) continue; // Skip if columnName is missing + if (!columnName) continue; // Skip if columnName is missing - const normalizedName = column.columnName.replace(/[^a-zA-Z0-9_]/g, '_'); + const normalizedName = columnName.replace(/[^a-zA-Z0-9_]/g, '_'); collectedValues[normalizedName] = { value: column.columnValue?.value, isExpression: column.columnValue?.isExpression, columnType: column.propertyType?.value, name: normalizedName, - displayName: column.columnName, + displayName: columnName, helpTip: '', }; } }
1623-1632: Add space after comma for consistent formatting.Line 1626 is missing a space after the comma in
this.onConnectionChange(fieldName,rpc). While minor, consistent formatting improves code readability.if (value === true) { this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.ONLINE); - this.onConnectionChange(fieldName,rpc) + this.onConnectionChange(fieldName, rpc) } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/**/*.tsx : Use React 18.2.0 features including concurrent rendering and automatic batching; avoid class components in favor of functional components with hooks
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-24T14:51:49.267Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 998
File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132
Timestamp: 2025-11-24T14:51:49.267Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
🧬 Code graph analysis (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (3)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (4)
DynamicFieldGroup(174-177)Element(119-153)getNameForController(179-184)DynamicField(160-172)workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (1)
DynamicField(2255-2266)workspaces/mi/mi-data-mapper-utils/src/dm-utils.ts (1)
match(231-233)
🪛 Biome (2.1.2)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
[error] 214-214: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1020-1020: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1087-1087: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1088-1088: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1089-1089: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1111-1111: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1130-1134: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1136-1136: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1251-1251: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1256-1256: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1261-1261: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1266-1266: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Show resolved
Hide resolved
…amicFieldsHandler.tsx Co-authored-by: Copilot <[email protected]>
…nectionFormGenerator.tsx Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…nectionFormGenerator.tsx Co-authored-by: Copilot <[email protected]>
…ddConnector.tsx Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workspaces/mi/mi-visualizer/src/views/Forms/ConnectionForm/ConnectionFormGenerator.tsx (1)
110-159: UseconnectionFound.connectionTypedirectly when resetting form on edit.Inside the edit flow, you call
setConnectionType(connectionFound.connectionType)and then immediatelyreset({ ..., connectionType: connectionType }). BecausesetConnectionTypeis async,connectionTypehere may still hold the old value, so the form can be reset with stale connection type.Prefer the concrete value from
connectionFound:setConnectionType(connectionFound.connectionType); setConnectionName(props.connectionName); setFormData(connectionSchema); - const parameters = connectionFound.parameters + const parameters = connectionFound.parameters; @@ - reset({ - name: props.connectionName, - connectionType: connectionType - }); + reset({ + name: props.connectionName, + connectionType: connectionFound.connectionType + });
♻️ Duplicate comments (10)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (6)
355-399: Fix unreachableupdatedOffline && updatedOnlinecondition insetElementVisibility.
updatedOfflineandupdatedOnlineare set in mutually exclusive branches based onenableCondition[0].queryType, soif (updatedOffline && updatedOnline)can never be true. Use||so the method returnstrueonce either offline or online element is updated.- if (updatedOffline && updatedOnline) { + if (updatedOffline || updatedOnline) { return true; }This also aligns the return value with the “found and updated” semantics used elsewhere in the method.
200-207: GuardqueryBuildConfigbefore using it inhandleDynamicContent.
queryBuildConfigis derived via a long optional-chaining chain, but thenqueryBuildConfig.queryTypeis accessed unconditionally. If any link in the chain is missing, this will throw at runtime when a user selects a table.- if (element.inputType === 'string' && element.name === FIELD_NAMES.TABLE_NAME) { - const parentElement = this.findElementByName(this.formData.elements, FIELD_NAMES.TABLE_NAME); - const queryBuildConfig = parentElement?.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]; - await this._buildQueryFromDynamicFields(FIELD_NAMES.TABLE_NAME, queryBuildConfig.queryType , queryBuildConfig, element); - } else { + if (element.inputType === 'string' && element.name === FIELD_NAMES.TABLE_NAME) { + const parentElement = this.findElementByName(this.formData.elements, FIELD_NAMES.TABLE_NAME); + const queryBuildConfig = + parentElement?.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]; + if (queryBuildConfig?.queryType) { + await this._buildQueryFromDynamicFields( + FIELD_NAMES.TABLE_NAME, + queryBuildConfig.queryType, + queryBuildConfig, + element + ); + } + } else { await this._handleDynamicContentChange(value, fieldName, element); }
632-695: Avoid unbounded retry loop when downloading DB driver.The
while (!isDriverDownloaded)loop arounddownloadDriverForConnectorcan spin forever if the RPC continues to fail (bad coordinates, network, permission), hanging the UI and hammering the backend.Replace it with a single attempt (or a bounded retry with backoff) and surface a clear error:
- let isDriverDownloaded = false; - if (!driverPath) { - while (!isDriverDownloaded) { - const args = { - groupId: groupId, - artifactId: artifactId, - version: version, - }; - this.setCustomError(getNameForController(FIELD_NAMES.CONFIG_KEY), "Checking DB Driver..."); - driverPath = await this.rpcClient.getMiDiagramRpcClient().downloadDriverForConnector(args); - if (driverPath) { - isDriverDownloaded = true; - } - } - } + if (!driverPath) { + const args = { groupId, artifactId, version }; + this.setCustomError( + getNameForController(FIELD_NAMES.CONFIG_KEY), + "Checking DB Driver..." + ); + driverPath = await this.rpcClient + .getMiDiagramRpcClient() + .downloadDriverForConnector(args); + + if (!driverPath) { + this.setCustomError( + getNameForController(FIELD_NAMES.CONFIG_KEY), + "Failed to download DB driver. Please check Maven coordinates or network connectivity." + ); + return undefined; + } + }
873-890: Remove hard‑coded'table'key in_setFieldValuedynamic field lookup.
this.dynamicFields['table']assumes all dynamic fields live under the'table'parent, which will break for other parents (e.g., different field names or multiple dynamic sections). This also couples the helper to a single parent key.Consider either:
- Passing the parent field key into
_setFieldValue, or- Searching across all dynamic field groups:
- const dynamicField = this.dynamicFields['table']?.fields.find((f: DynamicField) => f.value.name === fieldName); + const dynamicField = Object.values(this.dynamicFields) + .flatMap(group => group.fields) + .find((f: DynamicField) => f.value.name === fieldName);This keeps
expectsObjectdetection working regardless of which parent field owns the dynamic field.
214-221: Wrapcase 'buildQuery'(and other case declarations) in blocks to satisfynoSwitchDeclarations.
const parentField = ...is declared directly under thecase 'buildQuery':label. Biome flags this asnoSwitchDeclarations; the variable is technically visible to other cases and can cause subtle bugs.Wrap the case body in a block and keep the
breakinside it:- case 'buildQuery': - const parentField = (element as any).parentField; - if (parentField) { - await this.onDynamicFieldChange(value, element, parentField); - } else { - console.warn(`'buildQuery' called without parentField for element:`, element.name); - } - break; + case 'buildQuery': { + const parentField = (element as any).parentField; + if (parentField) { + await this.onDynamicFieldChange(value, element, parentField); + } else { + console.warn(`'buildQuery' called without parentField for element:`, element.name); + } + break; + }Apply the same pattern to:
case QUERY_TYPES.INSERT(around line 1019),case QUERY_TYPES.SELECT(orderBy/limit/offset locals),case QUERY_TYPES.CALL(callTemplate, callParams, prepParams), and- The switch in
_handleManualQueryChange(SELECT/INSERT/DELETE/CALL cases).
1480-1535: Guard against undefinedvaluesStrin_parseCallParams.
valuesStris typed asstring | undefined, butvaluesStr.split(',')is called unconditionally. If the regex ever matches without a values group (or_parseCallParamsis reused elsewhere), this will throw.private _parseCallParams(valuesStr: string | undefined, availableFields: Record<string, DynamicFieldValue>, connectionInfo: boolean): { success: boolean, fields: Record<string, DynamicFieldValue>, errorMessage?: string } { const matchedFields: Record<string, DynamicFieldValue> = {}; - const values = valuesStr.split(',').map(val => val.trim().replace(/^['"]|['"]$/g, '')); + if (!valuesStr) { + return { + success: false, + fields: {}, + errorMessage: "No parameters provided for CALL statement." + }; + } + const values = valuesStr.split(',').map(val => val.trim().replace(/^['"]|['"]$/g, ''));workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (4)
894-923: Hooks used insideParamManagerComponentinvoked as a plain function violate Rules of Hooks.
ParamManagerComponentcallsuseEffectbut is invoked asParamManagerComponent(...)inside the'ParamManager'case, not as<ParamManagerComponent />. This means React can’t track its hooks correctly, and Biome correctly flagsuseHookAtTopLevel.Refactor
ParamManagerComponentinto a proper child component and render it via JSX:- function ParamManagerComponent(element: Element, isRequired: boolean, helpTipElement: JSX.Element, field: any) { - useEffect(() => { - try { - handleValueChange(field.value, element.name.toString(), element); - } catch { - console.error("Error initializing ParamManagerComponent"); - } - }, []); // run only on mount - return <ComponentCard id={'parameterManager-' + element.name} sx={cardStyle} disbaleHoverEffect> + const ParamManagerComponent: React.FC<{ + element: Element; + isRequired: boolean; + helpTipElement: JSX.Element; + field: any; + }> = ({ element, isRequired, helpTipElement, field }) => { + useEffect(() => { + try { + handleValueChange(field.value, element.name.toString(), element); + } catch { + console.error("Error initializing ParamManagerComponent"); + } + }, []); + return <ComponentCard id={'parameterManager-' + element.name} sx={cardStyle} disbaleHoverEffect> ... - </ComponentCard>; - } + </ComponentCard>; + }; @@ - case 'ParamManager': { - return ( - ParamManagerComponent(element, isRequired, helpTipElement, field) - ); - } + case 'ParamManager': { + return ( + <ParamManagerComponent + element={element} + isRequired={isRequired} + helpTipElement={helpTipElement} + field={field} + /> + ); + }Apply the same pattern to
ExpressionFieldComponentandFormExpressionFieldComponent(render them as<ExpressionFieldComponent ... />/<FormExpressionFieldComponent ... />instead of calling them directly).
1191-1265: MoveuseEffectand declarations out of the'combo*'switch case to satisfy Rules of Hooks andnoSwitchDeclarations.Within the
'booleanOrExpression'/'comboOrExpression'/'combo'case you:
- Declare
const itemsandconst allowItemCreate, and- Call
useEffect(...)inside the case body.This triggers both
useHookAtTopLevelandnoSwitchDeclarationserrors: hooks must be called unconditionally from component/custom-hook top level, and case-local declarations should be wrapped.Extract this branch into a small child component that owns the effect and items calculation:
- case 'booleanOrExpression': - case 'comboOrExpression': - case 'combo': - const items = element.inputType === 'booleanOrExpression' ? ["true", "false"] : (props.comboValuesMap?.[name] || element.comboValues); - const allowItemCreate = element.inputType === 'comboOrExpression'; - useEffect(() => { - handleValueChange(field.value, name, element); - }, [props.comboValuesMap?.[name]]); - return ( - <> - <AutoComplete ... /> - {dynamicFields[name]?.fields?.length > 0 && (/* dynamic field rendering */)} - </> - ); + case 'booleanOrExpression': + case 'comboOrExpression': + case 'combo': { + return ( + <ComboWithDynamicFields + element={element} + name={name} + field={field} + helpTipElement={helpTipElement} + isRequired={isRequired} + errorMsg={errorMsg} + comboValuesMap={props.comboValuesMap} + dynamicFields={dynamicFields} + onValueChange={handleValueChange} + /> + ); + }Where
ComboWithDynamicFieldsis a new component declared at top level that:
- Computes
itemsandallowItemCreate,- Calls its own
useEffectto notifyonValueChangeon mount/whencomboValuesMap[name]changes, and- Renders the
AutoComplete+ dynamic field group.This removes hooks from the switch, fixes the declaration-scope warning, and keeps behavior localized.
1469-1525: Also move theuseEffectin the'connection'case into a child component.The
'connection'switch case callsuseEffectdirectly, which again violates Rules of Hooks and is flagged by Biome.As with the combo case, extract this into a dedicated
ConnectionFieldcomponent:- case 'connection': - useEffect(() => { - handleValueChange(field.value, name, element); - }, []); - ... - return ( - <> - {/* label + Add new connection */} - <AutoComplete ... /> - {generatedFormDetails && ...} - </> - ); + case 'connection': { + return ( + <ConnectionField + element={element} + name={name} + field={field} + helpTipElement={helpTipElement} + isDisabled={isDisabled} + errorMsg={errorMsg} + connectionNames={connectionNames} + getValues={getValues} + setValue={setValue} + handleValueChange={handleValueChange} + getConnectionNames={getConnectionNames} + rpcClient={rpcClient} + sidePanelContext={sidePanelContext} + generatedFormDetails={generatedFormDetails} + visibleDetails={visibleDetails} + numberOfDifferent={numberOfDifferent} + setVisibleDetails={setVisibleDetails} + setNumberOfDifferent={setNumberOfDifferent} + setIsClickedDropDown={setIsClickedDropDown} + setIsGenerating={setIsGenerating} + /> + ); + }
ConnectionFieldcan then own itsuseEffectand any local logic, preserving hook ordering inFormGenerator.
272-325: Driver config save still reads stale state; pass the next config explicitly intosaveDriverConfig.
handleDriverTypeChangecallssetDriverConfig(...)and thensaveDriverConfig(), butsaveDriverConfigreadsdriverConfigfrom state. BecausesetDriverConfigis async,driverConfiginsidesaveDriverConfigis still the previous value, so validation and persisted coordinates can be wrong (especially when reusing existing params).Refactor to compute a
nextConfigobject synchronously and pass it through:- const handleDriverTypeChange = async (driverType: string, element: Element) => { + const handleDriverTypeChange = async (driverType: string, element: Element) => { const option = DRIVER_OPTIONS.find(opt => opt.label === driverType); if (!option) return; - setDriverConfig(prev => ({ ...prev, type: option.configType })); + let nextConfig: DriverConfig = { ...driverConfig, type: option.configType }; + setDriverConfig(nextConfig); setDriverError(null); @@ - if (option.configType === 'custom') { + if (option.configType === 'custom') { if (existingGroupId && existingArtifactId && existingVersion && existingDriverPath) { - setDriverConfig(prev => ({ - ...prev, - groupId: existingGroupId, - artifactId: existingArtifactId, - version: existingVersion, - driverPath: existingDriverPath - })); + nextConfig = { + ...nextConfig, + groupId: existingGroupId, + artifactId: existingArtifactId, + version: existingVersion, + driverPath: existingDriverPath, + }; } } else { if (existingGroupId && existingArtifactId && existingVersion) { - setDriverConfig(prev => ({ - ...prev, - groupId: existingGroupId, - artifactId: existingArtifactId, - version: existingVersion, - driverPath: null - })); - } else if (existingDriverPath) { - setValue("driverPath", null); - } - } - saveDriverConfig(); + nextConfig = { + ...nextConfig, + groupId: existingGroupId, + artifactId: existingArtifactId, + version: existingVersion, + driverPath: null, + }; + } else if (existingDriverPath) { + setValue("driverPath", null); + } + } + setDriverConfig(nextConfig); + await saveDriverConfig(nextConfig); } }; @@ - const saveDriverConfig = async () => { + const saveDriverConfig = async (configOverride?: DriverConfig) => { + const cfg = configOverride ?? driverConfig; try { - if (driverConfig.type === 'maven' && - (!driverConfig.groupId || !driverConfig.artifactId || !driverConfig.version)) { + if (cfg.type === 'maven' && + (!cfg.groupId || !cfg.artifactId || !cfg.version)) { @@ - if (driverConfig.type === 'maven') { + if (cfg.type === 'maven') { const validDriver = await rpcClient.getMiDiagramRpcClient().downloadDriverForConnector({ - groupId: driverConfig.groupId, - artifactId: driverConfig.artifactId, - version: driverConfig.version + groupId: cfg.groupId, + artifactId: cfg.artifactId, + version: cfg.version }); @@ - setValue("groupId", driverConfig.groupId); - setValue("artifactId", driverConfig.artifactId); - setValue("version", driverConfig.version); - if (driverConfig.type === 'custom') { - setValue("driverPath", driverConfig.driverPath); + setValue("groupId", cfg.groupId); + setValue("artifactId", cfg.artifactId); + setValue("version", cfg.version); + if (cfg.type === 'custom') { + setValue("driverPath", cfg.driverPath); }Update any
onSave={saveDriverConfig}props to call the new signature, e.g.onSave={() => saveDriverConfig(driverConfig)}.
🧹 Nitpick comments (2)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (2)
418-438: DynamicFieldsHandler initialization is fine but should ideally react toparameters/dynamicFieldschanges.The handler is created once in a
useEffectwith an empty dependency array. That’s acceptable ifrpcClient,formData,parameters, etc. are effectively stable for the lifetime of the component, but if they can change between renders the handler will retain stale references.If
formData/parameters/connectionNameare expected to change for the same FormGenerator instance, consider including them in the dependency array and reconstructing the handler when they change.
1191-1217: EnsureuseEffectdependencies reflectnameandfield.valuewhen reusing the pattern.When you refactor the combo/connection cases into components, make sure any effects that depend on
nameorfield.valueinclude them in the dependency array so dynamic-field updates re-fire when the bound field changes:useEffect(() => { onValueChange(field.value, name, element); }, [field.value, name, element, comboValuesMap?.[name]]);This avoids stale closures while keeping hook usage valid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx(1 hunks)workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx(22 hunks)workspaces/mi/mi-diagram/src/components/sidePanel/connectors/AddConnector.tsx(5 hunks)workspaces/mi/mi-visualizer/src/views/Forms/ConnectionForm/ConnectionFormGenerator.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/Diagram.tsx : Implement input validation for CDModel structure: verify that all required properties (connections, listeners, services) are present and properly typed
Applied to files:
workspaces/mi/mi-diagram/src/components/sidePanel/connectors/AddConnector.tsxworkspaces/mi/mi-visualizer/src/views/Forms/ConnectionForm/ConnectionFormGenerator.tsxworkspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/utils/diagram.ts : Filter out autogenerated connections (symbol starting with '_') and connections with enableFlowModel=false before rendering
Applied to files:
workspaces/mi/mi-diagram/src/components/sidePanel/connectors/AddConnector.tsxworkspaces/mi/mi-visualizer/src/views/Forms/ConnectionForm/ConnectionFormGenerator.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/**/*.tsx : Use React 18.2.0 features including concurrent rendering and automatic batching; avoid class components in favor of functional components with hooks
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsxworkspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-24T14:51:49.267Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 998
File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132
Timestamp: 2025-11-24T14:51:49.267Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsxworkspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-24T22:16:28.380Z
Learnt from: dan-niles
Repo: wso2/vscode-extensions PR: 980
File: workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/RichTextTemplateEditor/RichTextTemplateEditor.tsx:384-412
Timestamp: 2025-11-24T22:16:28.380Z
Learning: In the RichTextTemplateEditor component (workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/RichTextTemplateEditor/RichTextTemplateEditor.tsx), token fetching on external `value` prop changes is intentionally disabled. Users cannot edit both the minimized and expanded editors simultaneously, so tokens only need to be generated based on user typing in the expanded editor view, not on external prop updates.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx
📚 Learning: 2025-11-27T07:59:33.522Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 897
File: workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx:50-59
Timestamp: 2025-11-27T07:59:33.522Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx, the `clause.properties.expression` property in the `fillDefaults` function does not require defensive null/undefined checks because it's a required property enforced by form validation in ClauseEditor.
Applied to files:
workspaces/mi/mi-visualizer/src/views/Forms/ConnectionForm/ConnectionFormGenerator.tsxworkspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/Diagram.tsx : Implement proper error boundaries around the Diagram component to gracefully handle rendering errors without crashing the parent application
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-26T06:34:09.752Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:128-134
Timestamp: 2025-11-26T06:34:09.752Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBreadcrumb() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-26T07:49:56.428Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:136-141
Timestamp: 2025-11-26T07:49:56.428Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBackButton() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
🧬 Code graph analysis (2)
workspaces/mi/mi-diagram/src/components/sidePanel/connectors/AddConnector.tsx (3)
workspaces/ballerina/ballerina-side-panel/src/components/ParamManager/ParamManager.tsx (1)
ParamConfig(46-50)workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (1)
getNameForController(179-184)workspaces/common-libs/ui-toolkit/src/components/ExpressionEditor/utils/token.ts (1)
setValue(103-145)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (4)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (1)
DynamicFieldsHandler(162-1633)workspaces/mi/mi-diagram/src/components/Form/DBConnector/DriverConfiguration.tsx (5)
DriverConfig(24-30)DRIVER_OPTIONS(50-69)DefaultDriverConfig(83-127)CustomDriverConfig(129-212)MavenDriverConfig(214-276)workspaces/common-libs/ui-toolkit/src/components/ExpressionEditor/utils/token.ts (1)
setValue(103-145)workspaces/mi/mi-diagram/src/components/Form/RadioButtonGroup/GenericRadioGroup.tsx (1)
GenericRadioGroup(24-60)
🪛 Biome (2.1.2)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx
[error] 895-896: This hook is being called from a nested function, but all hooks must be called unconditionally from the top-level component.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 928-929: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 983-984: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 1212-1213: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1213-1214: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1214-1215: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 1469-1470: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
[error] 214-214: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1020-1020: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1087-1087: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1088-1088: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1089-1089: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1111-1111: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1130-1134: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1136-1136: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1251-1251: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1256-1256: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1261-1261: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1266-1266: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (5)
workspaces/mi/mi-visualizer/src/views/Forms/ConnectionForm/ConnectionFormGenerator.tsx (2)
212-263: Value filtering inonAddConnectionlooks correct and handles non-string values.The updated guard
values[key] != nullensures0/falseare still serialized while skipping onlynull/undefined, and the exclusion ofconfigRef,connectionType, andconnectionNamematches the expected XML payload.No changes needed here.
441-455: Passingparameters={params}intoFormGeneratorintegrates driver params cleanly.Forwarding the derived
paramsstate intoFormGeneratorvia the newparametersprop aligns with the dynamic driver configuration logic introduced there and keeps the connection form’s driver-related params in sync.workspaces/mi/mi-diagram/src/components/sidePanel/connectors/AddConnector.tsx (2)
222-261: Filtering out dynamic param and prepared-statement fields beforeupdateMediatorfor DB connectors makes sense.
valuesForSynapseConfig’s exclusion of keys starting withdyn_paramandpreparedStmtwhenconnectorName === "db"prevents internal dynamic-field and prepared-statement wiring from leaking into the Synapse config, while preserving all other values.This is a reasonable specialization for the DB connector path, and the helper keeps the core
onClicklogic tidy.
375-395: The additional FormGenerator props (parameters, combo values, connection info) are wired consistently.Passing
parameters={params},setComboValues,comboValuesMap,connections,connectionName, andignoreFieldsintoFormGeneratormatches the extended FormGenerator API and enables dynamic fields and connection-aware behavior in the side panel.workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (1)
1618-1660:radiohandling for driver selection is structurally sound.The special case for
driverSelectOptioncorrectly:
- Uses
GenericRadioGroupto render options fromcomboValues,- Updates the form value via
field.onChange, and- Delegates driver-type behavior to
handleDriverTypeChange,- Then renders the appropriate driver configuration via
renderDriverConfiguration.Generic radio fields fall back to a simpler
GenericRadioGroupwith optionalonValueChangeintegration. This split is clear and keeps driver-specific logic isolated.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (4)
723-725: Correct typo inexpressionTypesarray entry.
'stringOrExpresion'is misspelled and duplicates the correctly spelled'stringOrExpression'above. This can cause subtle mismatches in inputType checks.If no backward‑compat dependency on the typo, simplify the array:
- const expressionTypes = ['stringOrExpression', 'integerOrExpression', 'expression', 'keyOrExpression', 'resourceOrExpression', - 'textOrExpression', 'textAreaOrExpression', 'stringOrExpresion' - ]; + const expressionTypes = [ + 'stringOrExpression', + 'integerOrExpression', + 'expression', + 'keyOrExpression', + 'resourceOrExpression', + 'textOrExpression', + 'textAreaOrExpression', + ];
895-919: Fix React hook rule violations in nested helper components.
ParamManagerComponent,ExpressionFieldComponent, andFormExpressionFieldComponentall calluseEffectbut are invoked as plain functions fromrenderFormElement(not rendered as<Component />), which breaks the Rules of Hooks and is flagged by Biome.Refactor these to real React components and render them via JSX, e.g.:
- function ParamManagerComponent(element: Element, isRequired: boolean, helpTipElement: JSX.Element, field: any) { - useEffect(() => { - handleValueChange(field.value, element.name.toString(), element); - }, []); // run only on mount - return <ComponentCard id={'parameterManager-' + element.name} sx={cardStyle} disbaleHoverEffect> + const ParamManagerComponent: React.FC<{ + element: Element; + isRequired: boolean; + helpTipElement: JSX.Element; + field: any; + }> = ({ element, isRequired, helpTipElement, field }) => { + useEffect(() => { + handleValueChange(field.value, element.name.toString(), element); + }, []); // run only on mount + return <ComponentCard id={'parameterManager-' + element.name} sx={cardStyle} disbaleHoverEffect> ... - </ComponentCard>; - } + </ComponentCard>; + }; @@ - case 'ParamManager': { - return ( - ParamManagerComponent(element, isRequired, helpTipElement, field) - ); - } + case 'ParamManager': { + return ( + <ParamManagerComponent + element={element} + isRequired={isRequired} + helpTipElement={helpTipElement} + field={field} + /> + ); + }Apply the same pattern to
ExpressionFieldComponentandFormExpressionFieldComponent(define asReact.FCand use<ExpressionFieldComponent ... />/<FormExpressionFieldComponent ... />), so all hooks are called from proper component top‑level.Also applies to: 921-977, 971-977
484-528: Fix staledriverConfigusage when saving driver settings.
handleDriverTypeChangemutatesdriverConfigviasetDriverConfigand then immediately callssaveDriverConfig, butsaveDriverConfigreads from the previous state. This can cause Maven validation and persisted values to be out of sync with what the UI shows (especially when loading existing coordinates fromparameters).Refactor to compute and use a single “nextConfig” object:
- const handleDriverTypeChange = async (driverType: string, element: Element) => { + const handleDriverTypeChange = async (driverType: string, element: Element) => { const option = DRIVER_OPTIONS.find(opt => opt.label === driverType); if (!option) return; - - setDriverConfig(prev => ({ ...prev, type: option.configType })); - setDriverError(null); + setDriverError(null); + + let nextConfig: DriverConfig = { + ...driverConfig, + type: option.configType, + }; @@ - if (option.configType === 'custom') { + if (option.configType === 'custom') { if (existingGroupId && existingArtifactId && existingVersion && existingDriverPath) { - setDriverConfig(prev => ({ - ...prev, - groupId: existingGroupId, - artifactId: existingArtifactId, - version: existingVersion, - driverPath: existingDriverPath - })); + nextConfig = { + ...nextConfig, + groupId: existingGroupId, + artifactId: existingArtifactId, + version: existingVersion, + driverPath: existingDriverPath, + }; } } else { if (existingGroupId && existingArtifactId && existingVersion) { - setDriverConfig(prev => ({ - ...prev, - groupId: existingGroupId, - artifactId: existingArtifactId, - version: existingVersion, - driverPath: null - })); - } else if (existingDriverPath) { + nextConfig = { + ...nextConfig, + groupId: existingGroupId, + artifactId: existingArtifactId, + version: existingVersion, + driverPath: null, + }; + } else if (existingDriverPath) { setValue("driverPath", null); } } - saveDriverConfig(); + setDriverConfig(nextConfig); + await saveDriverConfig(nextConfig); } }; @@ - const saveDriverConfig = async () => { + const saveDriverConfig = async (configOverride?: DriverConfig) => { + const cfg = configOverride ?? driverConfig; try { // Validate required fields based on driver type - if (driverConfig.type === 'maven' && - (!driverConfig.groupId || !driverConfig.artifactId || !driverConfig.version)) { + if (cfg.type === 'maven' && + (!cfg.groupId || !cfg.artifactId || !cfg.version)) { @@ - if (driverConfig.type === 'maven') { + if (cfg.type === 'maven') { const validDriver = await rpcClient.getMiDiagramRpcClient().downloadDriverForConnector({ - groupId: driverConfig.groupId, - artifactId: driverConfig.artifactId, - version: driverConfig.version + groupId: cfg.groupId, + artifactId: cfg.artifactId, + version: cfg.version, }); @@ - setValue("groupId", driverConfig.groupId); - setValue("artifactId", driverConfig.artifactId); - setValue("version", driverConfig.version); - if (driverConfig.type === 'custom') { - setValue("driverPath", driverConfig.driverPath); + setValue("groupId", cfg.groupId); + setValue("artifactId", cfg.artifactId); + setValue("version", cfg.version); + if (cfg.type === 'custom') { + setValue("driverPath", cfg.driverPath); }Also ensure any
onSave={saveDriverConfig}props are updated to pass the current config (e.g.onSave={() => saveDriverConfig(driverConfig)}).Also applies to: 549-587, 646-685
1203-1208: RemoveuseEffectcalls from insideswitchcases.The
combo*andconnectionswitch branches calluseEffectdirectly insiderenderFormElement, which is conditional and depends on inputType. This violates the Rules of Hooks and matches Biome’suseHookAtTopLevelerrors.Move these effects into dedicated child components or a top‑level custom hook, e.g.:
- For
combo*: create a smallComboFieldcomponent that owns theuseEffectand wraps theAutoCompleteand dynamic field rendering.- For
connection: create aConnectionFieldcomponent that runs the effect once on mount and renders the connectionAutoComplete.Both components can receive
element,field,name,handleValueChange, and any other needed props, and be invoked as<ComboField ... />/<ConnectionField ... />from the switch, keeping hook order stable.Also applies to: 1459-1463
workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (1)
2254-2316: Align RPCDynamicFieldtype with UI usage by addingcolumnName.The RPC
DynamicField.valueobject still lackscolumnName, but the FormGenerator/DynamicFieldsHandler side expects it (seeDynamicFieldinFormGenerator.tsxand_parseSelectColumns/ SELECT building logic), and prior review already called this out.Please update the interface to include
columnNameas first field to keep contracts consistent:export interface DynamicField { type: string; value: { + columnName: string; name: string; displayName: string; inputType: string; required: string; helpTip: string; placeholder: string; defaultValue: string; }; }This ensures the MI core RPC types match what the UI actually receives and what
DynamicFieldsHandler/FormGeneratorexpect.workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (4)
197-207: Guard against undefinedqueryBuildConfigbefore building query.You now use optional chaining for
queryBuildConfig?.queryType, but still call_buildQueryFromDynamicFieldseven whenqueryBuildConfigisundefined, which will then accessconfig.resultField!and throw.Wrap the call instead of passing
?.queryType:- if (element.inputType === 'string' && element.name === FIELD_NAMES.TABLE_NAME) { - const parentElement = this.findElementByName(this.formData.elements, FIELD_NAMES.TABLE_NAME); - const queryBuildConfig = parentElement?.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]; - await this._buildQueryFromDynamicFields(FIELD_NAMES.TABLE_NAME, queryBuildConfig?.queryType , queryBuildConfig, element); - } else { + if (element.inputType === 'string' && element.name === FIELD_NAMES.TABLE_NAME) { + const parentElement = this.findElementByName(this.formData.elements, FIELD_NAMES.TABLE_NAME); + const queryBuildConfig = + parentElement?.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]; + if (queryBuildConfig?.queryType) { + await this._buildQueryFromDynamicFields( + FIELD_NAMES.TABLE_NAME, + queryBuildConfig.queryType, + queryBuildConfig, + element, + ); + } + } else { await this._handleDynamicContentChange(value, fieldName, element); }
355-387: Fix unreachableupdatedOffline && updatedOnlinecondition in visibility logic.
updatedOfflineandupdatedOnlineare set in mutually exclusive branches based onenableCondition[0].queryType, soupdatedOffline && updatedOnlinecan never betrue; the function will never return early even when it did update visibility.Change the condition to
||so you return when either mode was updated:- if (updatedOffline && updatedOnline) { + if (updatedOffline || updatedOnline) { return true; }
879-896: Avoid hard‑codingparentFielddefault in_setFieldValuefor dynamic fields.Defaulting
parentFieldto'table'still bakes in a specific section key. For dynamic fields under other parents (e.g., stored procedures), this will fail to find the correctDynamicFieldmetadata.At minimum, pass the real parent field where you know it, e.g. in
_handleManualQueryChange:- if (matched) { - this._setFieldValue(fieldDef.name, matched.value, matched.isExpression); - } else { - this._clearFieldValue(fieldDef.name); - } - if (selectColMatched) { - this._setFieldValue(fieldDef.name, true, selectColMatched.isExpression); - } + if (matched) { + this._setFieldValue(fieldDef.name, matched.value, matched.isExpression, parentField); + } else { + this._clearFieldValue(fieldDef.name); + } + if (selectColMatched) { + this._setFieldValue(fieldDef.name, true, selectColMatched.isExpression, parentField); + }and keep the
'table'default only as a backward‑compat fallback.
214-221: WrapbuildQuerycase body in a block to satisfynoSwitchDeclarations.The
const parentFielddeclaration inside theswitchcase is visible to other cases and triggers Biome’snoSwitchDeclarationserror. Wrapping this case body in a block scopes the variable correctly and silences the lint error.- case 'buildQuery': - const parentField = (element as any).parentField; - if (parentField) { - await this.onDynamicFieldChange(value, element, parentField); - } else { - console.warn(`'buildQuery' called without parentField for element:`, element.name); - } - break; + case 'buildQuery': { + const parentField = (element as any).parentField; + if (parentField) { + await this.onDynamicFieldChange(value, element, parentField); + } else { + console.warn(`'buildQuery' called without parentField for element:`, element.name); + } + break; + }Apply the same block‑wrapping pattern to other
switchcases with localconstdeclarations (INSERT/DELETE/SELECT/CALL in_buildQueryFromDynamicFieldsand the parse switch in_handleManualQueryChange) as indicated by Biome.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts(2 hunks)workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx(1 hunks)workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx(22 hunks)workspaces/mi/mi-diagram/src/components/Form/RadioButtonGroup/GenericRadioGroup.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workspaces/mi/mi-diagram/src/components/Form/RadioButtonGroup/GenericRadioGroup.tsx
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/**/*.tsx : Use React 18.2.0 features including concurrent rendering and automatic batching; avoid class components in favor of functional components with hooks
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsxworkspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-24T14:51:49.267Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 998
File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132
Timestamp: 2025-11-24T14:51:49.267Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsxworkspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-24T22:16:28.380Z
Learnt from: dan-niles
Repo: wso2/vscode-extensions PR: 980
File: workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/RichTextTemplateEditor/RichTextTemplateEditor.tsx:384-412
Timestamp: 2025-11-24T22:16:28.380Z
Learning: In the RichTextTemplateEditor component (workspaces/ballerina/ballerina-side-panel/src/components/editors/MultiModeExpressionEditor/RichTextTemplateEditor/RichTextTemplateEditor.tsx), token fetching on external `value` prop changes is intentionally disabled. Users cannot edit both the minimized and expanded editors simultaneously, so tokens only need to be generated based on user typing in the expanded editor view, not on external prop updates.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/Diagram.tsx : Implement input validation for CDModel structure: verify that all required properties (connections, listeners, services) are present and properly typed
Applied to files:
workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.tsworkspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-27T07:59:33.534Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 897
File: workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx:50-59
Timestamp: 2025-11-27T07:59:33.534Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx, the `clause.properties.expression` property in the `fillDefaults` function does not require defensive null/undefined checks because it's a required property enforced by form validation in ClauseEditor.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-26T06:34:09.752Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:128-134
Timestamp: 2025-11-26T06:34:09.752Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBreadcrumb() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/Diagram.tsx : Implement proper error boundaries around the Diagram component to gracefully handle rendering errors without crashing the parent application
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-26T07:49:56.428Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:136-141
Timestamp: 2025-11-26T07:49:56.428Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBackButton() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
🧬 Code graph analysis (2)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (4)
workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (1)
DynamicField(2255-2266)workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (1)
DynamicFieldsHandler(162-1642)workspaces/mi/mi-diagram/src/components/Form/DBConnector/DriverConfiguration.tsx (5)
DriverConfig(24-30)DRIVER_OPTIONS(50-69)DefaultDriverConfig(83-127)CustomDriverConfig(129-212)MavenDriverConfig(214-276)workspaces/mi/mi-diagram/src/components/Form/RadioButtonGroup/GenericRadioGroup.tsx (1)
GenericRadioGroup(23-59)
workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (1)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (1)
DynamicField(160-172)
🪛 Biome (2.1.2)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx
[error] 895-896: This hook is being called from a nested function, but all hooks must be called unconditionally from the top-level component.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 923-924: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 974-975: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 1203-1204: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1204-1205: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1205-1206: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 1460-1461: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
[error] 214-214: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1026-1026: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1093-1093: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1094-1094: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1095-1095: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1117-1117: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1136-1140: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1142-1142: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1257-1257: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1262-1262: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1267-1267: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1272-1272: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (3)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (2)
272-288: DynamicFieldsHandler wiring and custom error plumbing look good.The handler is instantiated once and receives
rpcClient,formData,getValues/setValue,setComboValues,parameters,dynamicFields,setDynamicFields,connectionName, andsetCustomError. ThecustomErrorsmap is correctly integrated so form rendering prefers backend‑driven errors over schema validation.Also applies to: 418-438
160-177: DynamicField/DynamicFieldGroup definitions are consistent with DynamicFieldsHandler usage.The local
DynamicField/DynamicFieldGroupexports match howDynamicFieldsHandlerand the combo/dynamic‑fields branch usevalue.columnName,displayName, andinputType, and they correctly expose afieldscollection under a header. This is aligned with the RPC side oncecolumnNameis added there.workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (1)
1923-1929: Repository access issue prevents verification ofDSSFetchTablesRequestcall sites.Unable to verify the concern about all
DSSFetchTablesRequestcall sites passing the newdriverPathfield—the repository clone operation failed repeatedly. Manual verification is needed to confirm that all usages ofDSSFetchTablesRequestin the codebase (particularly in_fetchTablesForConnectionand related functions) properly supply thedriverPathparameter, and that no existing call sites still omit this field.
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
Show resolved
Hide resolved
…amicFieldsHandler.tsx Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (6)
735-789: Offline_getDynamicFieldValuesguard forresponseColumnslooks good.The added check:
if ( operationType === QUERY_TYPES.SELECT && typeof formValues[FIELD_NAMES.RESPONSE_COLUMNS] === 'string' && formValues[FIELD_NAMES.RESPONSE_COLUMNS].trim() !== '' ) { const selectColumns = formValues[FIELD_NAMES.RESPONSE_COLUMNS] .split(',') .map(...)eliminates the previous
.splitonundefinedand properly scopes this to SELECT only. This addresses the earlier runtime risk.
1165-1174: Remove unusedparseSuccessplumbing to simplify_handleManualQueryChange.
parseSuccessis set but never read:let parseSuccess = true; ... if (!match?.groups) { parseSuccess = false; ... } ... } catch (error: any) { parseSuccess = false; parseErrorMessage = error.message || ERROR_MESSAGES.COMPLEX_QUERY;All error handling is already driven by early returns and the
catchblock;parseSuccessitself is unused.You can safely drop
parseSuccess(and related assignments) to reduce noise:- let parseSuccess = true; let parseErrorMessage = ''; @@ - parseSuccess = false; @@ - parseSuccess = false; parseErrorMessage = error.message || ERROR_MESSAGES.COMPLEX_QUERY;[Suggest similarly cleaning any other now-dead variables flagged by your linter here.]
Also applies to: 1373-1376
735-747: Clean up unused locals (paramManagerElement,ctrlName) to appease linters.A few variables are declared but never used:
_getDynamicFieldValuesoffline branch:const paramManagerElement = this.findElementByName(this.formData.elements, FIELD_NAMES.COLUMNS);
_handleManualQueryChangeoffline branch:const paramManagerElement = this.findElementByName(this.formData.elements, FIELD_NAMES.COLUMNS);
_handleOptionalClause:const ctrlName = getNameForController(fieldName);These don’t affect behavior but will keep showing up in lint/static-analysis output. You can delete them (and any associated unused imports) without changing semantics.
Also applies to: 1305-1307, 1565-1567
193-207: GuardqueryBuildConfigbefore calling_buildQueryFromDynamicFieldsfor TABLE_NAME.In the
handleDynamicContentcase:const parentElement = this.findElementByName(this.formData.elements, FIELD_NAMES.TABLE_NAME); const queryBuildConfig = parentElement?.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]; await this._buildQueryFromDynamicFields( FIELD_NAMES.TABLE_NAME, queryBuildConfig?.queryType, queryBuildConfig, element );If
queryBuildConfigisundefined(orqueryTypeis missing), you end up calling_buildQueryFromDynamicFieldswith an undefinedoperationType/config, which then falls through itsswitchand can write empty values back into the form.Wrap the call in a guard so it only runs when the config is complete:
- if (element.inputType === 'string' && element.name === FIELD_NAMES.TABLE_NAME) { - const parentElement = this.findElementByName(this.formData.elements, FIELD_NAMES.TABLE_NAME); - const queryBuildConfig = parentElement?.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]; - await this._buildQueryFromDynamicFields(FIELD_NAMES.TABLE_NAME, queryBuildConfig?.queryType , queryBuildConfig, element); - } else { + if (element.inputType === 'string' && element.name === FIELD_NAMES.TABLE_NAME) { + const parentElement = this.findElementByName(this.formData.elements, FIELD_NAMES.TABLE_NAME); + const queryBuildConfig = + parentElement?.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]; + if (queryBuildConfig?.queryType) { + await this._buildQueryFromDynamicFields( + FIELD_NAMES.TABLE_NAME, + queryBuildConfig.queryType, + queryBuildConfig, + element, + ); + } else { + console.warn( + `'handleDynamicContent' called for TABLE_NAME without a valid queryBuildConfig`, + ); + } + } else { await this._handleDynamicContentChange(value, fieldName, element); }
355-399: Fix impossible condition insetElementVisibility(updatedOffline && updatedOnline).In the TABLE/ORDER_BY special-case:
if (isHidden) { if (element?.enableCondition?.[0]?.queryType === UI_MODES.OFFLINE) { ... updatedOffline = true; } if (element?.enableCondition?.[0]?.queryType === UI_MODES.ONLINE) { ... updatedOnline = true; } } else { if (element?.enableCondition?.[0]?.queryType === UI_MODES.ONLINE) { ... updatedOnline = true; } if (element?.enableCondition?.[0]?.queryType === UI_MODES.OFFLINE) { ... updatedOffline = true; } } if (updatedOffline && updatedOnline) { return true; }
enableCondition[0].queryTypeis a single value, soupdatedOfflineandupdatedOnlineare mutually exclusive; the finalif (updatedOffline && updatedOnline)is never true.If the intent is to return when either branch updated, switch to
||:- if (updatedOffline && updatedOnline) { + if (updatedOffline || updatedOnline) { return true; }This makes the function correctly signal that the element was found and updated in either offline or online configuration.
595-700: Driver download logic mislabels success/failure and still tests connection with missing driver.In
_getValidConnectionDetails:
isDriverDownloadedstarts asfalse, and is only settrueinside the retry loop.- If
driverPathis already present from the connection, the loop is skipped,isDriverDownloadedstaysfalse, and you still hit:if (!isDriverDownloaded) { this.setCustomError(getNameForController(FIELD_NAMES.CONFIG_KEY), "Failed to download the DB driver after 5 attempts."); } connection.parameters.push({ name: FIELD_NAMES.DRIVER_PATH, value: driverPath }); const testResult = await ...loadDriverAndTestConnection({ driverPath });So for preconfigured drivers you incorrectly show a “Failed to download…” error, and when download does fail you still push
driverPath(possiblyundefined) and attemptloadDriverAndTestConnection, causing an extra failing RPC and confusing UX.Consider:
- let isDriverDownloaded = false; + let isDriverDownloaded = !!driverPath; let retryCount = 0; const maxRetries = 5; if (!driverPath) { while (!isDriverDownloaded && retryCount < maxRetries) { @@ if (driverPath) { isDriverDownloaded = true; } retryCount++; } } - if (!isDriverDownloaded) { - this.setCustomError(getNameForController(FIELD_NAMES.CONFIG_KEY), "Failed to download the DB driver after 5 attempts."); - } + if (!isDriverDownloaded || !driverPath) { + this.setCustomError( + getNameForController(FIELD_NAMES.CONFIG_KEY), + "Failed to download the DB driver after 5 attempts." + ); + this.setValue(FIELD_NAMES.ASSISTANCE_MODE, false); + this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.OFFLINE); + return undefined; + } connection.parameters.push({ name: FIELD_NAMES.DRIVER_PATH, value: driverPath, });This avoids false negatives when a driver is already configured and cleanly stops validation when download ultimately fails (also addressing the earlier suggestion about bailing out).
🧹 Nitpick comments (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (1)
878-895: Consider passing the actualparentFieldwhen setting dynamic-field values.
_setFieldValuenow accepts aparentFieldargument and uses it to locate the dynamic field:private _setFieldValue( fieldName: string, value: string | number | boolean | null | undefined, isExpression: boolean = false, parentField: string = 'table' ): void { const targetElement = this.findElementByName(this.formData.elements, getNameForController(fieldName)); const dynamicField = this.dynamicFields[parentField]?.fields.find( (f: DynamicField) => f.value.name === fieldName );Most call sites still rely on the default
'table', including where you update dynamic fields in_handleManualQueryChange. If in future you introduce dynamic fields under other parent keys (e.g. different groups per operation), those updates will silently miss.Not urgent for current usage, but you may want to:
- Thread the actual
parentFieldthrough call sites that are updating dynamic fields (e.g._handleManualQueryChangewhen applyingmatchedFieldsandselectMatchedFields), and- Reserve the default
'table'only for the specific DB TABLE_NAME group.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-11-24T14:51:49.267Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 998
File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132
Timestamp: 2025-11-24T14:51:49.267Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-27T07:59:33.534Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 897
File: workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx:50-59
Timestamp: 2025-11-27T07:59:33.534Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx, the `clause.properties.expression` property in the `fillDefaults` function does not require defensive null/undefined checks because it's a required property enforced by form validation in ClauseEditor.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-26T06:34:09.752Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:128-134
Timestamp: 2025-11-26T06:34:09.752Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBreadcrumb() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-26T07:49:56.428Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:136-141
Timestamp: 2025-11-26T07:49:56.428Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBackButton() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/Diagram.tsx : Implement input validation for CDModel structure: verify that all required properties (connections, listeners, services) are present and properly typed
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/Diagram.tsx : Implement proper error boundaries around the Diagram component to gracefully handle rendering errors without crashing the parent application
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-26T06:33:22.950Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:86-112
Timestamp: 2025-11-26T06:33:22.950Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, empty catch blocks around progress ring waitForSelector calls (state: 'attached' and 'detached') are intentional. The progress ring duration depends on machine performance and may appear very briefly, causing the wait to miss the event. The try-catch allows the test to proceed gracefully regardless of timing.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
🧬 Code graph analysis (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (1)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (4)
DynamicFieldGroup(174-177)Element(119-153)getNameForController(179-184)DynamicField(160-172)
🪛 Biome (2.1.2)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
[error] 214-214: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1025-1025: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1092-1092: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1093-1093: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1094-1094: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1116-1116: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1135-1139: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1141-1141: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1256-1256: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1261-1261: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1266-1266: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1271-1271: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (3)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (3)
1631-1640: AwaitonConnectionChangeinside_handleAssistanceModeChangeto avoid unhandled rejections.
_handleAssistanceModeChangeisasync, but it calls the asynconConnectionChangewithoutawait:if (value === true) { this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.ONLINE); this.onConnectionChange(fieldName, rpc) } else { ... }Because
handleValueChangeawaits_handleAssistanceModeChangeand wraps it in atry/catch, any rejection fromonConnectionChangewill currently bypass that error handling and surface as an unhandled promise rejection.Change to:
- if (value === true) { - this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.ONLINE); - this.onConnectionChange(fieldName, rpc) - } else { + if (value === true) { + this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.ONLINE); + await this.onConnectionChange(fieldName, rpc); + } else {so connection-change failures are caught by the surrounding
try/catchinhandleValueChange.
1555-1607: Prepared-statement builders may ignore DB type when encoding column names.In
_buildSelectPreparedStatement,_buildInsertPreparedStatement, and_buildDeletePreparedStatement, verify whether_encodeColumnNameis called with thedbTypeparameter. If these methods do not passdbType, they fall back to default double-quote behavior, causing inconsistent identifier quoting across database types (e.g., MySQL backticks vs. SQL Server brackets).Since
_handleManualQueryChangeresolvesdbTypeand passes it when encoding the table name, the reconstructed prepared statements should also receivedbTypeto maintain consistency. Consider threadingdbTypeas a parameter through the prepared statement builder methods and passing it from_handleManualQueryChange.
213-221: Wrapswitchcase bodies that declare variables in blocks (BiomenoSwitchDeclarations).Several
switchcases declareconst/letdirectly in the case clause, triggering Biome'snoSwitchDeclarationsrule:
handleValueChange→case 'buildQuery'(Line 214)._buildQueryFromDynamicFields→case QUERY_TYPES.INSERT,DELETE,SELECT,CALL(Lines 1024–1151)._handleManualQueryChange→case QUERY_TYPES.SELECT | DELETE | INSERT | CALL(Lines 1247–1274).Lexical declarations in case clauses are hoisted to the entire switch statement and can be used before initialization across multiple cases. Wrap each case body in its own block to isolate scope:
- case 'buildQuery': - const parentField = (element as any).parentField; - if (parentField) { - await this.onDynamicFieldChange(value, element, parentField); - } else { - console.warn(`'buildQuery' called without parentField for element:`, element.name); - } - break; + case 'buildQuery': { + const parentField = (element as any).parentField; + if (parentField) { + await this.onDynamicFieldChange(value, element, parentField); + } else { + console.warn(`'buildQuery' called without parentField for element:`, element.name); + } + break; + }Apply this pattern to all INSERT/DELETE/SELECT/CALL branches in
_buildQueryFromDynamicFieldsand_handleManualQueryChange.Also applies to: 1023–1151, 1247–1275
…amicFieldsHandler.tsx Co-authored-by: Copilot <[email protected]>
…amicFieldsHandler.tsx Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (4)
200-207: Guard_buildQueryFromDynamicFieldswhen TABLE_NAME build config is missing.
queryBuildConfigcan beundefined, but_buildQueryFromDynamicFieldsassumes a non‑null config and will dereferenceconfig.*internally. With the current call, a missingonDynamicFieldChangewiring will surface as a runtime crash during query build rather than a clean no‑op/log.Consider guarding before calling:
- case 'handleDynamicContent': - if (element.inputType === 'string' && element.name === FIELD_NAMES.TABLE_NAME) { - const parentElement = this.findElementByName(this.formData.elements, FIELD_NAMES.TABLE_NAME); - const queryBuildConfig = parentElement?.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]; - await this._buildQueryFromDynamicFields(FIELD_NAMES.TABLE_NAME, queryBuildConfig?.queryType , queryBuildConfig, element); - } else { - await this._handleDynamicContentChange(value, fieldName, element); - } + case 'handleDynamicContent': + if (element.inputType === 'string' && element.name === FIELD_NAMES.TABLE_NAME) { + const parentElement = this.findElementByName( + this.formData.elements, + FIELD_NAMES.TABLE_NAME, + ); + const queryBuildConfig = + parentElement?.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]; + + if (queryBuildConfig?.queryType) { + await this._buildQueryFromDynamicFields( + FIELD_NAMES.TABLE_NAME, + queryBuildConfig.queryType, + queryBuildConfig, + element, + ); + } else { + console.warn( + "'handleDynamicContent' called for TABLE_NAME without a valid buildQuery configuration", + ); + } + } else { + await this._handleDynamicContentChange(value, fieldName, element); + }This keeps the TABLE_NAME fast‑path safe even if the form schema changes.
214-221: Wrap switch‑case variable declarations in blocks to satisfy Biome and avoid bleed‑through.Biome is flagging
noSwitchDeclarationsfor declarations likeconst parentField = …(Line 214),const insertColumns = …(Line 1025), and the variousconst/letdeclarations inside theoperationTypeandQUERY_TYPES.*switches. While you currentlybreakin each case, TS/JS still treats these declarations as shared across cases, which is what Biome is complaining about.You can fix this by wrapping case bodies that declare variables in their own blocks. For example:
case 'buildQuery': - const parentField = (element as any).parentField; - if (parentField) { - await this.onDynamicFieldChange(value, element, parentField); - } else { - console.warn(`'buildQuery' called without parentField for element:`, element.name); - } + { + const parentField = (element as any).parentField; + if (parentField) { + await this.onDynamicFieldChange(value, element, parentField); + } else { + console.warn( + `'buildQuery' called without parentField for element:`, + element.name, + ); + } + } break;And in
_buildQueryFromDynamicFields:- switch (operationType) { - case QUERY_TYPES.INSERT: - const insertColumns = Object.values(activeFields); - if (insertColumns.length > 0) { + switch (operationType) { + case QUERY_TYPES.INSERT: { + const insertColumns = Object.values(activeFields); + if (insertColumns.length > 0) { // ... - } else { + } else { // ... - } - - break; + } + break; + }Apply the same
{ … }wrapping pattern to the othercaseblocks that declareconst/let(INSERT/DELETE/SELECT/CALL here and in_handleManualQueryChange) so Biome’snoSwitchDeclarationserrors are resolved.Also applies to: 1023-1151, 1248-1275
361-385:updatedOffline && updatedOnlinecondition is unreachable; use OR or simplify.Within the TABLE_NAME / ORDER_BY special case,
updatedOfflineandupdatedOnlineare set in mutually exclusive branches depending onenableCondition[0].queryType, so the condition:if (updatedOffline && updatedOnline) { return true; }can never be true for a single element. The body of
setElementVisibilitystill works due to side‑effects, but this return condition is effectively dead code and misleading.If the intent is to signal success when either flag was updated, switch to
||:- if (updatedOffline && updatedOnline) { + if (updatedOffline || updatedOnline) { return true; }Alternatively, if the return value is not used for these fields, consider removing the flags/condition entirely for clarity.
631-687: Fix driver download flow: treat existing driverPath as success and bail out cleanly on repeated failures.The current logic around
isDriverDownloadedhas two problems:
- If
driverPathis already present on the connection,isDriverDownloadedstaysfalse, so you still set the “Failed to download…” error even though no download was attempted.- When download retries all fail, you set the error but still push an
undefineddriverPathintoconnection.parametersand proceed toloadDriverAndTestConnection, causing a guaranteed failing RPC.You can simplify by keying solely off
driverPathand returning early on failure:- let isDriverDownloaded = false; - let retryCount = 0; - const maxRetries = 5; - if (!driverPath) { - while (!isDriverDownloaded && retryCount < maxRetries) { - const args = { - groupId: groupId, - artifactId: artifactId, - version: version, - }; - this.setCustomError(getNameForController(FIELD_NAMES.CONFIG_KEY), "Checking DB Driver..."); - driverPath = await this.rpcClient.getMiDiagramRpcClient().downloadDriverForConnector(args); - if (driverPath) { - isDriverDownloaded = true; - } - retryCount++; - } - } - if (!isDriverDownloaded) { - this.setCustomError(getNameForController(FIELD_NAMES.CONFIG_KEY), "Failed to download the DB driver after 5 attempts."); - } + if (!driverPath) { + let retryCount = 0; + const maxRetries = 5; + while (!driverPath && retryCount < maxRetries) { + const args = { groupId, artifactId, version }; + this.setCustomError( + getNameForController(FIELD_NAMES.CONFIG_KEY), + "Checking DB Driver...", + ); + driverPath = await this.rpcClient + .getMiDiagramRpcClient() + .downloadDriverForConnector(args); + retryCount++; + } + + if (!driverPath) { + this.setCustomError( + getNameForController(FIELD_NAMES.CONFIG_KEY), + "Failed to download the DB driver after 5 attempts.", + ); + this.setValue(FIELD_NAMES.ASSISTANCE_MODE, false); + this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.OFFLINE); + return undefined; + } + }After this block,
driverPathis guaranteed to be truthy (either pre‑configured or downloaded), so pushing it intoconnection.parametersand testing the connection is safe and avoids an extra failing RPC and misleading error state.
🧹 Nitpick comments (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (1)
1630-1638: AwaitonConnectionChangeinside_handleAssistanceModeChangefor predictable async flow.
_handleAssistanceModeChangeis declaredasyncand is awaited fromhandleValueChange, but the internal call toonConnectionChangeis not awaited:if (value === true) { this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.ONLINE); this.onConnectionChange(fieldName, rpc) }This means the caller may proceed assuming the online UI/combos are updated while the connection test and table fetch are still in progress.
Consider awaiting the call so the assistance‑mode toggle behaves transactionally:
private async _handleAssistanceModeChange(value: any, fieldName: string, rpc?: string): Promise<void> { if (value === true) { this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.ONLINE); - this.onConnectionChange(fieldName, rpc) + await this.onConnectionChange(fieldName, rpc); } else {This way, any code awaiting
_handleAssistanceModeChangehas a clear point at which the connection change is fully processed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-11-24T14:51:49.267Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 998
File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132
Timestamp: 2025-11-24T14:51:49.267Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-27T07:59:33.534Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 897
File: workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx:50-59
Timestamp: 2025-11-27T07:59:33.534Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx, the `clause.properties.expression` property in the `fillDefaults` function does not require defensive null/undefined checks because it's a required property enforced by form validation in ClauseEditor.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-26T06:34:09.752Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:128-134
Timestamp: 2025-11-26T06:34:09.752Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBreadcrumb() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-26T07:49:56.428Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:136-141
Timestamp: 2025-11-26T07:49:56.428Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBackButton() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/Diagram.tsx : Implement input validation for CDModel structure: verify that all required properties (connections, listeners, services) are present and properly typed
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/Diagram.tsx : Implement proper error boundaries around the Diagram component to gracefully handle rendering errors without crashing the parent application
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-26T06:33:22.950Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:86-112
Timestamp: 2025-11-26T06:33:22.950Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, empty catch blocks around progress ring waitForSelector calls (state: 'attached' and 'detached') are intentional. The progress ring duration depends on machine performance and may appear very briefly, causing the wait to miss the event. The try-catch allows the test to proceed gracefully regardless of timing.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
🪛 Biome (2.1.2)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
[error] 214-214: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1025-1025: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1092-1092: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1093-1093: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1094-1094: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1116-1116: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1135-1139: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1141-1141: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1256-1256: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1261-1261: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1266-1266: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1271-1271: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
| } catch (error) { | ||
| console.error("Error testing DB connection:", error); | ||
| this.setCustomError(FIELD_NAMES.CONFIG_KEY, ERROR_MESSAGES.CONNECTION_FAILED + " (RPC Error)"); | ||
| this.setValue(FIELD_NAMES.ASSISTANCE_MODE, false); | ||
| this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.OFFLINE); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Use getNameForController consistently when setting CONFIG_KEY error.
Everywhere else in this file you call setCustomError with getNameForController(FIELD_NAMES.CONFIG_KEY), but the RPC error path uses the raw field name:
this.setCustomError(FIELD_NAMES.CONFIG_KEY, ERROR_MESSAGES.CONNECTION_FAILED + " (RPC Error)");If setCustomError expects the controller name (as in the other call sites), this path may fail to show the error on the correct field.
Align it with the rest:
- this.setCustomError(FIELD_NAMES.CONFIG_KEY, ERROR_MESSAGES.CONNECTION_FAILED + " (RPC Error)");
+ this.setCustomError(
+ getNameForController(FIELD_NAMES.CONFIG_KEY),
+ ERROR_MESSAGES.CONNECTION_FAILED + " (RPC Error)",
+ );This keeps error targeting consistent across all connection‑validation failures.
🤖 Prompt for AI Agents
In
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
around lines 693 to 699, the RPC error path calls setCustomError with the raw
FIELD_NAMES.CONFIG_KEY; change this to use
getNameForController(FIELD_NAMES.CONFIG_KEY) like the other call sites so the
error targets the correct form controller. Update the setCustomError call to
pass getNameForController(FIELD_NAMES.CONFIG_KEY) and ensure
getNameForController is in scope (it already is used elsewhere in the file).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (5)
744-789: Remove unusedparamManagerElement(and keep offline path focused).In
_getDynamicFieldValues’ offline branch:const paramManagerElement = this.findElementByName(this.formData.elements, FIELD_NAMES.COLUMNS); if (currentQueryType === UI_MODES.OFFLINE) { const paramManagerValues = formValues[FIELD_NAMES.COLUMNS] || []; // ... }
paramManagerElementisn’t used anywhere in this method. It looks like a leftover from an earlier iteration.Safe to drop:
- // Handling ParamManager (Offline Mode) - const paramManagerElement = this.findElementByName(this.formData.elements, FIELD_NAMES.COLUMNS); - if (currentQueryType === UI_MODES.OFFLINE) { + // Handling ParamManager (Offline Mode) + if (currentQueryType === UI_MODES.OFFLINE) {Same pattern appears later in
_handleManualQueryChangearound the offline param‑manager update; that local is also unused and can be removed in the same way.
197-221: GuardqueryBuildConfigbefore calling_buildQueryFromDynamicFieldsand address switch‑case scoping.In the
'handleDynamicContent'branch you now passqueryBuildConfig?.queryType:const queryBuildConfig = parentElement?.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]; await this._buildQueryFromDynamicFields( FIELD_NAMES.TABLE_NAME, queryBuildConfig?.queryType, queryBuildConfig, element );If
queryBuildConfigisundefined(orqueryTypeis missing), this will still call_buildQueryFromDynamicFieldswith an undefinedoperationTypeandconfig, which will later accessconfig.*and switch onoperationType. That’s a potential runtime error and also breaks the intended type contract foroperationType.Also, Biome is flagging
noSwitchDeclarationshere becauseconst parentField = ...is declared directly in acasewithout a block.Consider:
- Guarding before calling
_buildQueryFromDynamicFields.- Wrapping the case body in a block so declarations are scoped to that case.
Example fix:
- case 'handleDynamicContent': - if (element.inputType === 'string' && element.name === FIELD_NAMES.TABLE_NAME) { - const parentElement = this.findElementByName(this.formData.elements, FIELD_NAMES.TABLE_NAME); - const queryBuildConfig = parentElement?.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]; - await this._buildQueryFromDynamicFields(FIELD_NAMES.TABLE_NAME, queryBuildConfig?.queryType , queryBuildConfig, element); - } else { - await this._handleDynamicContentChange(value, fieldName, element); - } - break; + case 'handleDynamicContent': { + if (element.inputType === 'string' && element.name === FIELD_NAMES.TABLE_NAME) { + const parentElement = this.findElementByName(this.formData.elements, FIELD_NAMES.TABLE_NAME); + const queryBuildConfig = + parentElement?.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]; + + if (queryBuildConfig?.queryType) { + await this._buildQueryFromDynamicFields( + FIELD_NAMES.TABLE_NAME, + queryBuildConfig.queryType, + queryBuildConfig, + element, + ); + } else { + console.warn( + "'handleDynamicContent' called but queryBuildConfig/queryType is missing for TABLE_NAME", + { parentElement, fieldName: element.name }, + ); + } + } else { + await this._handleDynamicContentChange(value, fieldName, element); + } + break; + }You’ll also want to apply the same
{ ... }wrapping pattern to otherswitchcases that declareconst/let(e.g., in_buildQueryFromDynamicFieldsand_handleManualQueryChange) to satisfy Biome’snoSwitchDeclarationsand keep case scopes tight.
355-399: FixsetElementVisibilityflag logic:updatedOffline && updatedOnlinecan never be true.In the TABLE/ORDER_BY special‑case branch:
let updatedOffline = false; let updatedOnline = false; // ... if (isHidden) { if (element?.enableCondition?.[0]?.queryType === UI_MODES.OFFLINE) { element.value.hidden = true; updatedOffline = true; } if (element?.enableCondition?.[0]?.queryType === UI_MODES.ONLINE) { element.value.hidden = false; updatedOnline = true; } } else { // symmetric logic... } if (updatedOffline && updatedOnline) { return true; }Because each element’s
enableCondition[0].queryTypeis eitherofflineoronline, only one ofupdatedOffline/updatedOnlinecan ever be set for a given element, so the&&condition is effectively dead code.If the goal is to return
truewhen either variant is updated, switch to||:- if (updatedOffline && updatedOnline) { + if (updatedOffline || updatedOnline) { return true; }This also aligns with the earlier review comment on this block.
631-687: CorrectdriverPath/isDriverDownloadedhandling and bail out when the driver is unavailable.There are two intertwined issues in
_getValidConnectionDetails:
False “download failed” error when a driver path already exists.
isDriverDownloadedis initialized tofalse:let isDriverDownloaded = false; let retryCount = 0; const maxRetries = 5; if (!driverPath) { // retry loop... } if (!isDriverDownloaded) { this.setCustomError(getNameForController(FIELD_NAMES.CONFIG_KEY), "Failed to download the DB driver after 5 attempts."); }- If
driverPathis already present on the connection, the retry loop is skipped andisDriverDownloadedstaysfalse, so you always set the “after 5 attempts” error even though no download was attempted and a driver path exists.Continuing with connection test when no driver was obtained.
- Even if downloads fail and
driverPathremains falsy, the code still:
- Pushes a DRIVER_PATH parameter with an undefined value.
- Proceeds to
loadDriverAndTestConnection, which will inevitably fail and generate a second, less clear error.You can fix both by:
- Treating an existing
driverPathas “downloaded”.- Early‑returning when, after retries, there is still no
driverPath, instead of proceeding to push parameters and test the connection.Example refactor:
- let driverPath = connection.parameters.find(p => p.name === FIELD_NAMES.DRIVER_PATH)?.value; + let driverPath = connection.parameters.find(p => p.name === FIELD_NAMES.DRIVER_PATH)?.value; @@ - let isDriverDownloaded = false; + let isDriverDownloaded = !!driverPath; let retryCount = 0; const maxRetries = 5; if (!driverPath) { while (!isDriverDownloaded && retryCount < maxRetries) { @@ if (driverPath) { isDriverDownloaded = true; } retryCount++; } } - if (!isDriverDownloaded) { - this.setCustomError(getNameForController(FIELD_NAMES.CONFIG_KEY), "Failed to download the DB driver after 5 attempts."); - } - connection.parameters.push({ - name: FIELD_NAMES.DRIVER_PATH, - value: driverPath, - }); + if (!isDriverDownloaded || !driverPath) { + this.setCustomError( + getNameForController(FIELD_NAMES.CONFIG_KEY), + "Failed to download the DB driver after 5 attempts.", + ); + this.setValue(FIELD_NAMES.ASSISTANCE_MODE, false); + this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.OFFLINE); + return undefined; + } + + const existingDriverParam = connection.parameters.find( + (p) => p.name === FIELD_NAMES.DRIVER_PATH, + ); + if (existingDriverParam) { + existingDriverParam.value = driverPath; + } else { + connection.parameters.push({ + name: FIELD_NAMES.DRIVER_PATH, + value: driverPath, + }); + }This resolves the incorrect error when a driver is already configured and avoids an unnecessary failing RPC when the driver path cannot be obtained, matching the earlier “stop validation early when download fails” guidance.
693-699: UsegetNameForControllerwhen setting CONFIG_KEY RPC error.In the RPC error path you still call:
this.setCustomError(FIELD_NAMES.CONFIG_KEY, ERROR_MESSAGES.CONNECTION_FAILED + " (RPC Error)");Everywhere else in the file you pass
getNameForController(FIELD_NAMES.CONFIG_KEY)as the field identifier. For consistency, and to ensure the error attaches to the same controller as other CONFIG_KEY errors, align this call:- this.setCustomError(FIELD_NAMES.CONFIG_KEY, ERROR_MESSAGES.CONNECTION_FAILED + " (RPC Error)"); + this.setCustomError( + getNameForController(FIELD_NAMES.CONFIG_KEY), + ERROR_MESSAGES.CONNECTION_FAILED + " (RPC Error)", + );
🧹 Nitpick comments (2)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (2)
1172-1199: Remove unusedparseSuccessflag (minor cleanup).In
_handleManualQueryChange:let parseSuccess = true; let parseErrorMessage = ''; // ... if (!match?.groups) { parseSuccess = false; this.setCustomError(queryFieldName, ERROR_MESSAGES.COMPLEX_QUERY); // ... return; }
parseSuccessis assigned but never read; the control flow already returns early on failure and the catch block bases its behavior on thrown errors, not this flag.You can drop
parseSuccessentirely to reduce noise:- let parseSuccess = true; let parseErrorMessage = ''; @@ - parseSuccess = false; this.setCustomError(queryFieldName, ERROR_MESSAGES.COMPLEX_QUERY);
1630-1638: AwaitonConnectionChangeinside_handleAssistanceModeChange(optional).Here:
if (value === true) { this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.ONLINE); this.onConnectionChange(fieldName, rpc) } else { // ... }
onConnectionChangeis async and performs RPCs and UI updates. Calling it withoutawaitmeans any thrown error won’t be caught by callers awaiting_handleAssistanceModeChange, and the method can appear “done” before connection state is updated.Consider:
- if (value === true) { - this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.ONLINE); - this.onConnectionChange(fieldName, rpc) - } else { + if (value === true) { + this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.ONLINE); + await this.onConnectionChange(fieldName, rpc); + } else {This keeps async error handling and sequencing consistent with the rest of the handler.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-11-24T14:51:49.267Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 998
File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132
Timestamp: 2025-11-24T14:51:49.267Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-27T07:59:33.534Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 897
File: workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx:50-59
Timestamp: 2025-11-27T07:59:33.534Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx, the `clause.properties.expression` property in the `fillDefaults` function does not require defensive null/undefined checks because it's a required property enforced by form validation in ClauseEditor.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-26T06:34:09.752Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:128-134
Timestamp: 2025-11-26T06:34:09.752Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBreadcrumb() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-26T07:49:56.428Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:136-141
Timestamp: 2025-11-26T07:49:56.428Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBackButton() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/Diagram.tsx : Implement input validation for CDModel structure: verify that all required properties (connections, listeners, services) are present and properly typed
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/Diagram.tsx : Implement proper error boundaries around the Diagram component to gracefully handle rendering errors without crashing the parent application
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
📚 Learning: 2025-11-26T06:33:22.950Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:86-112
Timestamp: 2025-11-26T06:33:22.950Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, empty catch blocks around progress ring waitForSelector calls (state: 'attached' and 'detached') are intentional. The progress ring duration depends on machine performance and may appear very briefly, causing the wait to miss the event. The try-catch allows the test to proceed gracefully regardless of timing.
Applied to files:
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
🧬 Code graph analysis (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (1)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (4)
DynamicFieldGroup(174-177)Element(119-153)getNameForController(179-184)DynamicField(160-172)
🪛 Biome (2.1.2)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
[error] 214-214: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1025-1025: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1092-1092: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1093-1093: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1094-1094: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1116-1116: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1135-1139: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1141-1141: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1256-1256: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1261-1261: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1266-1266: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1271-1271: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
| const connectionData = await this._getDbConnectionDetails(); | ||
| const dbType = this._getConnectionDbType(connectionData); | ||
| const encodedTableName = this._encodeColumnName(cleanTableName, dbType); | ||
|
|
||
| switch (operationType) { | ||
| case QUERY_TYPES.SELECT: | ||
| preparedStatement = this._buildSelectPreparedStatement(columns, encodedTableName, matchedFields, orderBy, limit, offset); | ||
| break; | ||
| case QUERY_TYPES.INSERT: | ||
| preparedStatement = this._buildInsertPreparedStatement(encodedTableName, matchedFields); | ||
| break; | ||
| case QUERY_TYPES.DELETE: | ||
| preparedStatement = this._buildDeletePreparedStatement(encodedTableName, matchedFields); | ||
| break; | ||
| case QUERY_TYPES.CALL: | ||
| preparedStatement = this._buildCallPreparedStatement(encodedTableName, matchedFields); | ||
| break; | ||
| } | ||
| this._setFieldValue(config.preparedResultField!, preparedStatement, false); | ||
| // Clear any previous error message upon successful parsing | ||
| this.setCustomError(queryFieldName, null); | ||
|
|
||
| } catch (error: any) { | ||
| parseErrorMessage = error.message || ERROR_MESSAGES.COMPLEX_QUERY; | ||
| console.warn(`Query parsing error: ${parseErrorMessage}`); | ||
| this.setCustomError(queryFieldName, `Query parsing error: ${parseErrorMessage}`); | ||
|
|
||
| // Clear dependent fields on parse error | ||
| this._clearFieldValue(config.preparedResultField!); | ||
| this._clearFieldValue(config.columnNamesField!); | ||
| } | ||
|
|
||
| } | ||
|
|
||
|
|
||
| /** Helper: Parses WHERE clause conditions */ | ||
| private _parseWhereClause(whereClause: string | undefined, availableFields: Record<string, DynamicFieldValue>, connectionInfo: boolean): { success: boolean, fields: Record<string, DynamicFieldValue>, errorMessage?: string } { | ||
| const matchedFields: Record<string, DynamicFieldValue> = {}; | ||
| if (!whereClause) { | ||
| return { success: true, fields: matchedFields }; | ||
| } | ||
|
|
||
| // conditions within parenthesis are considered out of scope since it introduces unwanted complexicity | ||
| const conditions = whereClause.trim().split(/\s+AND\s+/i); | ||
|
|
||
| for (const condition of conditions) { | ||
| const conditionMatch = condition.match(REGEX.WHERE_CONDITION_PAIR); | ||
|
|
||
| if (!conditionMatch) { | ||
| console.warn(`${ERROR_MESSAGES.PARSE_WHERE_CONDITION}: ${condition}`); | ||
| return { success: false, fields: {}, errorMessage: `${ERROR_MESSAGES.PARSE_WHERE_CONDITION}: "${condition}"` }; | ||
| } | ||
|
|
||
| // Extract parts: column, operator, valueStr | ||
| const [, rawColumn, , rawValueStr] = conditionMatch; | ||
| //const columnName = rawColumn.replace(/[^a-zA-Z0-9]/g, '').trim(); | ||
| const columnName = rawColumn.trim().replace(/[`'"\[\]]/g, ''); // Remove quotes/brackets | ||
|
|
||
| const valueStr = rawValueStr.trim(); | ||
|
|
||
| // if connection info is false build a dummy field | ||
| const dynamicField = connectionInfo ? availableFields[columnName] : { | ||
| name: columnName, | ||
| displayName: columnName, | ||
| value: valueStr, | ||
| isExpression: false, | ||
| columnType: "VARCHAR", | ||
| helpTip: '' | ||
| }; | ||
|
|
||
| if (!dynamicField) { | ||
| console.warn(`Dynamic field definition not found for WHERE column: ${columnName}`); | ||
| return { success: false, fields: {}, errorMessage: `Field "${rawColumn}" not found for this table.` }; | ||
| } | ||
|
|
||
| // Remove leading/trailing single or double quotes | ||
| const fieldValue = valueStr.replace(/^['"]|['"]$/g, ''); | ||
|
|
||
| matchedFields[columnName] = dynamicField; | ||
| matchedFields[columnName].value = fieldValue; | ||
| const isExpression = REGEX.SYNAPSE_EXPRESSION.test(fieldValue); | ||
|
|
||
| matchedFields[columnName] = { ...dynamicField, value: fieldValue, isExpression: isExpression }; | ||
| } | ||
| return { success: true, fields: matchedFields }; | ||
| } | ||
|
|
||
| /** Helper: Parses INSERT columns and values */ | ||
| private _parseInsertValues(columnsStr: string | undefined, valuesStr: string | undefined, availableFields: Record<string, DynamicFieldValue>, connectionInfo: boolean): { success: boolean, fields: Record<string, DynamicFieldValue>, errorMessage?: string } { | ||
| const matchedFields: Record<string, DynamicFieldValue> = {}; | ||
|
|
||
| if (!columnsStr || !valuesStr) { | ||
| return { success: false, fields: {}, errorMessage: ERROR_MESSAGES.INSERT_MISMATCH + " (missing columns or values)" }; | ||
| } | ||
|
|
||
| // this does not support column names such as `Random, "``Name,' , .````test` | ||
| // this is an acceptable limitation since this only occurs when user manually enters the query | ||
|
|
||
| const columns = columnsStr.split(',').map(col => col.trim().replace(/[`'"\[\]]/g, '')); | ||
| const values = valuesStr.split(',').map(val => val.trim().replace(/^['"]|['"]$/g, '')); // Remove quotes | ||
|
|
||
| if (columns.length !== values.length || columns.length === 0) { | ||
| console.warn(ERROR_MESSAGES.INSERT_MISMATCH); | ||
| return { success: false, fields: {}, errorMessage: ERROR_MESSAGES.INSERT_MISMATCH }; | ||
| } | ||
| for (let i = 0; i < columns.length; i++) { | ||
| const columnName = columns[i]; | ||
| const valueStr = values[i]; | ||
| const dynamicField = connectionInfo ? availableFields[columnName] : { | ||
| name: columnName, | ||
| displayName: columnName, | ||
| value: valueStr, | ||
| isExpression: false, | ||
| columnType: "VARCHAR", | ||
| helpTip: '' | ||
| }; | ||
| if (!dynamicField) { | ||
| console.warn(`Dynamic field definition not found for INSERT column: ${columnName}`); | ||
| return { success: false, fields: {}, errorMessage: `Field "${columnName}" not found for this table.` }; | ||
| } | ||
|
|
||
| // Detect if value is expression or literal | ||
| const isSynapseExpr = REGEX.SYNAPSE_EXPRESSION.test(valueStr); | ||
| const isLegacyExpr = REGEX.EXPRESSION.test(valueStr); | ||
| const isExpression = isSynapseExpr || isLegacyExpr; | ||
|
|
||
| matchedFields[columnName] = { ...dynamicField, value: valueStr, isExpression: isExpression }; | ||
| } | ||
|
|
||
| return { success: true, fields: matchedFields }; | ||
| } | ||
|
|
||
| /** Helper: Parses CALL parameters */ | ||
| private _parseCallParams(valuesStr: string | undefined, availableFields: Record<string, DynamicFieldValue>, connectionInfo: boolean): { success: boolean, fields: Record<string, DynamicFieldValue>, errorMessage?: string } { | ||
| const matchedFields: Record<string, DynamicFieldValue> = {}; | ||
| if (!valuesStr) { | ||
| return { success: false, fields: {}, errorMessage: "No parameters provided." }; | ||
| } | ||
| const values = valuesStr.split(',').map(val => val.trim().replace(/^['"]|['"]$/g, '')); | ||
|
|
||
| // there should be values matching all of the dynamic fields | ||
| // iterate over availbale fields with counter | ||
| if (!connectionInfo) { | ||
| // For offline mode, iterate over values array | ||
| for (let i = 0; i < values.length; i++) { | ||
| const valueStr = values[i]; | ||
| const fieldName = `param${i + 1}`; | ||
|
|
||
| const dynamicField = { | ||
| name: fieldName, | ||
| displayName: fieldName, | ||
| value: valueStr, | ||
| isExpression: false, | ||
| columnType: "VARCHAR", | ||
| helpTip: '' | ||
| }; | ||
|
|
||
| const isSynapseExpr = REGEX.SYNAPSE_EXPRESSION.test(valueStr); | ||
| const isLegacyExpr = REGEX.EXPRESSION.test(valueStr); | ||
| const isExpression = isSynapseExpr || isLegacyExpr; | ||
|
|
||
| matchedFields[fieldName] = { ...dynamicField, value: valueStr, isExpression: isExpression }; | ||
| } | ||
| } else { | ||
| // For online mode, iterate over available fields | ||
| let counter = 0; | ||
| for (const fieldName in availableFields) { | ||
| const dynamicField = availableFields[fieldName]; | ||
|
|
||
| if (!dynamicField) { | ||
| console.warn(`Dynamic field definition not found for CALL column: ${fieldName}`); | ||
| return { success: false, fields: {}, errorMessage: `Field "${fieldName}" not found for this table.` }; | ||
| } | ||
|
|
||
| // Check if value exists for this field | ||
| if (counter >= values.length) { | ||
| console.warn(`Not enough values provided for CALL parameters.`); | ||
| return { success: false, fields: {}, errorMessage: `Not enough values provided for CALL parameters.` }; | ||
| } | ||
|
|
||
| const valueStr = values[counter]; | ||
| const isSynapseExpr = REGEX.SYNAPSE_EXPRESSION.test(valueStr); | ||
| const isLegacyExpr = REGEX.EXPRESSION.test(valueStr); | ||
| const isExpression = isSynapseExpr || isLegacyExpr; | ||
|
|
||
| matchedFields[fieldName] = { ...dynamicField, value: valueStr, isExpression: isExpression }; | ||
| counter++; | ||
| } | ||
| } | ||
|
|
||
| return { success: true, fields: matchedFields }; | ||
| } | ||
| /** Helper: Parses SELECT columns */ | ||
| private _parseSelectColumns(columnsStr: string | undefined, availableFields: Record<string, DynamicFieldValue>, connectionInfo: boolean): { success: boolean, fields: Record<string, DynamicFieldValue>, errorMessage?: string } { | ||
| if (!columnsStr) return { success: false, fields: {}, errorMessage: "No columns specified for SELECT." }; | ||
| // Handle SELECT * case | ||
| // if (columnsStr.trim() === '*') { | ||
| // return { success: true, fields: availableFields }; | ||
| // } | ||
| const columns = columnsStr.split(',').map(col => col.trim()); | ||
| const matchedFields: Record<string, DynamicFieldValue> = {}; | ||
| for (const col of columns) { | ||
| const field = Object.values(availableFields).find(f => f.columnName && f.columnName === col); | ||
| if (!field) { | ||
| console.warn(`No matching field found for column: ${col}`); | ||
| return { success: false, fields: {}, errorMessage: `No matching field found for column: ${col}` }; | ||
| } | ||
| matchedFields[col] = field; | ||
| } | ||
| return { success: true, fields: matchedFields }; | ||
| } | ||
|
|
||
| /** Helper: Handles optional clauses like ORDER BY, LIMIT, OFFSET during parsing */ | ||
| private _handleOptionalClause(clauseValue: string | undefined, fieldName: string, checkExpression: boolean = false, isCombo: boolean = false) { | ||
|
|
||
| if (clauseValue) { | ||
| const cleanValue = clauseValue.replace(/[`"\[\]]/g, ''); | ||
| const isExpression = checkExpression && REGEX.SYNAPSE_EXPRESSION.test(clauseValue); | ||
| this._setFieldValue(fieldName, cleanValue, isExpression); | ||
| } else { | ||
| this._clearFieldValue(fieldName); // Clear the field if clause not present | ||
| } | ||
| } | ||
|
|
||
| /** Helper: Builds a prepared statement string for SELECT */ | ||
| private _buildSelectPreparedStatement(columnsStr: string, tableName: string, matchedWhereFields: Record<string, DynamicFieldValue>, orderBy?: string, limit?: string, offset?: string): string { | ||
|
|
||
| const selectCols = columnsStr?.trim() || '*'; | ||
| let statement = `SELECT ${selectCols} FROM ${tableName}`; | ||
|
|
||
| const wherePlaceholders = Object.keys(matchedWhereFields) | ||
| .map(key => `${this._encodeColumnName(matchedWhereFields[key].displayName)} = ?`) // Use displayName for column name | ||
| .join(' AND '); | ||
|
|
||
| if (wherePlaceholders) { | ||
| statement += ` WHERE ${wherePlaceholders}`; | ||
| } | ||
|
|
||
| if (orderBy) { | ||
| const cleanOrderBy = orderBy.replace(/[`"\[\]]/g, '').trim(); | ||
| statement += ` ORDER BY ${cleanOrderBy}`; | ||
| } | ||
|
|
||
| if (limit) { statement += ` LIMIT ?`; } | ||
| if (offset) { statement += ` OFFSET ?`; } | ||
|
|
||
| return statement; | ||
| } | ||
|
|
||
| /** Helper: Builds a prepared statement string for INSERT */ | ||
| private _buildInsertPreparedStatement(tableName: string, matchedFields: Record<string, DynamicFieldValue>): string { | ||
| const columns = Object.values(matchedFields).map(f => this._encodeColumnName(f.displayName)).join(', '); | ||
| const placeholders = Object.keys(matchedFields).map(() => '?').join(', '); | ||
| if (!columns) return `INSERT INTO ${tableName} DEFAULT VALUES`; // Handle case with no columns | ||
| return `INSERT INTO ${tableName} (${columns}) VALUES (${placeholders})`; | ||
| } |
There was a problem hiding this comment.
Pass dbType into prepared‑statement builders to keep identifier quoting consistent.
For manually entered queries parsed in _handleManualQueryChange, you correctly derive dbType and an encodedTableName:
const connectionData = await this._getDbConnectionDetails();
const dbType = this._getConnectionDbType(connectionData);
const encodedTableName = this._encodeColumnName(cleanTableName, dbType);
switch (operationType) {
case QUERY_TYPES.SELECT:
preparedStatement = this._buildSelectPreparedStatement(
columns,
encodedTableName,
matchedFields,
orderBy,
limit,
offset,
);
break;
case QUERY_TYPES.INSERT:
preparedStatement = this._buildInsertPreparedStatement(encodedTableName, matchedFields);
break;
case QUERY_TYPES.DELETE:
preparedStatement = this._buildDeletePreparedStatement(encodedTableName, matchedFields);
break;
// ...
}But the helper methods themselves call _encodeColumnName without passing dbType, so they fall back to the "default" quoting rule (double quotes) regardless of the actual database. That’s inconsistent with _buildQueryFromDynamicFields, which always passes dbType, and can be wrong for engines like MySQL that expect backticks by default.
Example: for MySQL you can end up with:
INSERT INTO `my_table` ("id", "name") VALUES (?, ?)instead of using backticks for columns as you do elsewhere.
Consider threading dbType through these helpers:
- private _buildSelectPreparedStatement(
- columnsStr: string,
- tableName: string,
- matchedWhereFields: Record<string, DynamicFieldValue>,
- orderBy?: string,
- limit?: string,
- offset?: string,
- ): string {
+ private _buildSelectPreparedStatement(
+ columnsStr: string,
+ tableName: string,
+ matchedWhereFields: Record<string, DynamicFieldValue>,
+ dbType: string | undefined,
+ orderBy?: string,
+ limit?: string,
+ offset?: string,
+ ): string {
@@
- const wherePlaceholders = Object.keys(matchedWhereFields)
- .map(key => `${this._encodeColumnName(matchedWhereFields[key].displayName)} = ?`)
+ const wherePlaceholders = Object.keys(matchedWhereFields)
+ .map(key => `${this._encodeColumnName(matchedWhereFields[key].displayName, dbType)} = ?`)
.join(' AND ');
@@
- if (orderBy) {
- const cleanOrderBy = orderBy.replace(/[`"\[\]]/g, '').trim();
- statement += ` ORDER BY ${cleanOrderBy}`;
- }
+ if (orderBy) {
+ const cleanOrderBy = orderBy.replace(/[`"\[\]]/g, '').trim();
+ const encodedOrderBy = this._encodeColumnName(cleanOrderBy, dbType);
+ statement += ` ORDER BY ${encodedOrderBy}`;
+ }- private _buildInsertPreparedStatement(tableName: string, matchedFields: Record<string, DynamicFieldValue>): string {
- const columns = Object.values(matchedFields).map(f => this._encodeColumnName(f.displayName)).join(', ');
+ private _buildInsertPreparedStatement(
+ tableName: string,
+ matchedFields: Record<string, DynamicFieldValue>,
+ dbType: string | undefined,
+ ): string {
+ const columns = Object.values(matchedFields)
+ .map(f => this._encodeColumnName(f.displayName, dbType))
+ .join(', ');- private _buildDeletePreparedStatement(tableName: string, matchedWhereFields: Record<string, DynamicFieldValue>): string {
+ private _buildDeletePreparedStatement(
+ tableName: string,
+ matchedWhereFields: Record<string, DynamicFieldValue>,
+ dbType: string | undefined,
+ ): string {
@@
- const wherePlaceholders = Object.keys(matchedWhereFields)
- .map(key => `${this._encodeColumnName(matchedWhereFields[key].displayName)} = ?`)
+ const wherePlaceholders = Object.keys(matchedWhereFields)
+ .map(key => `${this._encodeColumnName(matchedWhereFields[key].displayName, dbType)} = ?`)
.join(' AND ');And update the call‑sites in _handleManualQueryChange:
- case QUERY_TYPES.SELECT:
- preparedStatement = this._buildSelectPreparedStatement(columns, encodedTableName, matchedFields, orderBy, limit, offset);
+ case QUERY_TYPES.SELECT:
+ preparedStatement = this._buildSelectPreparedStatement(
+ columns,
+ encodedTableName,
+ matchedFields,
+ dbType,
+ orderBy,
+ limit,
+ offset,
+ );
break;
case QUERY_TYPES.INSERT:
- preparedStatement = this._buildInsertPreparedStatement(encodedTableName, matchedFields);
+ preparedStatement = this._buildInsertPreparedStatement(encodedTableName, matchedFields, dbType);
break;
case QUERY_TYPES.DELETE:
- preparedStatement = this._buildDeletePreparedStatement(encodedTableName, matchedFields);
+ preparedStatement = this._buildDeletePreparedStatement(encodedTableName, matchedFields, dbType);
break;This keeps quoting behavior consistent between auto‑built and parsed queries and avoids DB‑specific syntax issues.
🤖 Prompt for AI Agents
In
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
around lines 1351 to 1606, helper methods that build prepared statements call
this._encodeColumnName without the dbType, causing inconsistent identifier
quoting; change the signatures of _buildSelectPreparedStatement,
_buildInsertPreparedStatement, _buildDeletePreparedStatement and
_buildCallPreparedStatement to accept a dbType parameter, update every internal
call to this._encodeColumnName to pass that dbType, and update the call sites in
_handleManualQueryChange to forward the dbType (derived from
_getConnectionDbType) when invoking these builders so quoting is consistent with
the connection.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
3 participants
$Subject
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.