Add support for SMB connection and listener#1821
Add support for SMB connection and listener#1821Nuvindu wants to merge 9 commits intowso2:release/bi-1.8.xfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SMB support across the BI codebase: protocol enum, module registration, icons, ServiceDesigner UI and SMB-specific forms, type-editor adjustments, entry-node icon, parameters prop, and end-to-end tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ServiceDesigner
participant SMBConfigForm
participant SMBForm
participant ServiceModel
User->>ServiceDesigner: Click "Add File Handler"
ServiceDesigner->>SMBConfigForm: Open handler selection
User->>SMBConfigForm: Select handler (onCreate/onDelete)
SMBConfigForm->>ServiceDesigner: onSubmit(selectedHandler)
ServiceDesigner->>SMBForm: Open SMB configuration panel with handler
ServiceDesigner->>ServiceModel: Load functions & params
SMBForm->>ServiceModel: Update DATA_BINDING / params / imports
User->>SMBForm: Edit schema / toggle stream / enable params
User->>SMBForm: Click Save
SMBForm->>ServiceDesigner: onSave(functionModel, isNew)
ServiceDesigner->>ServiceModel: Persist changes
ServiceDesigner->>User: Refresh file handlers list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx (1)
198-232: Hardcoded parameter name differs fromFileIntegrationFormpattern.
handleTypeCreatedhardcodes the parameter name to"content"(Line 214), whileFileIntegrationFormusestypeNameToParamName(typeValue, shouldPluralize)to derive a meaningful name from the type. This inconsistency could confuse users who work with both FTP and SMB integrations.🤖 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/ServiceDesigner/Forms/SMBForm/index.tsx` around lines 198 - 232, handleTypeCreated currently hardcodes the parameter name to "content" which differs from FileIntegrationForm's naming; change it to compute the parameter name using the same helper as FileIntegrationForm (typeNameToParamName) based on the selected typeValue and whether streaming is enabled (functionModel.properties.stream?.enabled) before updating the DATA_BINDING parameter, and ensure REQUIRED parameters named the derived name are disabled if present; update references in handleTypeCreated and use the same naming logic as FileIntegrationForm to keep behavior consistent.workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/SMBConfigForm.tsx (1)
26-31: UnusedonBackprop.The
onBackprop is defined in the interface and destructured but never used in the component. Either remove it from the interface or implement the back navigation if needed.♻️ Remove unused prop
interface SMBConfigFormProps { - onBack?: () => void; isSaving: boolean; onSubmit?: (selectedHandler: string) => void; serviceModel: ServiceModel; } export function SMBConfigForm(props: SMBConfigFormProps) { - const { onBack, onSubmit, isSaving, serviceModel } = props; + const { onSubmit, isSaving, serviceModel } = props;Also applies to: 35-35
🤖 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/ServiceDesigner/Forms/SMBForm/SMBConfigForm.tsx` around lines 26 - 31, SMBConfigFormProps declares an unused onBack prop (and it’s destructured in the component), which should be removed or wired up; either delete onBack from the SMBConfigFormProps interface and remove it from the component destructuring, or implement the back navigation by calling the onBack callback from the component’s back button/handler (e.g., where the form’s cancel/back action is handled) so that the onBack prop is actually invoked.workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/index.tsx (2)
272-285: Consider extracting shared handler availability logic.
hasAvailableSMBHandlers()is nearly identical tohasAvailableFTPHandlers()(Lines 255-270). The only difference is that FTP also checks foronError. Consider refactoring to a shared helper that accepts the handler types as parameters.♻️ Proposed refactor
+ // Generalized handler availability check + const hasAvailableHandlers = (handlerTypes: string[]) => { + if (!serviceModel?.functions) return false; + + const deprecatedFunctions = serviceModel.functions.filter(fn => fn.metadata?.label === 'EVENT'); + const hasDeprecatedFunctions = deprecatedFunctions.length > 0 && deprecatedFunctions.some(fn => fn.enabled); + if (hasDeprecatedFunctions) return false; + + return handlerTypes.some(handlerType => { + const functions = serviceModel.functions.filter(fn => fn.metadata?.label === handlerType); + return functions.length > 0 && functions.some(fn => !fn.enabled); + }); + }; + + const hasAvailableFTPHandlers = () => hasAvailableHandlers(['onCreate', 'onDelete', 'onError']); + const hasAvailableSMBHandlers = () => hasAvailableHandlers(['onCreate', 'onDelete']);🤖 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/ServiceDesigner/index.tsx` around lines 272 - 285, hasAvailableSMBHandlers duplicates logic in hasAvailableFTPHandlers; extract a reusable helper (e.g., getHasAvailableHandlers) that accepts the serviceModel.functions array and a list of handler labels to check (e.g., ['onCreate','onDelete'] for SMB and ['onCreate','onDelete','onError'] for FTP) plus the deprecated label 'EVENT'; implement the helper to filter functions by label, compute "available" when any matching handler exists and is not enabled, compute "hasDeprecated" when any 'EVENT' functions are enabled, and return the combined boolean, then replace hasAvailableSMBHandlers and hasAvailableFTPHandlers with calls to this helper (referencing serviceModel.functions, hasAvailableSMBHandlers, hasAvailableFTPHandlers, and the 'EVENT' deprecated label for locating code).
1180-1226: Significant code duplication with FTP service section.The SMB service UI block (Lines 1180-1226) is nearly identical to the FTP service block (Lines 1077-1123). Both render the same "File Handlers" section with identical structure. Consider extracting a shared
FileHandlersSectioncomponent that accepts the handler availability check function as a prop.🤖 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/ServiceDesigner/index.tsx` around lines 1180 - 1226, The SMB block duplicates the FTP block; extract a reusable FileHandlersSection component and replace both SMB and FTP JSX with it: create FileHandlersSection that accepts props like handlers (enabledHandlers), isVisible (isSmbService / isFtpService), hasAvailableHandlers (hasAvailableSMBHandlers / corresponding FTP checker), onAdd (onSelectAddHandler / ftp add handler), onSearch (handleSearch), searchValue, goToSource, onEditResource (handleFunctionEdit), onDeleteResource (handleFunctionDelete), and onResourceImplement (handleOpenDiagram); move the SectionHeader, ActionGroup, conditional TextField, Add button, mapping to ResourceAccordion (preserving key `${index}-${functionModel.name.value}` and method/functionModel props), and EmptyReadmeContainer into that component and use it for both service blocks to eliminate duplication.
🤖 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-core/src/utils/identifier-utils.ts`:
- Line 26: The constant FILE_INTEGRATION_MODULES is inconsistent across modules
(one version includes "smb" while the other in platform-ext utils does not);
update the other definition of FILE_INTEGRATION_MODULES (the same-named constant
in the platform-ext utils file) to include "smb" so both scope resolution
functions classify SMB the same way.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx`:
- Around line 86-97: The useEffect that sets service and function state
(useEffect in SMBForm) reads isNew and nonEnabledFunctions but does not list
isNew in its dependency array, which can lead to stale updates; update the
dependency array to include isNew (alongside model and selectedHandler) so the
effect re-runs when isNew changes, ensuring the branches that call
setServiceModel, setFunctionModel, and setSelectedFileFormat (which reference
nonEnabledFunctions and props.functionModel) run with current values.
- Around line 205-208: The code risks a null reference when reading
functionModel.properties.stream?.enabled; update the usage in the updatedType
creation to safely handle missing properties (e.g. use optional chaining like
functionModel?.properties?.stream?.enabled or otherwise guard that
functionModel.properties exists) so selectType receives a defined boolean or
undefined. Locate the updatedType construction (uses param.type and selectType)
and replace the direct functionModel.properties.stream access with a null-safe
access pattern (functionModel?.properties?.stream?.enabled) or add an earlier
guard that ensures functionModel.properties is defined before using it.
- Around line 117-121: The handleSave currently calls onSave(functionModel,
isNew) which incorrectly uses isNew as the openDiagram flag; update handleSave
to call onSave(functionModel, false) (or omit the second arg) so it passes the
correct openDiagram boolean instead of isNew; modify the function in SMBForm
(handleSave, functionModel, isNew, onSave) to stop using isNew as the second
parameter.
In
`@workspaces/ballerina/component-diagram/src/components/nodes/EntryNode/components/GeneralWidget.tsx`:
- Around line 125-126: Replace the hardcoded "smb" string in the GeneralWidget
component's switch/case with a shared constant: add a new exported constant
(e.g., NODE_TYPE_SMB) to constants.ts, import that constant into
GeneralWidget.tsx, and use it in the case (case NODE_TYPE_SMB: return <Icon
name="bi-smb" />;). Ensure the constant name follows existing node-type naming
conventions and update any other occurrences of the "smb" literal to reference
NODE_TYPE_SMB.
In
`@workspaces/ballerina/type-editor/src/TypeEditor/ContextBasedTypeEditor/GenericImportTab.tsx`:
- Line 557: The intermediate "or" Typography is rendered unconditionally
relative to the surrounding conditional blocks, causing a dangling "or" when the
JSON continuation is hidden for FTP/SMB; update GenericImportTab.tsx so the "or"
is only rendered when at least one of the adjacent follow-up blocks will render
— compute booleans (e.g., jsonContinuationVisible based on
payloadContext?.protocol !== Protocol.FTP && payloadContext?.protocol !==
Protocol.SMB and any other existing JSON conditions, and smbFtpFollowUpVisible
based on payloadContext?.protocol === Protocol.FTP || payloadContext?.protocol
=== Protocol.SMB and the authentication condition) and change the "or" render
to: if (jsonContinuationVisible && smbFtpFollowUpVisible) render the
Typography("or"). This ensures the "or" appears only when both sides exist.
In
`@workspaces/bi/bi-extension/src/test/e2e-playwright-tests/file-integration/smb.spec.ts`:
- Around line 106-107: Replace the hard-coded sleep (await
page.page.waitForTimeout(500)) with an explicit wait for the element to be
hidden: remove the waitForTimeout call and use Playwright's explicit condition
against the serviceTreeItem (e.g., expect(serviceTreeItem).toBeHidden() or
serviceTreeItem.waitForElementState('hidden')) before asserting not.toBeVisible
so the test waits deterministically for the tree item to disappear; update the
test around serviceTreeItem and page.page accordingly.
---
Nitpick comments:
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx`:
- Around line 198-232: handleTypeCreated currently hardcodes the parameter name
to "content" which differs from FileIntegrationForm's naming; change it to
compute the parameter name using the same helper as FileIntegrationForm
(typeNameToParamName) based on the selected typeValue and whether streaming is
enabled (functionModel.properties.stream?.enabled) before updating the
DATA_BINDING parameter, and ensure REQUIRED parameters named the derived name
are disabled if present; update references in handleTypeCreated and use the same
naming logic as FileIntegrationForm to keep behavior consistent.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/SMBConfigForm.tsx`:
- Around line 26-31: SMBConfigFormProps declares an unused onBack prop (and it’s
destructured in the component), which should be removed or wired up; either
delete onBack from the SMBConfigFormProps interface and remove it from the
component destructuring, or implement the back navigation by calling the onBack
callback from the component’s back button/handler (e.g., where the form’s
cancel/back action is handled) so that the onBack prop is actually invoked.
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/index.tsx`:
- Around line 272-285: hasAvailableSMBHandlers duplicates logic in
hasAvailableFTPHandlers; extract a reusable helper (e.g.,
getHasAvailableHandlers) that accepts the serviceModel.functions array and a
list of handler labels to check (e.g., ['onCreate','onDelete'] for SMB and
['onCreate','onDelete','onError'] for FTP) plus the deprecated label 'EVENT';
implement the helper to filter functions by label, compute "available" when any
matching handler exists and is not enabled, compute "hasDeprecated" when any
'EVENT' functions are enabled, and return the combined boolean, then replace
hasAvailableSMBHandlers and hasAvailableFTPHandlers with calls to this helper
(referencing serviceModel.functions, hasAvailableSMBHandlers,
hasAvailableFTPHandlers, and the 'EVENT' deprecated label for locating code).
- Around line 1180-1226: The SMB block duplicates the FTP block; extract a
reusable FileHandlersSection component and replace both SMB and FTP JSX with it:
create FileHandlersSection that accepts props like handlers (enabledHandlers),
isVisible (isSmbService / isFtpService), hasAvailableHandlers
(hasAvailableSMBHandlers / corresponding FTP checker), onAdd (onSelectAddHandler
/ ftp add handler), onSearch (handleSearch), searchValue, goToSource,
onEditResource (handleFunctionEdit), onDeleteResource (handleFunctionDelete),
and onResourceImplement (handleOpenDiagram); move the SectionHeader,
ActionGroup, conditional TextField, Add button, mapping to ResourceAccordion
(preserving key `${index}-${functionModel.name.value}` and method/functionModel
props), and EmptyReadmeContainer into that component and use it for both service
blocks to eliminate duplication.
🪄 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: 7b72a09e-c791-45a4-9f03-402ed6447ac9
⛔ Files ignored due to path filters (1)
workspaces/common-libs/font-wso2-vscode/src/icons/bi-smb.svgis excluded by!**/*.svg
📒 Files selected for processing (14)
workspaces/ballerina/ballerina-core/src/interfaces/service.tsworkspaces/ballerina/ballerina-core/src/utils/identifier-utils.tsworkspaces/ballerina/ballerina-extension/src/utils/project-artifacts.tsworkspaces/ballerina/ballerina-visualizer/src/views/BI/ComponentListView/FileIntegrationPanel.tsxworkspaces/ballerina/ballerina-visualizer/src/views/BI/PackageOverview/utils.tsworkspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/SMBConfigForm.tsxworkspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsxworkspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/index.tsxworkspaces/ballerina/ballerina-visualizer/src/views/BI/TypeEditor/utils.tsxworkspaces/ballerina/component-diagram/src/components/nodes/EntryNode/components/GeneralWidget.tsxworkspaces/ballerina/type-editor/src/TypeEditor/ContextBasedTypeEditor/GenericImportTab.tsxworkspaces/bi/bi-extension/src/test/e2e-playwright-tests/file-integration/SMB_INTEGRATION_TEST_SPEC.mdworkspaces/bi/bi-extension/src/test/e2e-playwright-tests/file-integration/smb.spec.tsworkspaces/bi/bi-extension/src/test/e2e-playwright-tests/test.list.ts
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx
Outdated
Show resolved
Hide resolved
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx
Show resolved
Hide resolved
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx
Show resolved
Hide resolved
...aces/ballerina/component-diagram/src/components/nodes/EntryNode/components/GeneralWidget.tsx
Show resolved
Hide resolved
workspaces/ballerina/type-editor/src/TypeEditor/ContextBasedTypeEditor/GenericImportTab.tsx
Show resolved
Hide resolved
workspaces/bi/bi-extension/src/test/e2e-playwright-tests/file-integration/smb.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/ServiceDesigner/Forms/SMBForm/index.tsx`:
- Around line 175-195: The helper withoutStreamType currently strips a trailing
"[]" before unwrapping "stream<...>", which causes stream<byte[], error> ->
byte[] -> byte; fix by normalizing/unwrapping stream<> first and only after that
remove a trailing "[]" (keeping the string[] special-case). Concretely, inside
withoutStreamType reorder the logic so you check and handle
baseType.startsWith("stream<") and extract the inner type first (handle both ",
error>" and ">" endings), then after that perform the baseType.endsWith("[]") &&
baseType !== "string[]" slice to drop the array suffix; keep existing guards for
empty type and hasStreamProperty unchanged.
- Around line 238-255: The delete-schema handler (handleDeleteContentSchema)
only resets DATA_BINDING parameters; update it to also locate the REQUIRED
parameter named "content" in functionModel.parameters, set its enabled flag to
true, and restore its type.value based on the current stream setting (use
hasStreamProperty and functionModel.properties.stream.enabled with selectType to
pick the correct placeholder/value as you do for DATA_BINDING). Ensure you
modify the same updatedParameters array so the raw content param is re-enabled
and its type matches the stream state after a define→delete cycle.
- Around line 343-350: The Parameters component is being fed only
[contentParameter], and its onChange passes that single-item array straight to
handleParamChange, which overwrites functionModel.parameters and drops
fileInfo/caller; instead, when onChange yields updated params for the content
parameter, merge those updates into the existing functionModel.parameters (e.g.,
map over functionModel.parameters and replace the parameter whose identity
matches contentParameter (by name or id) with the updated content param from
params[0]), and call handleParamChange with the full merged array; keep the
existing deletion path calling handleDeleteContentSchema when params.length ===
0.
- Around line 65-97: Remove the redundant serviceModel state and derive
nonEnabledFunctions directly from the incoming model prop (replace usages of
serviceModel/functions with model/functions), then consolidate the two effects
into a single useEffect that initializes functionModel and selectedFileFormat;
ensure the effect depends on [model, selectedHandler, isNew,
props.functionModel] and uses model to compute nonEnabledFunctions, call
setFunctionModel(...) and setSelectedFileFormat(...) once using either the first
nonEnabledFunctions entry for new items or props.functionModel when !isNew, and
remove setServiceModel and any serviceModel state reads (references:
serviceModel, setServiceModel, nonEnabledFunctions, useEffect, functionModel,
setFunctionModel, selectedFileFormat, setSelectedFileFormat, selectedHandler,
isNew, props.functionModel).
🪄 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: 3a7955fd-a247-4c8d-9327-d64df7f3deb9
📒 Files selected for processing (2)
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/FileIntegrationForm/Parameters/Parameters.tsxworkspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx
✅ Files skipped from review due to trivial changes (1)
- workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/FileIntegrationForm/Parameters/Parameters.tsx
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx
Outdated
Show resolved
Hide resolved
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx
Show resolved
Hide resolved
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx
Show resolved
Hide resolved
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx (5)
65-105: 🛠️ Refactor suggestion | 🟠 MajorRedundant
serviceModelstate and incomplete effect dependencies.The
serviceModelstate duplicates themodelprop.nonEnabledFunctionsis derived fromserviceModelon render, but the first effect updatesserviceModelafter reading the stale-computed list. Additionally, the second effect (lines 99-105) readsisNewandnonEnabledFunctionsbut only depends onselectedHandler.Derive
nonEnabledFunctionsdirectly frommodelprop and consolidate both effects with complete dependencies.♻️ Proposed fix
- const [serviceModel, setServiceModel] = useState<ServiceModel>(model); const [functionModel, setFunctionModel] = useState<FunctionModel | null>(props.functionModel || null); const [selectedFileFormat, setSelectedFileFormat] = useState<string>(''); // ... payloadContext and isTypeEditorOpen ... - const nonEnabledFunctions = serviceModel.functions?.filter(fn => { + const nonEnabledFunctions = model.functions?.filter(fn => { if (!fn.enabled && selectedHandler && fn.metadata?.label === selectedHandler) { return true; } if (!fn.enabled && !selectedHandler && fn.metadata?.label === "onCreate") { return true; } return false; }) || []; useEffect(() => { - setServiceModel(model); if (isNew && nonEnabledFunctions.length > 0) { const initialFunction = nonEnabledFunctions[0]; setFunctionModel(initialFunction); setSelectedFileFormat(initialFunction.name?.metadata?.label || ''); - } - if (!isNew) { + } else if (!isNew) { setFunctionModel(props.functionModel); setSelectedFileFormat(props.functionModel?.name?.metadata?.label || ''); } - }, [model, selectedHandler, isNew]); - - useEffect(() => { - if (isNew && selectedHandler && nonEnabledFunctions.length > 0) { - const initialFunction = nonEnabledFunctions[0]; - setFunctionModel(initialFunction); - setSelectedFileFormat(initialFunction.name?.metadata?.label || ''); - } - }, [selectedHandler]); + }, [model, selectedHandler, isNew, props.functionModel]);🤖 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/ServiceDesigner/Forms/SMBForm/index.tsx` around lines 65 - 105, Remove the redundant serviceModel state and derive nonEnabledFunctions directly from the incoming model prop (replace references to serviceModel with model), then consolidate the two useEffect blocks into a single effect that lists complete dependencies (model, selectedHandler, isNew, props.functionModel) and sets functionModel and selectedFileFormat via setFunctionModel and setSelectedFileFormat appropriately for both new and non-new flows; also remove the separate second effect and ensure any initial selection uses nonEnabledFunctions computed from model.
238-262:⚠️ Potential issue | 🟠 MajorDeleting schema doesn't restore the raw
contentparameter.When a schema is created (lines 219-220), the
REQUIREDcontentparameter is disabled. However,handleDeleteContentSchemaonly resets theDATA_BINDINGparameter. After a define → delete cycle, the handler saves without its raw content argument, and stream toggle updates skip disabled parameters (lines 378-385).💡 Proposed fix
const handleDeleteContentSchema = () => { const updatedParameters = functionModel.parameters.map((p) => { if (p.kind === "DATA_BINDING") { const resetValue = hasStreamProperty && functionModel.properties.stream.enabled ? selectType(p.type.placeholder, functionModel.properties.stream.enabled) : p.type.placeholder; return { ...p, type: { ...p.type, value: resetValue }, enabled: true }; } + if (p.kind === "REQUIRED" && p.name.value === "content") { + const resetValue = hasStreamProperty && functionModel.properties.stream.enabled + ? selectType(p.type.placeholder, functionModel.properties.stream.enabled) + : p.type.placeholder; + + return { + ...p, + enabled: true, + type: { + ...p.type, + value: resetValue + } + }; + } return p; });🤖 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/ServiceDesigner/Forms/SMBForm/index.tsx` around lines 238 - 262, handleDeleteContentSchema only resets parameters with kind "DATA_BINDING", so the REQUIRED "content" parameter remains disabled after deleting a schema; update the parameters mapping in handleDeleteContentSchema to also detect the REQUIRED content parameter (e.g., p.kind === "REQUIRED" && p.name === "content") and restore it by setting enabled: true and resetting p.type.value to the same resetValue logic (use hasStreamProperty && functionModel.properties.stream.enabled ? selectType(p.type.placeholder, functionModel.properties.stream.enabled) : p.type.placeholder); then build updatedFunctionModel and call setFunctionModel as before.
175-196:⚠️ Potential issue | 🟠 Major
withoutStreamTypeproduces inconsistent results for RAW + stream.For RAW format:
withoutStreamType("byte[]")returns"byte", butwithoutStreamType("stream<byte[], error>")returns"byte[]". This inconsistency causes the Line 329 comparison to fail when stream is enabled, incorrectly showing the payload editor instead of the "Define Content Schema" button.Unwrap
stream<>first, then strip the trailing[].💡 Proposed fix
let baseType = typeValue; - if (baseType.endsWith("[]") && baseType !== "string[]") { - baseType = baseType.slice(0, -2); - } else if (baseType.startsWith("stream<")) { + if (baseType.startsWith("stream<")) { if (baseType.endsWith(", error>")) { baseType = baseType.slice(7, -8); } else if (baseType.endsWith(">")) { baseType = baseType.slice(7, -1); } } + if (baseType.endsWith("[]") && baseType !== "string[]") { + baseType = baseType.slice(0, -2); + } return baseType;🤖 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/ServiceDesigner/Forms/SMBForm/index.tsx` around lines 175 - 196, The withoutStreamType function yields inconsistent results because it strips array brackets before unwrapping stream<>; change its logic (in function withoutStreamType) to first detect and unwrap stream<> (handle both "stream<T, error>" and "stream<T>") to extract the inner type, then after unwrapping remove a trailing "[]" (except for "string[]") — keep the early return when !typeValue and when !hasStreamProperty, and preserve existing slicing bounds when extracting the inner type.
343-355:⚠️ Potential issue | 🔴 CriticalEditing content schema drops other handler parameters.
Parametersreceives only[contentParameter], so itsonChangecallback returns a single-item array. Passing this directly tohandleParamChange(line 349) replacesfunctionModel.parameterswholesale, losingfileInfoParameterandcallerParameter.🐛 Proposed fix
<Parameters parameters={[contentParameter]} onChange={(params: ParameterModel[]) => { if (params.length === 0) { handleDeleteContentSchema(); } else { - handleParamChange(params); + const [updatedContentParam] = params; + handleParamChange( + functionModel.parameters.map((param) => + param.kind === "DATA_BINDING" ? updatedContentParam : param + ) + ); } }}🤖 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/ServiceDesigner/Forms/SMBForm/index.tsx` around lines 343 - 355, The Parameters component is being passed only [contentParameter], so its onChange currently forwards a single-item array to handleParamChange and overwrites functionModel.parameters, dropping fileInfoParameter and callerParameter; change the onChange handler to merge the incoming params into the existing functionModel.parameters: when params is empty call handleDeleteContentSchema(), otherwise build newParams by taking functionModel.parameters and replacing (or appending) the contentParameter entry with the updated params[0] (match by parameter id/name), then call handleParamChange(newParams) so other handler parameters (fileInfoParameter, callerParameter) are preserved.
117-121:⚠️ Potential issue | 🟠 MajorIncorrect second parameter passed to
onSave.The
onSavecallback signature is(functionModel: FunctionModel, openDiagram?: boolean). PassingisNewcauses unintended navigation behavior: new handlers will open the flow diagram (whenisNew=true), and edits won't (whenisNew=false).🐛 Proposed fix
const handleSave = () => { if (functionModel) { - onSave(functionModel, isNew); + onSave(functionModel); } };🤖 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/ServiceDesigner/Forms/SMBForm/index.tsx` around lines 117 - 121, handleSave currently calls onSave(functionModel, isNew) which passes the wrong boolean into the onSave(openDiagram?) parameter; change the second argument to the intended openDiagram value (use !isNew if you want edits to open the diagram and new creations to not open it) so update handleSave to call onSave(functionModel, !isNew) (referencing handleSave, onSave, functionModel, and isNew).
🧹 Nitpick comments (1)
workspaces/bi/bi-extension/src/test/e2e-playwright-tests/file-integration/smb.spec.ts (1)
65-90: Assert persisted host value after save.This test currently verifies button state and listener presence, but it does not prove the
hostchange (127.0.0.6) was actually persisted. Please add a post-save assertion by reopening the form (or checking generated config/source) and validating the saved value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/bi/bi-extension/src/test/e2e-playwright-tests/file-integration/smb.spec.ts` around lines 65 - 90, After saving, reopen the listener edit form and assert the host field contains "127.0.0.6": click the existing edit button (editBtn) or the listener entry (artifactWebView.locator(`text=${listenerName}`)) to reopen the form, wait for the form to appear, locate the host cmEditor field (the form field with name 'host' / type 'cmEditor') and assert its value equals "127.0.0.6" (or inspect the generated config/source panel if that’s how persistence is exposed); ensure you use the same locators (saveChangesBtn, backBtn, editBtn, artifactWebView) so the new check runs after the save and navigation steps.
🤖 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/ServiceDesigner/Forms/SMBForm/index.tsx`:
- Around line 69-72: The construction of payloadContext may throw when
functionModel.name exists but name.metadata is undefined; update the filterType
expression to safely access nested fields (e.g., use
functionModel?.name?.metadata?.label || "JSON") or otherwise check name and
metadata before using label so payloadContext (typed as GeneralPayloadContext)
always gets a defined string; locate the payloadContext variable and change the
filterType accessor to include the extra optional chaining or conditional guard
for name.metadata.label.
In
`@workspaces/bi/bi-extension/src/test/e2e-playwright-tests/file-integration/smb.spec.ts`:
- Around line 25-30: The suite is stateful (listenerName is set in the "Create
SMB Integration" test and consumed by "Editing SMB Service") so enable serial
execution on the test.describe block to prevent test-level parallelism and flaky
retries; update the test.describe options for the SMB Integration Tests to
include serial: true so the tests (including Create SMB Integration and Editing
SMB Service) run in order without parallel retries interfering with listenerName
state.
---
Duplicate comments:
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx`:
- Around line 65-105: Remove the redundant serviceModel state and derive
nonEnabledFunctions directly from the incoming model prop (replace references to
serviceModel with model), then consolidate the two useEffect blocks into a
single effect that lists complete dependencies (model, selectedHandler, isNew,
props.functionModel) and sets functionModel and selectedFileFormat via
setFunctionModel and setSelectedFileFormat appropriately for both new and
non-new flows; also remove the separate second effect and ensure any initial
selection uses nonEnabledFunctions computed from model.
- Around line 238-262: handleDeleteContentSchema only resets parameters with
kind "DATA_BINDING", so the REQUIRED "content" parameter remains disabled after
deleting a schema; update the parameters mapping in handleDeleteContentSchema to
also detect the REQUIRED content parameter (e.g., p.kind === "REQUIRED" &&
p.name === "content") and restore it by setting enabled: true and resetting
p.type.value to the same resetValue logic (use hasStreamProperty &&
functionModel.properties.stream.enabled ? selectType(p.type.placeholder,
functionModel.properties.stream.enabled) : p.type.placeholder); then build
updatedFunctionModel and call setFunctionModel as before.
- Around line 175-196: The withoutStreamType function yields inconsistent
results because it strips array brackets before unwrapping stream<>; change its
logic (in function withoutStreamType) to first detect and unwrap stream<>
(handle both "stream<T, error>" and "stream<T>") to extract the inner type, then
after unwrapping remove a trailing "[]" (except for "string[]") — keep the early
return when !typeValue and when !hasStreamProperty, and preserve existing
slicing bounds when extracting the inner type.
- Around line 343-355: The Parameters component is being passed only
[contentParameter], so its onChange currently forwards a single-item array to
handleParamChange and overwrites functionModel.parameters, dropping
fileInfoParameter and callerParameter; change the onChange handler to merge the
incoming params into the existing functionModel.parameters: when params is empty
call handleDeleteContentSchema(), otherwise build newParams by taking
functionModel.parameters and replacing (or appending) the contentParameter entry
with the updated params[0] (match by parameter id/name), then call
handleParamChange(newParams) so other handler parameters (fileInfoParameter,
callerParameter) are preserved.
- Around line 117-121: handleSave currently calls onSave(functionModel, isNew)
which passes the wrong boolean into the onSave(openDiagram?) parameter; change
the second argument to the intended openDiagram value (use !isNew if you want
edits to open the diagram and new creations to not open it) so update handleSave
to call onSave(functionModel, !isNew) (referencing handleSave, onSave,
functionModel, and isNew).
---
Nitpick comments:
In
`@workspaces/bi/bi-extension/src/test/e2e-playwright-tests/file-integration/smb.spec.ts`:
- Around line 65-90: After saving, reopen the listener edit form and assert the
host field contains "127.0.0.6": click the existing edit button (editBtn) or the
listener entry (artifactWebView.locator(`text=${listenerName}`)) to reopen the
form, wait for the form to appear, locate the host cmEditor field (the form
field with name 'host' / type 'cmEditor') and assert its value equals
"127.0.0.6" (or inspect the generated config/source panel if that’s how
persistence is exposed); ensure you use the same locators (saveChangesBtn,
backBtn, editBtn, artifactWebView) so the new check runs after the save and
navigation steps.
🪄 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: 0ca75981-76be-4122-8b6c-69baf4b9ca0c
📒 Files selected for processing (4)
workspaces/ballerina/ballerina-core/src/rpc-types/platform-ext/utils.tsworkspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsxworkspaces/ballerina/type-editor/src/TypeEditor/ContextBasedTypeEditor/GenericImportTab.tsxworkspaces/bi/bi-extension/src/test/e2e-playwright-tests/file-integration/smb.spec.ts
✅ Files skipped from review due to trivial changes (1)
- workspaces/ballerina/ballerina-core/src/rpc-types/platform-ext/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- workspaces/ballerina/type-editor/src/TypeEditor/ContextBasedTypeEditor/GenericImportTab.tsx
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx
Show resolved
Hide resolved
workspaces/bi/bi-extension/src/test/e2e-playwright-tests/file-integration/smb.spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (3)
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx (3)
175-195:⚠️ Potential issue | 🟠 Major
withoutStreamTypestill misclassifiesstream<byte[], error>asbyte.Because the helper strips
[]before/inside stream handling, RAW+stream can normalize incorrectly and flip the schema UI branch (Line 342 comparison).💡 Suggested fix
- if (baseType.endsWith("[]") && baseType !== "string[]") { - baseType = baseType.slice(0, -2); - } else if (baseType.startsWith("stream<")) { + if (baseType.startsWith("stream<")) { if (baseType.endsWith(", error>")) { baseType = baseType.slice(7, -8); } else if (baseType.endsWith(">")) { baseType = baseType.slice(7, -1); } - if (baseType.endsWith("[]") && baseType !== "string[]") { - baseType = baseType.slice(0, -2); - } + } + if (baseType.endsWith("[]") && baseType !== "string[]") { + baseType = baseType.slice(0, -2); }🤖 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/ServiceDesigner/Forms/SMBForm/index.tsx` around lines 175 - 195, The withoutStreamType function misclassifies stream<byte[], error> as byte because array suffixes are stripped before/while handling stream<>; update withoutStreamType so you first detect and unwrap stream<...> to get the exact inner type (e.g., preserve any trailing [] inside the stream inner type) and only then strip a top-level [] suffix (with the same string[] exception) — specifically adjust the stream-handling branch inside withoutStreamType to extract the inner content between "stream<" and the matching ">" (handling ", error>" case) and then apply the slice(0, -2) array-trimming only once on that extracted inner type.
65-105:⚠️ Potential issue | 🟠 MajorStale initialization path due duplicated
serviceModelstate and split effects.
nonEnabledFunctionsis computed fromserviceModel, butLine 87updates that state asynchronously, soLine 88/Line 100can still use stale function lists. Also, the first effect readsprops.functionModel(Line 94-95) but does not depend on it.💡 Suggested fix
- const [serviceModel, setServiceModel] = useState<ServiceModel>(model); const [functionModel, setFunctionModel] = useState<FunctionModel | null>(props.functionModel || null); const [selectedFileFormat, setSelectedFileFormat] = useState<string>(''); @@ - const nonEnabledFunctions = serviceModel.functions?.filter(fn => { + const nonEnabledFunctions = model.functions?.filter(fn => { @@ - useEffect(() => { - setServiceModel(model); - if (isNew && nonEnabledFunctions.length > 0) { - const initialFunction = nonEnabledFunctions[0]; - setFunctionModel(initialFunction); - setSelectedFileFormat(initialFunction.name?.metadata?.label || ''); - } - if (!isNew) { - setFunctionModel(props.functionModel); - setSelectedFileFormat(props.functionModel?.name?.metadata?.label || ''); - } - }, [model, selectedHandler, isNew]); - - useEffect(() => { - if (isNew && selectedHandler && nonEnabledFunctions.length > 0) { + useEffect(() => { + if (isNew && nonEnabledFunctions.length > 0) { const initialFunction = nonEnabledFunctions[0]; setFunctionModel(initialFunction); setSelectedFileFormat(initialFunction.name?.metadata?.label || ''); + } else if (!isNew) { + setFunctionModel(props.functionModel); + setSelectedFileFormat(props.functionModel?.name?.metadata?.label || ''); } - }, [selectedHandler]); + }, [model, selectedHandler, isNew, props.functionModel]);🤖 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/ServiceDesigner/Forms/SMBForm/index.tsx` around lines 65 - 105, The bug is stale computation of nonEnabledFunctions because it derives from mutable state serviceModel while effects update serviceModel asynchronously and effects also read props.functionModel without depending on it; fix by either removing the duplicated serviceModel state and using the incoming model directly for computations, or move the nonEnabledFunctions calculation inside the effects so it always derives from the latest model variable, and add props.functionModel to the dependency arrays; consolidate the two useEffect hooks into one that depends on [model, selectedHandler, isNew, props.functionModel] and within that effect compute nonEnabledFunctions from model (or serviceModel immediately after setServiceModel) before calling setFunctionModel/setSelectedFileFormat to ensure consistent, non-stale initialization.
117-120:⚠️ Potential issue | 🟠 Major
onSaveis still receivingisNewin theopenDiagramslot.At
Line 119,isNewis passed as the second argument, butonSaveexpectsopenDiagram?: boolean. Inworkspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/index.tsx:694-730, that flag controls navigation behavior after save.💡 Suggested fix
const handleSave = () => { if (functionModel) { - onSave(functionModel, isNew); + onSave(functionModel); } };🤖 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/ServiceDesigner/Forms/SMBForm/index.tsx` around lines 117 - 120, The save handler is passing the creation flag isNew as the second argument to onSave (which expects openDiagram?: boolean); update handleSave to pass the correct navigation flag (openDiagram) or omit the second argument if navigation shouldn't occur — i.e., change the call in handleSave from onSave(functionModel, isNew) to onSave(functionModel, openDiagram) (or simply onSave(functionModel) if you intend no navigation), referencing the handleSave and onSave symbols so the correct prop is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsx`:
- Around line 175-195: The withoutStreamType function misclassifies
stream<byte[], error> as byte because array suffixes are stripped before/while
handling stream<>; update withoutStreamType so you first detect and unwrap
stream<...> to get the exact inner type (e.g., preserve any trailing [] inside
the stream inner type) and only then strip a top-level [] suffix (with the same
string[] exception) — specifically adjust the stream-handling branch inside
withoutStreamType to extract the inner content between "stream<" and the
matching ">" (handling ", error>" case) and then apply the slice(0, -2)
array-trimming only once on that extracted inner type.
- Around line 65-105: The bug is stale computation of nonEnabledFunctions
because it derives from mutable state serviceModel while effects update
serviceModel asynchronously and effects also read props.functionModel without
depending on it; fix by either removing the duplicated serviceModel state and
using the incoming model directly for computations, or move the
nonEnabledFunctions calculation inside the effects so it always derives from the
latest model variable, and add props.functionModel to the dependency arrays;
consolidate the two useEffect hooks into one that depends on [model,
selectedHandler, isNew, props.functionModel] and within that effect compute
nonEnabledFunctions from model (or serviceModel immediately after
setServiceModel) before calling setFunctionModel/setSelectedFileFormat to ensure
consistent, non-stale initialization.
- Around line 117-120: The save handler is passing the creation flag isNew as
the second argument to onSave (which expects openDiagram?: boolean); update
handleSave to pass the correct navigation flag (openDiagram) or omit the second
argument if navigation shouldn't occur — i.e., change the call in handleSave
from onSave(functionModel, isNew) to onSave(functionModel, openDiagram) (or
simply onSave(functionModel) if you intend no navigation), referencing the
handleSave and onSave symbols so the correct prop is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eed45cad-818e-48b0-9458-85ac0654a5ee
📒 Files selected for processing (2)
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsxworkspaces/bi/bi-extension/src/test/e2e-playwright-tests/file-integration/smb.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ServiceDesigner/Forms/SMBForm/index.tsx`:
- Around line 99-105: The useEffect that initializes functionModel reads the
isNew variable but only depends on selectedHandler, which can cause stale
closures; update the dependency array of that useEffect (the one referencing
isNew, selectedHandler, nonEnabledFunctions) to include isNew (and ideally
nonEnabledFunctions if it can change) so that setFunctionModel and
setSelectedFileFormat run when isNew changes and use the latest
nonEnabledFunctions/initialFunction values.
- Around line 150-172: The selectType function in SMBForm returns non-optional
error types (e.g., returning "stream<byte[], error>" and handling baseType
slices for ", error>" but not ", error?>"), which differs from
FileIntegrationForm's "stream<byte[], error?>"; either add a concise comment
near selectType and the helper that strips stream types (referencing selectType
and withoutStreamType) stating that SMB protocol responses use a non-optional
error type (and why) to make the divergence intentional and clear, or change the
implementation to match the existing pattern by making the error optional (use
"error?") and update the stream-stripping logic in withoutStreamType to also
handle ", error?>" forms consistently. Ensure the comment or change is placed
adjacent to selectType and the stream-handling code so future readers see the
rationale or consistent behavior.
In
`@workspaces/bi/bi-extension/src/test/e2e-playwright-tests/file-integration/smb.spec.ts`:
- Around line 77-80: The assertion using saveChangesBtn.toHaveClass('disabled')
does an exact match and can be flaky if other classes exist; update the check on
the saveChangesBtn locator to assert the presence of the "disabled" class via a
partial match (e.g., use a regex that matches "disabled" as a separate token
like /(^|\s)disabled($|\s)/ or use an array/regex form supported by Playwright)
so the test passes when additional classes (e.g., "primary") are present.
🪄 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: ddcb2e3d-05dd-4ed1-8296-9adba3af4fea
📒 Files selected for processing (2)
workspaces/ballerina/ballerina-visualizer/src/views/BI/ServiceDesigner/Forms/SMBForm/index.tsxworkspaces/bi/bi-extension/src/test/e2e-playwright-tests/file-integration/smb.spec.ts
Purpose
Summary by CodeRabbit
New Features
Chores
Tests
Documentation