Revert "Fix datamapper button not appearing"#1895
Revert "Fix datamapper button not appearing"#1895KCSAbeywickrama merged 1 commit intorelease/bi-1.8.xfrom
Conversation
📝 WalkthroughWalkthroughThe changes convert asynchronous type-change handlers to synchronous across multiple form components. Error handling with try/catch blocks and promise-based flows are removed. The Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR reverts the earlier “Fix datamapper button not appearing” change set, aligning BI form/type-selection behavior with the newer fix referenced in #1893 and simplifying type-change handling across the visualizer and side-panel form components.
Changes:
- Simplifies
handleSelectedTypeChangeinFormGenerator(removes async logic/error handling and some visualizable-field fetch paths). - Updates the side-panel
FormAPI to makehandleSelectedTypeChangesynchronous and adjusts callers accordingly. - Adds a
handleOnTypeChangehook in the side-panelFormto refresh visualizable fields when the type field changes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx | Reverts async type-selection flow; removes some visualizable-field fetching paths and try/catch handling. |
| workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/DeclareVariableForm/index.tsx | Updates type-change forwarding to match the now-synchronous handleSelectedTypeChange. |
| workspaces/ballerina/ballerina-side-panel/src/components/Form/index.tsx | Changes handleSelectedTypeChange type signature and wires handleOnTypeChange to refresh visualizable fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Promise.resolve(handleSelectedTypeChange?.(type)).catch((error) => { | ||
| console.error("Error in handleSelectedTypeChange", error); | ||
| }); | ||
| handleSelectedTypeChange(type); |
There was a problem hiding this comment.
handleSelectedTypeChange is optional on FormProps, but it’s being called unconditionally here. If a caller doesn’t provide it, this will throw at runtime when the user changes the type. Guard with optional chaining (or a default no-op) similar to the previous implementation, and keep error handling if the callback can fail.
| handleSelectedTypeChange(type); | |
| handleSelectedTypeChange?.(type); |
| console.error("Error handling selected type change", error); | ||
| const handleSelectedTypeChange = (type: CompletionItem | string) => { | ||
| if (typeof type === "string") { | ||
| handleSelectedTypeByName(type); |
There was a problem hiding this comment.
handleSelectedTypeByName is async, but handleSelectedTypeChange now calls it without awaiting or handling rejections. This can lead to unhandled promise rejections (and out-of-order state updates) when type resolution fails or the RPC call throws. Consider making handleSelectedTypeChange async and awaiting the call, or explicitly catching/logging the promise.
| handleSelectedTypeByName(type); | |
| void handleSelectedTypeByName(type).catch((error) => { | |
| console.error("Failed to handle selected type by name:", error); | |
| }); |
| const handleSelectedTypeByName = async (typeName: string) => { | ||
| // Early return for invalid input | ||
| if (!typeName || typeName.length === 0) { | ||
| importsCodedataRef.current = null; | ||
| await fetchVisualizableFields(fileName, typeName); | ||
| setValueTypeConstraints(''); | ||
| return; | ||
| } | ||
|
|
||
| const type = await searchImportedTypeByName(typeName); | ||
| if (!type) { | ||
| importsCodedataRef.current = null; | ||
| await fetchVisualizableFields(fileName, typeName); | ||
| setValueTypeConstraints(''); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When typeName is empty/invalid or the type can’t be found, this function now returns without updating/clearing visualizableField (and without resetting importsCodedataRef). Since visualizableField is used to decide whether the Data Mapper button is enabled and to provide default values, it can become stale after a type is cleared/changed. Consider explicitly resetting visualizableField (and importsCodedataRef.current) on these early-return paths, or triggering a refresh that results in a cleared state.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx (1)
1402-1409:handleSelectedTypeChangeconverted to synchronous with reduced side effects.Key changes in this revert:
handleSelectedTypeByName(type)is called withoutawait— this async function now runs fire-and-forget- The
CompletionItembranch no longer clearsimportsCodedataRef.currentor fetches visualizable fieldsThe fire-and-forget pattern for
handleSelectedTypeByNamemeans any errors will be unobserved (no rejection handler). If the async operations in that path need error visibility, consider adding a.catch():♻️ Optional: Add error logging for fire-and-forget async call
const handleSelectedTypeChange = (type: CompletionItem | string) => { if (typeof type === "string") { - handleSelectedTypeByName(type); + handleSelectedTypeByName(type).catch((error) => { + console.error(">>> Error handling type change by name", error); + }); return; } setSelectedType(type); updateRecordTypeFields(type); };Based on learnings, the
CompletionItembranch changes are safe in the current flow and will be revisited after the "unifying efforts of types API calls" work is complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx` around lines 1402 - 1409, In handleSelectedTypeChange, the string branch calls handleSelectedTypeByName(type) fire-and-forget which leaves any errors unobserved and also removed previous side-effects; change that call to handleSelectedTypeByName(type).catch(err => /* log error */) to surface rejections, and restore the prior side-effects by clearing importsCodedataRef.current (set to null/undefined) and invoking the routine that fetches/updates visualizable fields (e.g., fetchVisualizableFields or updateRecordTypeFields) as the original flow did so you still reset importsCodedataRef and populate fields; reference: handleSelectedTypeChange, handleSelectedTypeByName, importsCodedataRef.current, fetchVisualizableFields/updateRecordTypeFields, setSelectedType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/DeclareVariableForm/index.tsx`:
- Around line 35-37: The call to handleSelectedTypeChange inside
handleOnTypeChange can throw because handleSelectedTypeChange is optional in
FormProps; add a defensive null-check or optional chaining before invoking it
(e.g., if (handleSelectedTypeChange) handleSelectedTypeChange(type) or
handleSelectedTypeChange?.(type)) so handleOnTypeChange safely handles the case
when handleSelectedTypeChange is undefined; update the handleOnTypeChange
function to perform this guard using the existing parameter names.
---
Nitpick comments:
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx`:
- Around line 1402-1409: In handleSelectedTypeChange, the string branch calls
handleSelectedTypeByName(type) fire-and-forget which leaves any errors
unobserved and also removed previous side-effects; change that call to
handleSelectedTypeByName(type).catch(err => /* log error */) to surface
rejections, and restore the prior side-effects by clearing
importsCodedataRef.current (set to null/undefined) and invoking the routine that
fetches/updates visualizable fields (e.g., fetchVisualizableFields or
updateRecordTypeFields) as the original flow did so you still reset
importsCodedataRef and populate fields; reference: handleSelectedTypeChange,
handleSelectedTypeByName, importsCodedataRef.current,
fetchVisualizableFields/updateRecordTypeFields, setSelectedType.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7fa3f392-eda9-4413-9432-3cf682e41b10
📒 Files selected for processing (3)
workspaces/ballerina/ballerina-side-panel/src/components/Form/index.tsxworkspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/DeclareVariableForm/index.tsxworkspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/FormGenerator/index.tsx
| const handleOnTypeChange = (type: string | CompletionItem) => { | ||
| Promise.resolve(handleSelectedTypeChange?.(type)).catch((error) => { | ||
| console.error("Error in handleSelectedTypeChange", error); | ||
| }); | ||
| handleSelectedTypeChange(type); | ||
| }; |
There was a problem hiding this comment.
Missing null check after removing optional chaining.
The handleSelectedTypeChange prop is optional (per FormProps), but the revert removed the optional chaining safeguard (?.). If this prop is ever undefined, the call will throw a TypeError.
While FormGenerator always passes this prop when rendering VariableForm, the component interface still allows it to be omitted. Consider adding a guard:
🛡️ Proposed fix to restore defensive check
const handleOnTypeChange = (type: string | CompletionItem) => {
- handleSelectedTypeChange(type);
+ handleSelectedTypeChange?.(type);
};📝 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.
| const handleOnTypeChange = (type: string | CompletionItem) => { | |
| Promise.resolve(handleSelectedTypeChange?.(type)).catch((error) => { | |
| console.error("Error in handleSelectedTypeChange", error); | |
| }); | |
| handleSelectedTypeChange(type); | |
| }; | |
| const handleOnTypeChange = (type: string | CompletionItem) => { | |
| handleSelectedTypeChange?.(type); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/Forms/DeclareVariableForm/index.tsx`
around lines 35 - 37, The call to handleSelectedTypeChange inside
handleOnTypeChange can throw because handleSelectedTypeChange is optional in
FormProps; add a defensive null-check or optional chaining before invoking it
(e.g., if (handleSelectedTypeChange) handleSelectedTypeChange(type) or
handleSelectedTypeChange?.(type)) so handleOnTypeChange safely handles the case
when handleSelectedTypeChange is undefined; update the handleOnTypeChange
function to perform this guard using the existing parameter names.
Reverts #1832
wso2/product-integrator#254 is addressed with #1893
Summary by CodeRabbit
Refactor
Style