Add VSCode Extension Support for RDBMS Connector #999
Conversation
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds dynamic-field and driver-management capabilities: new RPC types/endpoints and wiring, a DynamicFieldsHandler for runtime form interactions and SQL construction, 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 LangClient as ExtendedLanguageClient
participant RPC as MiDiagramRpcManager
User->>UI: change connection/driver/field
UI->>Handler: handleValueChange(field, value, element)
Handler->>LangClient: request (getDynamicFields / getStoredProcedures / loadDriverAndTestConnection / downloadDriverForConnector / getDriverMavenCoordinates)
LangClient->>RPC: send synapse/* RPC request
RPC-->>LangClient: RPC response
LangClient-->>Handler: forward response
Handler->>UI: update combos, visibility, queries
UI-->>User: render updated form
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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]>
…amicFieldsHandler.tsx Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (2)
893-918: Fix React Hooks rule violations in helper functions and switch cases.
useEffectis being called from helpers invoked like plain functions (ParamManagerComponent,ExpressionFieldComponent,FormExpressionFieldComponent) and directly insiderenderFormElementswitch cases (combo*,connection). This breaks the Rules of Hooks and will cause Biome errors and potentially inconsistent React state.Refactor these to proper child components/custom hooks and render them via JSX instead of calling them as plain functions, and move the switch‑case effects into those components.
Example for
ParamManagerComponentand its case:-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> - ... - </ComponentCard>; -} +const ParamManagerComponent: React.FC<{ + element: Element; + isRequired: boolean; + helpTipElement: JSX.Element; + field: any; +}> = ({ element, isRequired, helpTipElement, field }) => { + useEffect(() => { + void handleValueChange(field.value, element.name.toString(), element); + }, [element.name, field.value]); + + return ( + <ComponentCard id={`parameterManager-${element.name}`} sx={cardStyle} disbaleHoverEffect> + ... + </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 them as components and render via<ExpressionFieldComponent ... />/<FormExpressionFieldComponent ... />), and extract the'combo*'and'connection'branches into small child components that own theiruseEffectcalls instead of calling hooks insiderenderFormElement.This will resolve the Biome
useHookAtTopLevel/noSwitchDeclarationserrors and restore hook ordering guarantees.Also applies to: 920-968, 970-1031, 1202-1207, 1459-1461
287-295: Avoid saving driver config from staledriverConfigstate.
handleDriverTypeChangemutatesdriverConfigviasetDriverConfigand then immediately callssaveDriverConfig, which readsdriverConfigfrom state. BecausesetStateis async, validations and saves can run against the previous config.renderDriverConfigurationalso passesonSave={saveDriverConfig}so child components can invoke saves with whatever state happened to be current when the closure was created.Refactor to derive a
nextConfigobject synchronously, update state from it, and pass that same object intosaveDriverConfig(and from child components):- 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 (existingGroupId && existingArtifactId && existingVersion && existingDriverPath) { - setDriverConfig(prev => ({ - ...prev, - 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(); + if (option.configType === 'custom') { + if (existingGroupId && existingArtifactId && existingVersion && existingDriverPath) { + nextConfig = { + ...nextConfig, + groupId: existingGroupId, + artifactId: existingArtifactId, + version: existingVersion, + driverPath: existingDriverPath, + }; + } + } else { + if (existingGroupId && existingArtifactId && existingVersion) { + 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 { // 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)) { setDriverError("All Maven coordinates are required"); return; } @@ - 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); }And in
renderDriverConfigurationpass the current config through theonSaveclosures:- <DefaultDriverConfig + <DefaultDriverConfig config={driverConfig} isReadOnly={true} onConfigChange={setDriverConfig} - onSave={saveDriverConfig} + onSave={() => saveDriverConfig(driverConfig)} /> @@ - <CustomDriverConfig + <CustomDriverConfig config={driverConfig} onConfigChange={setDriverConfig} - onSave={saveDriverConfig} + onSave={() => saveDriverConfig(driverConfig)} @@ - <MavenDriverConfig + <MavenDriverConfig config={driverConfig} onConfigChange={setDriverConfig} - onSave={saveDriverConfig} + onSave={() => saveDriverConfig(driverConfig)}This keeps validation, downloads, and form persistence consistent with the actual config the user sees.
Also applies to: 296-327, 483-527, 548-587, 645-684, 1608-1650
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (3)
355-399: Correct unreachableupdatedOffline && updatedOnlinecondition insetElementVisibility.For
TABLE_NAME/ORDER_BY,updatedOfflineandupdatedOnlineare set in mutually exclusive branches based onenableCondition[0].queryType, so theif (updatedOffline && updatedOnline)can never be true and the function always falls throughfalsefor those names.Use OR to indicate “found and updated in at least one mode”:
- if (updatedOffline && updatedOnline) { + if (updatedOffline || updatedOnline) { return true; }This makes the return value meaningful if any of the special‑cased elements were toggled.
595-700: Stop connection validation when DB driver download ultimately fails.After the bounded retry in
_getValidConnectionDetails, you still append aDRIVER_PATHparameter and callloadDriverAndTestConnectioneven whenisDriverDownloadedis false anddriverPathis falsy. That produces an extra failing RPC and a less clear UX.Short‑circuit when the driver cannot be obtained:
- if (!isDriverDownloaded) { - this.setCustomError(getNameForController(FIELD_NAMES.CONFIG_KEY), "Failed to download the DB driver after 5 attempts."); - } - connection.parameters.push({ + if (!isDriverDownloaded) { + 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, }); @@ - const testResult = await this.rpcClient.getMiDiagramRpcClient().loadDriverAndTestConnection(testArgs); + const testResult = await this.rpcClient.getMiDiagramRpcClient().loadDriverAndTestConnection(testArgs);This avoids testing a connection with a known‑bad/undefined driver and cleanly falls back to offline mode.
913-1151: PassdbTypeinto prepared‑statement builders to keep identifier quoting consistent.For auto‑built queries in
_buildQueryFromDynamicFieldsyou already passdbTypeinto_encodeColumnName, but for manually entered queries parsed in_handleManualQueryChangethe prepared‑statement helpers don’t receivedbTypeand call_encodeColumnNamewithout it. That can generate incorrect quoting for non‑default dialects (e.g., MySQL prefers backticks), producing SQL that parses differently from the dynamic‑field path.Thread
dbTypethrough the builders and use it for column/order‑by quoting:- 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 ');- private _buildCallPreparedStatement(tableName: string, matchedFields: Record<string, DynamicFieldValue>): string { + private _buildCallPreparedStatement( + tableName: string, + matchedFields: Record<string, DynamicFieldValue>, + dbType: string | undefined, + ): string { const placeholders = Object.keys(matchedFields).map(() => '?').join(', '); - return `CALL ${tableName}(${placeholders})`; + return `CALL ${tableName}(${placeholders})`; }And in
_handleManualQueryChange:- const connectionData = await this._getDbConnectionDetails(); - const dbType = this._getConnectionDbType(connectionData); + const connectionData = await this._getDbConnectionDetails(); + const dbType = this._getConnectionDbType(connectionData); @@ - 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; case QUERY_TYPES.CALL: - preparedStatement = this._buildCallPreparedStatement(encodedTableName, matchedFields); + preparedStatement = this._buildCallPreparedStatement(encodedTableName, matchedFields, dbType); break;This aligns manual‑query parsing with the db‑aware encoding already used in
_buildQueryFromDynamicFields.Also applies to: 1575-1628, 1355-1368
🧹 Nitpick comments (1)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (1)
271-281: Consider removing unused local state helpers to reduce noise.
setElements/updateElementsandisDriverConfigLoadingare currently written but not used elsewhere in this component. Unless you plan to wire them in a follow‑up (e.g., passingupdateElementsintoDynamicFieldsHandler), consider deleting them to keep the FormGenerator surface lean.- const [, setElements] = useState(formData.elements); - const updateElements = (newElements: any) => { - setElements(newElements); - formData.elements = newElements; - }; @@ - const [isDriverConfigLoading, setIsDriverConfigLoading] = useState(true); + // Remove isDriverConfigLoading until you actually surface a loading indicatorIf these are placeholders for upcoming behavior, a short comment/TODO would help clarify the intent.
Also applies to: 287-295
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx(1 hunks)workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx(22 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 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/**/*.{ts,tsx} : Define all constants (node types, sizing, spacing) in src/resources/constants.ts and import them where needed instead of hardcoding values
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-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-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-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/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-1640)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)
🪛 Biome (2.1.2)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx
[error] 894-895: 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] 922-923: 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] 973-974: 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] 1202-1203: 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] 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: 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] 1459-1460: 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] 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 (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (1)
197-221: Wrapswitchcase bodies that declare variables in blocks (BiomenoSwitchDeclarations).Several
switchcases declareconst/letdirectly in the case body without block scoping. Biome'snoSwitchDeclarationsrule flags these because variable bindings are visible for the entire switch block but only initialized when their case runs, creating temporal-dead-zone issues and enabling unintended cross-case access.The
buildQuerycase and cases in_buildQueryFromDynamicFields(QUERY_TYPES.INSERT,DELETE,SELECT,CALL) and_handleManualQueryChangeneed wrapping in{ ... }blocks:- 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 affected cases at lines 1023-1151 and 1247-1275.
| /** Handles the specific logic for 'handleDynamicContent' onChange */ | ||
| private async _handleDynamicContentChange(value: any, fieldName: string, element: Element): Promise<void> { | ||
| const newFields = await this.fetchDynamicFields(element, value, fieldName); | ||
| // Update dynamic fields state only if fetch was successful (newFields is not null) | ||
| if (newFields !== null && newFields !== undefined) { | ||
| //let augmentedFields = newFields; | ||
| if (element.onValueChange?.params?.[0]?.onDynamicFieldChange.params?.[0]?.queryType === QUERY_TYPES.SELECT) { | ||
| const augmentedFields: typeof newFields = []; | ||
| newFields.forEach((field: any) => { | ||
| // Original field | ||
| augmentedFields.push(field); | ||
| // Extra checkbox field for "Include in Response" | ||
| augmentedFields.push({ | ||
| ...field, | ||
| value: { | ||
| ...field.value, | ||
| name: `${field.value.name}_include`, | ||
| columnName: field.value.displayName, | ||
| helpTip: `Check to include the column '${field.value.displayName}' in the SELECT response.`, | ||
| displayName: field.value.displayName.charAt(0).toUpperCase() + field.value.displayName.slice(1), | ||
| inputType: "checkbox", | ||
| defaultValue: false, | ||
| parentField: fieldName, | ||
| }, | ||
| }); | ||
| }); | ||
| this.setDynamicFields({ | ||
| ...this.dynamicFields, [fieldName]: { | ||
| header: "Table Columns", | ||
| fields: augmentedFields, | ||
| }, | ||
| }); | ||
| } else if (element.onValueChange?.params?.[0]?.onDynamicFieldChange.params?.[0]?.queryType === QUERY_TYPES.CALL) { | ||
| this.setDynamicFields({ | ||
| ...this.dynamicFields, [fieldName]: { | ||
| header: "Parameters", | ||
| fields: newFields, | ||
| }, | ||
| }); | ||
| } else { | ||
| this.setDynamicFields({ | ||
| ...this.dynamicFields, [fieldName]: { | ||
| header: "Table Columns", | ||
| fields: newFields, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd workspaces/mi/mi-diagram && head -n 450 src/components/Form/DynamicFields/DynamicFieldsHandler.tsx | tail -n 50Repository: wso2/vscode-extensions
Length of output: 188
🏁 Script executed:
cd workspaces/mi/mi-diagram && cat -n src/components/Form/DynamicFields/DynamicFieldsHandler.tsx | sed -n '400,455p'Repository: wso2/vscode-extensions
Length of output: 188
🌐 Web query:
TypeScript optional chaining semantics short-circuit undefined property access
💡 Result:
Short answer
- The optional chaining operator (?.) checks only the value immediately to its left; if that value is null or undefined the whole optional access short‑circuits and yields undefined — and any expressions to the right (including side effects like x++) are not evaluated. [1][2]
Important details
- It only tests for nullish (null or undefined), not other falsy values (0, "", false). [1][2]
- Short‑circuiting applies along a continuous chain (a?.b?.c). If you break the chain with parentheses (a?.b).c, the parenthesized result may be undefined and the subsequent access will still run and can throw. [1][2]
- Optional call and element access follow the same rule: fn?.() and arr?.[i] evaluate the left operand first and return undefined if it’s nullish; they do not prevent evaluation of unrelated expressions outside the optional chain. [1][2]
Minimal examples
- Side effect skipped:
const o = null; let i = 0; o?.[i++]; // i remains 0. [1] - Grouping breaks short‑circuit:
(a?.b).c // can throw if a is nullish because .c runs on undefined. [1][2]
Sources
- MDN: Optional chaining. [1]
- TypeScript release notes (Optional Chaining). [2]
Fix optional‑chaining on onDynamicFieldChange to avoid runtime TypeError.
In _handleDynamicContentChange, the checks for queryType use:
element.onValueChange?.params?.[0]?.onDynamicFieldChange.params?.[0]?.queryTypeIf onDynamicFieldChange is undefined, the .params access will still throw despite the earlier ?.. This breaks when onValueChange is configured without onDynamicFieldChange.
Guard the access fully with ?.:
- if (element.onValueChange?.params?.[0]?.onDynamicFieldChange.params?.[0]?.queryType === QUERY_TYPES.SELECT) {
+ if (element.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]?.queryType === QUERY_TYPES.SELECT) {
@@
- } else if (element.onValueChange?.params?.[0]?.onDynamicFieldChange.params?.[0]?.queryType === QUERY_TYPES.CALL) {
+ } else if (element.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]?.queryType === QUERY_TYPES.CALL) {This keeps the handler safe when dynamic‑field wiring is partially configured.
🤖 Prompt for AI Agents
In
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx
around lines 403 to 449, the code accesses
element.onValueChange?.params?.[0]?.onDynamicFieldChange.params?.[0]?.queryType
which will throw if onDynamicFieldChange is undefined because .params is
accessed without optional chaining; update those checks to fully guard the chain
(e.g.
element.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0]?.queryType)
in both the SELECT and CALL branches (and any other similar occurrences in this
block) so the handler safely handles partially configured onValueChange wiring
and avoids runtime TypeError.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
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-diagram/src/components/Form/FormGenerator.tsx (1)
1198-1251: CRITICAL: Remove hook from switch case and wrap declarations in block.Three issues:
CRITICAL:
useEffectat lines 1200-1202 is called conditionally inside a switch case, violating the Rules of Hooks.MAJOR: Variable declarations
itemsandallowItemCreateat lines 1198-1199 can leak to other switch cases.MINOR: The
useEffectdependency array is missingname, which the past review flagged (line 1202).Apply these fixes:
- Move the hook to component top level or extract into a small component:
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]]); // run on mount and on props.comboValuesMap return ( <> <AutoComplete name={name} label={element.displayName} labelAdornment={helpTipElement} errorMsg={errorMsg} items={items} value={field.value} onValueChange={(e: any) => { field.onChange(e); handleValueChange(e, name, element); }} required={isRequired} allowItemCreate={allowItemCreate} /> {dynamicFields[name]?.fields?.length > 0 && ( <> {/* ... dynamic fields rendering ... */} </> )} </> ); + }
- For the value change on mount, consider calling
handleValueChangedirectly when items change, or use a top-leveluseEffectwith proper conditions:useEffect(() => { const items = props.comboValuesMap?.[someFieldName] || []; if (items.length > 0) { const fieldValue = getValues(someFieldName); handleValueChange(fieldValue, someFieldName, /* element */); } }, [props.comboValuesMap]);Alternatively, extract the combo rendering into a separate component that owns its
useEffect.Based on learnings, past review and Biome flag these violations.
♻️ Duplicate comments (7)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (7)
889-914: CRITICAL: Fix React hooks rule violation in ParamManagerComponent.
ParamManagerComponentcontains auseEffecthook (lines 890-892) but is invoked as a plain function at line 1367 rather than rendered as a React component. This violates the Rules of Hooks and can cause state inconsistencies.Convert it to a proper component that takes props and render it via JSX:
-function ParamManagerComponent(element: Element, isRequired: boolean, helpTipElement: JSX.Element, field: any) { +function ParamManagerComponent(props: { + element: Element; + isRequired: boolean; + helpTipElement: JSX.Element; + field: any; +}) { + const { element, isRequired, helpTipElement, field } = props; useEffect(() => { handleValueChange(field.value, element.name.toString(), element); }, []); // run only on mount return <ComponentCard id={'parameterManager-' + element.name} sx={cardStyle} disbaleHoverEffect> {/* ... rest unchanged ... */} </ComponentCard>; }Then at line 1367:
case 'ParamManager': { return ( - ParamManagerComponent(element, isRequired, helpTipElement, field) + <ParamManagerComponent + element={element} + isRequired={isRequired} + helpTipElement={helpTipElement} + field={field} + /> ); }Apply the same pattern to
ExpressionFieldComponentandFormExpressionFieldComponent. See past review comment (lines 271-282) for complete fix.Based on learnings, past review comment provides detailed solution.
916-964: CRITICAL: Fix React hooks rule violation in ExpressionFieldComponent.
ExpressionFieldComponentcontains auseEffecthook (lines 918-921) but is invoked as a plain function at line 1185. This violates the Rules of Hooks.Convert to a proper component with props object and render via JSX as described in the past review comment (lines 271-282). The same pattern applies as
ParamManagerComponent.Based on learnings, past review comment flags this violation.
966-1027: CRITICAL: Fix React hooks rule violation in FormExpressionFieldComponent.
FormExpressionFieldComponentcontains auseEffecthook (lines 969-971) but is invoked as a plain function at line 1194. This violates the Rules of Hooks.Convert to a proper component as described in the past review comment (lines 271-282).
However, the custom error handling integration at lines 968 and 991 is well done! The prioritization of custom errors over standard errors is correct.
Based on learnings, past review comment flags this violation.
544-582: Update saveDriverConfig to accept config parameter.This function reads from the
driverConfigstate, which can be stale when called immediately aftersetDriverConfig. Update the signature to accept an optional config parameter as suggested in the past review (lines 271-283).Apply this diff:
- 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)) { setDriverError("All Maven coordinates are required"); return; } //check if valid coordinates - 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 }); // ... rest of validation } // Save to form values - 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); }Based on learnings, past review comment provides detailed fix.
1455-1457: CRITICAL: Remove hook from switch case.
useEffectat lines 1455-1457 is called conditionally inside the'connection'switch case, violating the Rules of Hooks.Move this logic to component top level or extract into a separate component. The past review comment (line 1457) also notes missing dependencies:
field.value,name, andelementshould be included in the dependency array if kept as a useEffect.Wrap the case in a block and remove the hook as shown in the previous comment for the combo case.
Based on learnings, past review and Biome flag this violation.
290-290: Remove unused state variable.The
isDriverConfigLoadingvariable is declared but never used to control rendering or logic flow.Apply this diff:
- const [isDriverConfigLoading, setIsDriverConfigLoading] = useState(true); + const [, setIsDriverConfigLoading] = useState(true);Alternatively, if you intend to use this for loading states, integrate it into the driver configuration rendering logic.
479-523: Fix stale state usage and logic error.Two issues:
Stale state issue (MAJOR):
handleDriverTypeChangecallssetDriverConfigmultiple times (lines 483, 500-506, 510-516) then immediately callssaveDriverConfig()at line 521. SincesetDriverConfigis asynchronous,saveDriverConfigwill read the previousdriverConfigstate, not the updated values. This can cause validation errors or incorrect values being saved.Logic error (MINOR): Line 517 has
} else if (existingDriverPath)which appears correct, but the past review flagged inconsistent spacing. Verify the logic flow is as intended.For the stale state issue, apply the fix suggested in the past review comment (lines 271-283): compute
nextConfigsynchronously, update state with it, and pass it tosaveDriverConfig(nextConfig). The past review provides a detailed diff.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); + + let nextConfig: DriverConfig = { + ...driverConfig, + type: option.configType + }; if (option.configType === 'default') { handleClearDriver(); await loadDefaultDriverDetails(); } else { const paramValues = parameters?.paramValues || []; const getParamValue = (key: string) => paramValues.find((p: any) => p.key === key)?.value; const existingGroupId = getParamValue('groupId'); const existingArtifactId = getParamValue('artifactId'); const existingVersion = getParamValue('version'); const existingDriverPath = getParamValue('driverPath'); if (option.configType === 'custom') { if (existingGroupId && existingArtifactId && existingVersion && existingDriverPath) { - setDriverConfig(prev => ({ - ...prev, + nextConfig = { + ...nextConfig, groupId: existingGroupId, artifactId: existingArtifactId, version: existingVersion, driverPath: existingDriverPath - })); + }; } } else { if (existingGroupId && existingArtifactId && existingVersion) { - setDriverConfig(prev => ({ - ...prev, + nextConfig = { + ...nextConfig, groupId: existingGroupId, artifactId: existingArtifactId, version: existingVersion, driverPath: null - })); + }; } else if (existingDriverPath) { setValue("driverPath", null); } } - saveDriverConfig(); + setDriverConfig(nextConfig); + await saveDriverConfig(nextConfig); } };Also update
saveDriverConfigsignature to accept an optional config parameter as shown in the past review.Based on learnings, past review comment flags this exact issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx(22 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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.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.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
🪛 Biome (2.1.2)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx
[error] 890-891: 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] 918-919: 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] 969-970: 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] 1198-1199: 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] 1199-1200: 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] 1200-1201: 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] 1455-1456: 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)
🔇 Additional comments (6)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (6)
292-323: LGTM!The driver configuration initialization logic correctly reads from both parameters and form values, determines the appropriate driver type, and handles errors appropriately.
1033-1036: LGTM! Good error handling pattern.The custom error prioritization is correctly implemented and consistent throughout the component.
1604-1646: LGTM! Well-structured radio button implementation.The radio button handling is well-implemented:
- Driver-specific radio (
driverSelectOption) correctly integrates withhandleDriverTypeChangeand renders configuration UI- Generic radio buttons use the same
GenericRadioGroupcomponent consistently- Conditional value change propagation (lines 1640-1642) correctly checks for
element.onValueChangebefore calling the handler
465-470: LGTM! Clean value change propagation.The
handleValueChangefunction provides a clean integration point for dynamic field updates with appropriate null checking and async handling.
641-680: LGTM! Clean driver configuration rendering.The
renderDriverConfigurationfunction cleanly separates the three driver configuration types (default, custom, Maven) into dedicated components with appropriate props and error handling.
160-177: LGTM! Well-defined interfaces for dynamic fields.The
DynamicFieldandDynamicFieldGroupinterfaces provide clear type definitions for the dynamic fields feature, including all necessary metadata for rendering.
| useEffect(() => { | ||
| try { | ||
| if (!dynamicFieldsHandler.current) { | ||
| dynamicFieldsHandler.current = new DynamicFieldsHandler({ | ||
| rpcClient, | ||
| formData, | ||
| getValues, | ||
| setValue, | ||
| setComboValues, | ||
| documentUri: props.documentUri, | ||
| parameters, | ||
| dynamicFields, | ||
| setDynamicFields, | ||
| connectionName, | ||
| setCustomError | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error initializing dynamicFieldsHandler:", error); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Consider adding dependencies to avoid stale closures.
The DynamicFieldsHandler is initialized once on mount with an empty dependency array. If props like parameters, setComboValues, or connectionName change after mount, the handler will continue using stale references, potentially causing bugs.
Either add all captured props to the dependency array, or consider whether this handler needs to be recreated on prop changes:
- }, []);
+ }, [parameters, setComboValues, connectionName, documentUri]);Alternatively, if recreation is expensive, consider using refs for values that might change or restructure to avoid closures over props.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| try { | |
| if (!dynamicFieldsHandler.current) { | |
| dynamicFieldsHandler.current = new DynamicFieldsHandler({ | |
| rpcClient, | |
| formData, | |
| getValues, | |
| setValue, | |
| setComboValues, | |
| documentUri: props.documentUri, | |
| parameters, | |
| dynamicFields, | |
| setDynamicFields, | |
| connectionName, | |
| setCustomError | |
| }); | |
| } | |
| } catch (error) { | |
| console.error("Error initializing dynamicFieldsHandler:", error); | |
| } | |
| }, []); | |
| useEffect(() => { | |
| try { | |
| if (!dynamicFieldsHandler.current) { | |
| dynamicFieldsHandler.current = new DynamicFieldsHandler({ | |
| rpcClient, | |
| formData, | |
| getValues, | |
| setValue, | |
| setComboValues, | |
| documentUri: props.documentUri, | |
| parameters, | |
| dynamicFields, | |
| setDynamicFields, | |
| connectionName, | |
| setCustomError | |
| }); | |
| } | |
| } catch (error) { | |
| console.error("Error initializing dynamicFieldsHandler:", error); | |
| } | |
| }, [parameters, setComboValues, connectionName, documentUri]); |
🤖 Prompt for AI Agents
In workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx around lines
413 to 433, the DynamicFieldsHandler is created inside a useEffect with an empty
dependency array which can cause captured props (rpcClient, formData, getValues,
setValue, setComboValues, props.documentUri, parameters, dynamicFields,
setDynamicFields, connectionName, setCustomError) to become stale; fix by either
adding those dependencies to the useEffect dependency array so the handler is
recreated when they change, or keep the empty array but move any changing values
into stable refs (e.g., create refs for parameters, connectionName,
setComboValues, etc.) and read from those refs inside the handler so it always
uses current values without re-instantiating the handler.
…amicFieldsHandler.tsx Co-authored-by: Copilot <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…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)
197-227: Wrap switch case bodies that declare variables in blocks (BiomenoSwitchDeclarations).Cases at Line 214 (
buildQuery), Line 1025 (INSERT), Line 1069 (SELECT), Line 1114 (CALL), and Lines 1249–1274 declareconst/letdirectly undercase. This matches Biome’snoSwitchDeclarationserrors and can let variables bleed across cases in JS.Wrap those case bodies in blocks to scope declarations and satisfy linting:
@@ - switch (config.function) { + switch (config.function) { @@ - 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; + }@@ - switch (operationType) { - case QUERY_TYPES.INSERT: - const insertColumns = Object.values(activeFields); + switch (operationType) { + case QUERY_TYPES.INSERT: { + const insertColumns = Object.values(activeFields); @@ - } else { + } else { query = `INSERT INTO ${encodedTableName} () VALUES ()`; // Or handle as error? preparedStatement = `INSERT INTO ${encodedTableName} () VALUES ()`; } - break; + break; + } - case QUERY_TYPES.DELETE: + case QUERY_TYPES.DELETE: { query = `DELETE FROM ${encodedTableName}`; @@ - } - - break; - - case QUERY_TYPES.SELECT: + } + + break; + } + + case QUERY_TYPES.SELECT: { @@ - // Append ORDER BY, LIMIT, OFFSET for SELECT - const orderByVal = this.getValues(getNameForController(FIELD_NAMES.ORDER_BY)); - const limitVal = this.getValues(getNameForController(FIELD_NAMES.LIMIT)); - const offsetVal = this.getValues(getNameForController(FIELD_NAMES.OFFSET)); + // Append ORDER BY, LIMIT, OFFSET for SELECT + const orderByVal = this.getValues(getNameForController(FIELD_NAMES.ORDER_BY)); + const limitVal = this.getValues(getNameForController(FIELD_NAMES.LIMIT)); + const offsetVal = this.getValues(getNameForController(FIELD_NAMES.OFFSET)); @@ - } - - break; - - case QUERY_TYPES.CALL: - - let callTemplate; + } + + break; + } + + case QUERY_TYPES.CALL: { + + let callTemplate; @@ - const callParams = Object.values(allFields).map(f => { + const callParams = Object.values(allFields).map(f => { @@ - const prepParams = Object.values(allFields).map(f => '?').join(', '); + const prepParams = Object.values(allFields).map(f => '?').join(', '); @@ - columnTypes.push(...Object.values(allFields).map(f => f.columnType ?? 'UNKNOWN')); - - break; + columnTypes.push(...Object.values(allFields).map(f => f.columnType ?? 'UNKNOWN')); + + break; + } }- switch (operationType) { - case QUERY_TYPES.SELECT: - if(columns.trim() !== '*') { - const selectResult = this._parseSelectColumns(columns, dynamicFieldDefs, connectionInfo !== undefined); - if (!selectResult.success) throw new Error(selectResult.errorMessage || ERROR_MESSAGES.FIELD_NOT_FOUND); - Object.assign(selectMatchedFields, selectResult.fields); - } - - const whereResult = this._parseWhereClause(whereClause, dynamicFieldDefs, connectionInfo !== undefined); - if (!whereResult.success) throw new Error(whereResult.errorMessage || ERROR_MESSAGES.FIELD_NOT_FOUND); - Object.assign(matchedFields, whereResult.fields); - break; - case QUERY_TYPES.DELETE: - const deleteWhereResult = this._parseWhereClause(whereClause, dynamicFieldDefs, connectionInfo !== undefined); - if (!deleteWhereResult.success) throw new Error(deleteWhereResult.errorMessage || ERROR_MESSAGES.FIELD_NOT_FOUND); - Object.assign(matchedFields, deleteWhereResult.fields); - break; - case QUERY_TYPES.INSERT: - const insertResult = this._parseInsertValues(columns, values, dynamicFieldDefs, connectionInfo !== undefined); - if (!insertResult.success) throw new Error(insertResult.errorMessage || ERROR_MESSAGES.INSERT_MISMATCH); - Object.assign(matchedFields, insertResult.fields); - break; - case QUERY_TYPES.CALL: - const callResult = this._parseCallParams(values, dynamicFieldDefs, connectionInfo !== undefined); - if (!callResult.success) throw new Error(callResult.errorMessage || ERROR_MESSAGES.FIELD_NOT_FOUND); - Object.assign(matchedFields, callResult.fields); - break; - } + switch (operationType) { + case QUERY_TYPES.SELECT: { + if (columns.trim() !== '*') { + const selectResult = this._parseSelectColumns( + columns, + dynamicFieldDefs, + connectionInfo !== undefined, + ); + if (!selectResult.success) { + throw new Error(selectResult.errorMessage || ERROR_MESSAGES.FIELD_NOT_FOUND); + } + Object.assign(selectMatchedFields, selectResult.fields); + } + + const whereResult = this._parseWhereClause( + whereClause, + dynamicFieldDefs, + connectionInfo !== undefined, + ); + if (!whereResult.success) { + throw new Error(whereResult.errorMessage || ERROR_MESSAGES.FIELD_NOT_FOUND); + } + Object.assign(matchedFields, whereResult.fields); + break; + } + case QUERY_TYPES.DELETE: { + const deleteWhereResult = this._parseWhereClause( + whereClause, + dynamicFieldDefs, + connectionInfo !== undefined, + ); + if (!deleteWhereResult.success) { + throw new Error(deleteWhereResult.errorMessage || ERROR_MESSAGES.FIELD_NOT_FOUND); + } + Object.assign(matchedFields, deleteWhereResult.fields); + break; + } + case QUERY_TYPES.INSERT: { + const insertResult = this._parseInsertValues( + columns, + values, + dynamicFieldDefs, + connectionInfo !== undefined, + ); + if (!insertResult.success) { + throw new Error(insertResult.errorMessage || ERROR_MESSAGES.INSERT_MISMATCH); + } + Object.assign(matchedFields, insertResult.fields); + break; + } + case QUERY_TYPES.CALL: { + const callResult = this._parseCallParams( + values, + dynamicFieldDefs, + connectionInfo !== undefined, + ); + if (!callResult.success) { + throw new Error(callResult.errorMessage || ERROR_MESSAGES.FIELD_NOT_FOUND); + } + Object.assign(matchedFields, callResult.fields); + break; + } + }This should clear the Biome
noSwitchDeclarationserrors. Based on static analysis hints (Biome).Also applies to: 1023-1151, 1248-1275
355-399: UnreachableupdatedOffline && updatedOnlinecondition insetElementVisibility.At Line 383,
if (updatedOffline && updatedOnline)can never be true becauseupdatedOfflineandupdatedOnlineare initialized tofalsefor each element and only one of the mutually exclusivequeryTypechecks can pass per element. The method still mutates visibility correctly, but the return path is effectively dead.If the intent is to return
trueas soon as either offline or online element is updated, use||instead, or drop the flags and always returntrueonce the matching element is processed:- if (updatedOffline && updatedOnline) { - return true; - } + if (updatedOffline || updatedOnline) { + return true; + }Please double-check the desired behavior (especially if there can be separate online/offline variants of the same field) before applying.
403-481: Fix optional chaining ononDynamicFieldChangeto avoid runtime TypeError.In
_handleDynamicContentChange, Line 409 and Line 435 accessonDynamicFieldChange.paramswithout optional chaining. IfonDynamicFieldChangeis undefined, this will throw despite earlier?.usage ononValueChange.Refactor to cache the config and use
?.onparams:- const newFields = await this.fetchDynamicFields(element, value, fieldName); - // Update dynamic fields state only if fetch was successful (newFields is not null) - if (newFields !== null && newFields !== undefined) { - //let augmentedFields = newFields; - if (element.onValueChange?.params?.[0]?.onDynamicFieldChange.params?.[0]?.queryType === QUERY_TYPES.SELECT) { + const newFields = await this.fetchDynamicFields(element, value, fieldName); + // Update dynamic fields state only if fetch was successful (newFields is not null) + if (newFields !== null && newFields !== undefined) { + const onDynamicFieldChangeConfig = element.onValueChange?.params?.[0]?.onDynamicFieldChange; + const queryType = onDynamicFieldChangeConfig?.params?.[0]?.queryType; + + if (queryType === QUERY_TYPES.SELECT) { @@ - this.setDynamicFields({ - ...this.dynamicFields, [fieldName]: { - header: "Table Columns", - fields: augmentedFields, - }, - }); - } else if (element.onValueChange?.params?.[0]?.onDynamicFieldChange.params?.[0]?.queryType === QUERY_TYPES.CALL) { - this.setDynamicFields({ - ...this.dynamicFields, [fieldName]: { - header: "Parameters", - fields: newFields, - }, - }); - } else { - this.setDynamicFields({ - ...this.dynamicFields, [fieldName]: { - header: "Table Columns", - fields: newFields, - }, - }); - } + this.setDynamicFields({ + ...this.dynamicFields, + [fieldName]: { + header: "Table Columns", + fields: augmentedFields, + }, + }); + } else if (queryType === QUERY_TYPES.CALL) { + this.setDynamicFields({ + ...this.dynamicFields, + [fieldName]: { + header: "Parameters", + fields: newFields, + }, + }); + } else { + this.setDynamicFields({ + ...this.dynamicFields, + [fieldName]: { + header: "Table Columns", + fields: newFields, + }, + }); + } @@ - const dummyElement = { + const dummyElement = { @@ - onValueChange: element.onValueChange?.params?.[0]?.onDynamicFieldChange, + onValueChange: onDynamicFieldChangeConfig,This keeps the handler safe even when
onDynamicFieldChangeis not wired.
595-700: Stop connection validation when DB driver cannot be obtained.In
_getValidConnectionDetailsyou now bound the retry loop, which is good, but:
isDriverDownloadedstarts asfalseeven when adriverPathis already configured.- If download ultimately fails, you still push a
DRIVER_PATHparameter with a falsy value and callloadDriverAndTestConnection, causing a guaranteed failing RPC and unclear UX.You can treat an existing
driverPathas already “downloaded” and bail out early when no path is available after retries:- let isDriverDownloaded = false; + let isDriverDownloaded = !!driverPath; let retryCount = 0; const maxRetries = 5; if (!driverPath) { while (!isDriverDownloaded && retryCount < maxRetries) { @@ 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 (!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 a redundant failing connection test and cleanly falls back to offline mode when the driver isn’t available.
693-699: Align CONFIG_KEY error field with other paths in RPC catch.In the catch block of
_getValidConnectionDetails(Line 695) you callsetCustomErrorwithFIELD_NAMES.CONFIG_KEYdirectly, whereas every other path usesgetNameForController(FIELD_NAMES.CONFIG_KEY). IfsetCustomErrorexpects the controller name, this branch may not attach the error to the right field.Align it with the rest:
} catch (error) { console.error("Error testing DB connection:", error); - 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.setValue(FIELD_NAMES.ASSISTANCE_MODE, false); this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.OFFLINE); return undefined; }This keeps error targeting consistent for all connection-validation failures.
1354-1367: ThreaddbTypeinto prepared-statement builders to keep identifier quoting consistent.In
_handleManualQueryChange, you correctly computedbTypeandencodedTableName, but_buildSelectPreparedStatement,_buildInsertPreparedStatement, and_buildDeletePreparedStatementcall_encodeColumnNamewithout passingdbType, so they always use the default quoting style. This can produce inconsistent SQL compared to_buildQueryFromDynamicFields(e.g., double quotes for MySQL columns instead of backticks).Update the helper signatures and call sites to pass
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; - } + switch (operationType) { + case QUERY_TYPES.SELECT: + preparedStatement = this._buildSelectPreparedStatement( + columns, + encodedTableName, + matchedFields, + dbType, + orderBy, + limit, + offset, + ); + break; + case QUERY_TYPES.INSERT: + preparedStatement = this._buildInsertPreparedStatement( + encodedTableName, + matchedFields, + dbType, + ); + break; + case QUERY_TYPES.DELETE: + preparedStatement = this._buildDeletePreparedStatement( + encodedTableName, + matchedFields, + dbType, + ); + break; + case QUERY_TYPES.CALL: + preparedStatement = this._buildCallPreparedStatement(encodedTableName, matchedFields); + break; + }- /** 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 '); + /** Helper: Builds a prepared statement string for SELECT */ + private _buildSelectPreparedStatement( + columnsStr: string, + tableName: string, + matchedWhereFields: Record<string, DynamicFieldValue>, + dbType: string | undefined, + 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, + dbType, + )} = ?`, + ) // Use displayName for column name + .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}`; + }- /** 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(', '); + /** Helper: Builds a prepared statement string for INSERT */ + 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(', ');- /** Helper: Builds a prepared statement string for DELETE */ - private _buildDeletePreparedStatement(tableName: string, matchedWhereFields: Record<string, DynamicFieldValue>): string { - let statement = `DELETE FROM ${tableName}`; // tableName should be encoded already - - const wherePlaceholders = Object.keys(matchedWhereFields) - .map(key => `${this._encodeColumnName(matchedWhereFields[key].displayName)} = ?`) - .join(' AND '); + /** Helper: Builds a prepared statement string for DELETE */ + private _buildDeletePreparedStatement( + tableName: string, + matchedWhereFields: Record<string, DynamicFieldValue>, + dbType: string | undefined, + ): string { + let statement = `DELETE FROM ${tableName}`; // tableName should be encoded already + + const wherePlaceholders = Object.keys(matchedWhereFields) + .map( + key => + `${this._encodeColumnName( + matchedWhereFields[key].displayName, + dbType, + )} = ?`, + ) + .join(' AND ');This keeps quoting consistent for parsed manual queries across different DBs (MySQL, PostgreSQL, SQL Server, etc.).
Also applies to: 1575-1621
🧹 Nitpick comments (2)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (2)
197-207: Guard TABLE_NAME branch inhandleValueChangewhenqueryBuildConfigis missing.In the
'handleDynamicContent'case at Line 200, you now use optional chaining forqueryBuildConfig, but_buildQueryFromDynamicFieldsis still called even ifqueryBuildConfigisundefined, which will passundefinedasoperationTypeand can lead to surprising behavior (no-op switch, fields reset).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); + 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 queryType in onDynamicFieldChange config.`, + ); + } } else {This aligns the behavior with
onDynamicFieldChange, which already validatesqueryBuildConfigbefore using it.
156-158: Remove duplicate'stringOrExpression'entry fromEXPRESSION_TYPES.
EXPRESSION_TYPEScurrently includes'stringOrExpression'twice (Line 156 and again at Line 157–158). That’s harmless but redundant and can confuse future readers.You can deduplicate and format for readability:
-const EXPRESSION_TYPES = ['stringOrExpression', 'integerOrExpression', 'expression', 'keyOrExpression', 'resourceOrExpression', - 'textOrExpression', 'textAreaOrExpression', 'stringOrExpression' -]; +const EXPRESSION_TYPES = [ + 'stringOrExpression', + 'integerOrExpression', + 'expression', + 'keyOrExpression', + 'resourceOrExpression', + 'textOrExpression', + 'textAreaOrExpression', +];
📜 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 (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/**/*.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-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/**/*.{ts,tsx} : Define all constants (node types, sizing, spacing) in src/resources/constants.ts and import them where needed instead of hardcoding values
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
📚 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 (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-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] 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 (1)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (1)
162-191: Overall DynamicFieldsHandler structure looks solid.Constructor wiring, prop shape (
DynamicFieldsHandlerProps), and the public methods (handleValueChange,fetchDynamicFields,onConnectionChange,onDynamicFieldChange) form a coherent API around the form generator and RPC layer. Error handling and separation between online/offline modes are well thought through.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (11)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx (7)
693-699: UsegetNameForControllerconsistently when setting CONFIG_KEY error on RPC failure.Elsewhere,
setCustomErrorusesgetNameForController(FIELD_NAMES.CONFIG_KEY), but the RPC error path uses the raw field name. Aligning this avoids any mismatch if controller naming changes.- console.error("Error testing DB connection:", error); - this.setCustomError(FIELD_NAMES.CONFIG_KEY, ERROR_MESSAGES.CONNECTION_FAILED + " (RPC Error)"); + console.error("Error testing DB connection:", error); + this.setCustomError( + getNameForController(FIELD_NAMES.CONFIG_KEY), + ERROR_MESSAGES.CONNECTION_FAILED + " (RPC Error)", + );
213-220: WrapbuildQuerycase body in a block to satisfy switch scoping (Biome noSwitchDeclarations).Declaring
const parentFielddirectly in thecase 'buildQuery'clause leaks its scope to other cases and triggers Biome’snoSwitchDeclarationserror. Wrap the case body in a block.- 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; + }
361-385: Fix unreachableupdatedOffline && updatedOnlinecondition insetElementVisibility.
updatedOfflineandupdatedOnlineare set in mutually exclusive branches (offline vs online enableCondition), soupdatedOffline && updatedOnlinecan never be true; the function will never returntruefor TABLE_NAME / ORDER_BY.- if (updatedOffline && updatedOnline) { + if (updatedOffline || updatedOnline) { return true; }
403-449: Fully guardonDynamicFieldChangeaccess with optional chaining to avoid runtime TypeError.In
_handleDynamicContentChange, both SELECT and CALL checks access.paramsononDynamicFieldChangewithout?.. IfonDynamicFieldChangeis undefined, this will throw.- if (element.onValueChange?.params?.[0]?.onDynamicFieldChange.params?.[0]?.queryType === QUERY_TYPES.SELECT) { + if ( + element.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0] + ?.queryType === QUERY_TYPES.SELECT + ) { @@ - } else if (element.onValueChange?.params?.[0]?.onDynamicFieldChange.params?.[0]?.queryType === QUERY_TYPES.CALL) { + } else if ( + element.onValueChange?.params?.[0]?.onDynamicFieldChange?.params?.[0] + ?.queryType === QUERY_TYPES.CALL + ) {
631-691: Correct driver download logic and bail early when download fails (avoid false errors and bad test calls).Two issues in
_getValidConnectionDetails:
- If
driverPathalready exists,isDriverDownloadedstaysfalse, so you still set “Failed to download…” even though no download was attempted.- When download retries fail, you still push an
undefineddriverPath and callloadDriverAndTestConnection, causing an unnecessary failing RPC.- let isDriverDownloaded = false; + let isDriverDownloaded = !!driverPath; let retryCount = 0; const maxRetries = 5; - if (!driverPath) { + if (!driverPath) { while (!isDriverDownloaded && retryCount < maxRetries) { @@ retryCount++; } } - if (!isDriverDownloaded) { - this.setCustomError(getNameForController(FIELD_NAMES.CONFIG_KEY), "Failed to download the DB driver after 5 attempts."); - } - connection.parameters.push({ + if (!driverPath || !isDriverDownloaded) { + 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, }); @@ if (!testResult.success) { this.setCustomError(getNameForController(FIELD_NAMES.CONFIG_KEY), ERROR_MESSAGES.CONNECTION_FAILED + (testResult.message ? `: ${testResult.message}` : '')); - this.setValue(FIELD_NAMES.ASSISTANCE_MODE, false); - this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.OFFLINE); + this.setValue(FIELD_NAMES.ASSISTANCE_MODE, false); + this.setValue(FIELD_NAMES.QUERY_TYPE, UI_MODES.OFFLINE);
1353-1365: ThreaddbTypeinto prepared‑statement builders to keep identifier quoting consistent across DBs.In
_handleManualQueryChange, you derivedbTypeandencodedTableName, but_buildSelectPreparedStatement,_buildInsertPreparedStatement,_buildDeletePreparedStatement, and_buildCallPreparedStatementinternally call_encodeColumnNamewithoutdbType, so they fall back to the “default” quoting rule. This can generate inconsistent SQL for engines like MySQL vs PostgreSQL.- switch (operationType) { - case QUERY_TYPES.SELECT: - preparedStatement = this._buildSelectPreparedStatement(columns, encodedTableName, matchedFields, orderBy, limit, offset); + switch (operationType) { + 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; case QUERY_TYPES.CALL: - preparedStatement = this._buildCallPreparedStatement(encodedTableName, matchedFields); + preparedStatement = this._buildCallPreparedStatement( + encodedTableName, + matchedFields, + dbType, + ); break; }- 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 ');- private _buildCallPreparedStatement(tableName: string, matchedFields: Record<string, DynamicFieldValue>): string { + private _buildCallPreparedStatement( + tableName: string, + matchedFields: Record<string, DynamicFieldValue>, + _dbType: string | undefined, + ): string { const placeholders = Object.keys(matchedFields).map(() => '?').join(', '); return `CALL ${tableName}(${placeholders})`; }Also applies to: 1573-1625
1023-1151: Block‑scopeswitch (operationType)cases in_buildQueryFromDynamicFieldsto satisfy Biome and avoid declaration bleed.
constdeclarations in the INSERT, SELECT, and CALL cases (insertColumns,orderByVal,limitVal,offsetVal,callTemplate,callParams,prepParams) are currently in barecasebodies, triggeringnoSwitchDeclarationsand risking accidental reuse.- switch (operationType) { - case QUERY_TYPES.INSERT: - const insertColumns = Object.values(activeFields); + switch (operationType) { + case QUERY_TYPES.INSERT: { + const insertColumns = Object.values(activeFields); @@ - } - - break; - - case QUERY_TYPES.DELETE: + } + + break; + } + + case QUERY_TYPES.DELETE: { @@ - } - - break; - - case QUERY_TYPES.SELECT: + } + + break; + } + + case QUERY_TYPES.SELECT: { @@ - const orderByVal = this.getValues(getNameForController(FIELD_NAMES.ORDER_BY)); - const limitVal = this.getValues(getNameForController(FIELD_NAMES.LIMIT)); - const offsetVal = this.getValues(getNameForController(FIELD_NAMES.OFFSET)); + const orderByVal = this.getValues(getNameForController(FIELD_NAMES.ORDER_BY)); + const limitVal = this.getValues(getNameForController(FIELD_NAMES.LIMIT)); + const offsetVal = this.getValues(getNameForController(FIELD_NAMES.OFFSET)); @@ - break; - - case QUERY_TYPES.CALL: - - let callTemplate; + break; + } + + case QUERY_TYPES.CALL: { + + let callTemplate; @@ - const callParams = Object.values(allFields).map(f => { + const callParams = Object.values(allFields).map(f => { @@ - const prepParams = Object.values(allFields).map(f => '?').join(', '); + const prepParams = Object.values(allFields).map(f => '?').join(', '); @@ - break; + break; + } }workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (4)
412-432: Reconsider empty dependency array when constructingDynamicFieldsHandler(stale props risk).
DynamicFieldsHandleris created once withuseEffect(..., []), but it capturesparameters,setComboValues,connectionName, anddocumentUri. If any of those props change after mount, the handler will continue using stale references.Consider either:
- Adding dependencies so the handler is re‑created when inputs change:
- useEffect(() => { + useEffect(() => { try { if (!dynamicFieldsHandler.current) { dynamicFieldsHandler.current = new DynamicFieldsHandler({ rpcClient, formData, getValues, setValue, setComboValues, documentUri: props.documentUri, parameters, dynamicFields, setDynamicFields, connectionName, setCustomError }); } } catch (error) { console.error("Error initializing dynamicFieldsHandler:", error); } - }, []); + }, [rpcClient, formData, getValues, setValue, setComboValues, props.documentUri, parameters, connectionName]);or
- Keeping the empty array but moving changing values into
useRefs and passing those refs intoDynamicFieldsHandlerso it always reads current values.
889-913: Fix React Hooks rule violations in nested helpers (ParamManagerComponent,ExpressionFieldComponent,FormExpressionFieldComponent).These helpers call
useEffectbut are invoked as plain functions (e.g.ParamManagerComponent(...)), not as React components. This breaks the Rules of Hooks and is flagged by Biome.Refactor them into proper components used via JSX. For example:
- 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> + function ParamManagerComponent(props: { + element: Element; + isRequired: boolean; + helpTipElement: JSX.Element; + field: any; + handleValueChange: (value: any, fieldName: string, element: Element) => Promise<void>; + documentUri?: string; + range?: Range; + }) { + const { element, isRequired, helpTipElement, field, handleValueChange, documentUri, range } = props; + useEffect(() => { + handleValueChange(field.value, element.name.toString(), element); + }, [field.value, element, handleValueChange]); + return <ComponentCard id={'parameterManager-' + element.name} sx={cardStyle} disbaleHoverEffect> @@ - <ParameterManager - documentUri={documentUri} + <ParameterManager + documentUri={documentUri} @@ - } + } @@ - case 'ParamManager': { - return ( - ParamManagerComponent(element, isRequired, helpTipElement, field) - ); - } + case 'ParamManager': { + return ( + <ParamManagerComponent + element={element} + isRequired={isRequired} + helpTipElement={helpTipElement} + field={field} + handleValueChange={handleValueChange} + documentUri={documentUri} + range={range} + /> + ); + }Apply the same pattern to
ExpressionFieldComponentandFormExpressionFieldComponent: define them as components taking a props object and render them as<ExpressionFieldComponent ... />/<FormExpressionFieldComponent ... />inside the switch instead of calling them like regular functions.Also applies to: 915-971, 965-1002
271-290: Avoid staledriverConfigwhen saving; pass the next config intosaveDriverConfig.
handleDriverTypeChangemutatesdriverConfigviasetDriverConfigand then immediately callssaveDriverConfig, butsaveDriverConfigreadsdriverConfigfrom state. BecausesetStateis async,saveDriverConfigcan run with the previous config, causing validation and saved values to disagree with the UI.A safer pattern is to compute
nextConfigsynchronously and pass it intosaveDriverConfig:- 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 === 'default') { - handleClearDriver(); - await loadDefaultDriverDetails(); + handleClearDriver(); + await loadDefaultDriverDetails(); } else { @@ - 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, + }; + } + 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); }Update
onSaveprops to callsaveDriverConfig(driverConfig)where needed.Also applies to: 478-521, 543-581
1197-1202: Remove or relocateuseEffectcalls from insiderenderFormElementswitch cases (combo*andconnection).The
useEffectinvocations in the'booleanOrExpression' / 'comboOrExpression' / 'combo'and'connection'cases run conditionally inside a switch and are not at the top level of a component, violating the Rules of Hooks (BiomeuseHookAtTopLevel).Two viable approaches:
- Move effects into dedicated child components:
Create small components (e.g.
ComboFieldWithDynamicandConnectionField) that own their ownuseEffectand render the existing JSX, then use them from the switch:- case 'booleanOrExpression': - case 'comboOrExpression': - case 'combo': - const items = ... - const allowItemCreate = ... - useEffect(() => { - handleValueChange(field.value, name, element); - }, [props.comboValuesMap?.[name]]); - return ( - <> - <AutoComplete ... /> - {...dynamic fields...} - </> - ); + case 'booleanOrExpression': + case 'comboOrExpression': + case 'combo': + return ( + <ComboFieldWithDynamic + name={name} + element={element} + field={field} + helpTipElement={helpTipElement} + errorMsg={errorMsg} + isRequired={isRequired} + comboValuesMap={props.comboValuesMap} + dynamicFields={dynamicFields} + handleValueChange={handleValueChange} + /> + );Inside
ComboFieldWithDynamic, calluseEffectat its top level.Do the same for the
'connection'case (wrap in aConnectionFieldcomponent and move itsuseEffectthere).
- If the mount‑time side‑effects are not essential, drop the
useEffectentirely and rely solely ononValueChangehandlers to triggerhandleValueChange.Given the linter errors, option 1 is safer if you rely on the initialization behavior.
Also applies to: 1454-1456
🧹 Nitpick comments (1)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (1)
271-290: Consider removing unusedisDriverConfigLoadingstate.
isDriverConfigLoadingis set but never read, which suggests leftover wiring from an earlier spinner UX.If you don’t plan to show a loading indicator, drop this state to reduce noise:
- const [isDriverConfigLoading, setIsDriverConfigLoading] = useState(true); + // Removed unused isDriverConfigLoading state; reintroduce if a loading UI is added.and remove the related
setIsDriverConfigLoadingcalls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workspaces/mi/mi-diagram/src/components/Form/DynamicFields/DynamicFieldsHandler.tsx(1 hunks)workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx(22 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/**/*.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.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 (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] 889-890: 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] 917-918: 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] 968-969: 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] 1197-1198: 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] 1198-1199: 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] 1199-1200: 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] 1454-1455: 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] 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 (1)
workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (1)
965-971: SurfacecustomErrorsin legacyExpressionFieldComponentas well.
FormExpressionFieldComponentcorrectly prioritizescustomErrors[name]over RHF errors, butExpressionFieldComponentstill uses onlyerrors[name]. For fields using legacy expression mode, custom errors fromDynamicFieldsHandler(e.g., parse/connection issues) won't be shown.Mirror the
FormExpressionFieldComponentpattern inExpressionFieldComponent:const ExpressionFieldComponent = ({ element, canChange, field, helpTipElement, placeholder, isRequired }: { element: Element, canChange: boolean, field: any, helpTipElement: JSX.Element, placeholder: string, isRequired: boolean }) => { const name = getNameForController(element.name); + const customError = customErrors[name]; useEffect(() => { handleValueChange(field.value, name, element); }, []); return expressionEditorField !== name ? ( <ExpressionField {...field} @@ - errorMsg={errors[name] && errors[name].message.toString()} + errorMsg={customError ?? (errors[name] && errors[name].message.toString())}This keeps error display consistent across both expression field variants.
3 participants
$Subject
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.