Add custom fields for Citrus Endpoints and MessageBody#3098
Open
christophd wants to merge 2 commits intoKaotoIO:mainfrom
Open
Add custom fields for Citrus Endpoints and MessageBody#3098christophd wants to merge 2 commits intoKaotoIO:mainfrom
christophd wants to merge 2 commits intoKaotoIO:mainfrom
Conversation
Contributor
|
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 UI components and tests for creating, selecting, editing, and deleting Citrus test endpoints (typeahead, modal, table), a new EndpointsEntityHandler for schema/enumeration/mutations, maps new custom fields, and an early-return root update in CitrusTestVisualEntity. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EndpointsField
participant EndpointModal
participant EndpointFieldInner
participant EndpointsEntityHandler
participant TestResource
User->>EndpointsField: Click "Add"
EndpointsField->>EndpointModal: open(mode="Create")
EndpointModal->>EndpointFieldInner: render oneOf schema form
User->>EndpointFieldInner: select type & fill model
User->>EndpointModal: Click confirm
EndpointModal->>EndpointsEntityHandler: addNewEndpoint(type, model)
EndpointsEntityHandler->>TestResource: mutate/persist endpoints
EndpointsEntityHandler-->>EndpointModal: success
EndpointModal->>EndpointsField: onConfirm(type, model)
EndpointsField->>EndpointsField: update form value / refresh table
sequenceDiagram
participant User
participant EndpointsField
participant EndpointModal
participant EndpointFieldInner
participant EndpointsEntityHandler
participant ActionConfirmationModal
participant TestResource
User->>EndpointsField: Click "Edit" on row
EndpointsField->>EndpointModal: open(mode="Update", endpoint)
EndpointModal->>EndpointFieldInner: populate model & type
User->>EndpointFieldInner: modify model (may rename)
User->>EndpointModal: Click confirm
EndpointModal->>EndpointsEntityHandler: updateEndpoint(newType, newModel, prevName)
alt name changed
EndpointsEntityHandler->>ActionConfirmationModal: request rename confirmation
ActionConfirmationModal->>TestResource: confirm -> persist rename
else name unchanged
EndpointsEntityHandler->>TestResource: update endpoint in place
end
EndpointModal->>EndpointsField: onConfirm(...)
EndpointsField->>EndpointsField: refresh items
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
christophd
force-pushed
the
issue/add-citrus-custom-fields
branch
from
7fb28d6 to
fe42daa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/models/visualization/flows/citrus-test-visual-entity.ts (1)
213-218:⚠️ Potential issue | 🟠 MajorRoot-path updates are dropped here.
When
path === this.getRootPath(), the method returns before applyingvalue, so root-form edits never reachthis.test.VisualizationNode.updateModel()forwards the node path directly, and the other visual entities handle root updates with a real merge/update instead of a no-op. Please update the backing test object first, then recomputeid.Possible fix
if (path === this.getRootPath()) { - this.id = this.test.name; + Object.assign(this.test, value as Partial<Test>); + this.id = this.test.name; return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/citrus-test-visual-entity.ts` around lines 213 - 218, The updateModel path check currently returns before applying the incoming value to the backing test object; change updateModel so that when path === this.getRootPath() you first merge/apply value into this.test (e.g., shallow or project-appropriate deep merge of value into this.test), then recompute this.id = this.test.name and return; keep the early-return behavior only after the test object has been updated so root-form edits propagate to the underlying test before id is recalculated.
🧹 Nitpick comments (1)
packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts (1)
16-23: Avoid mutating catalog-backed schema objects
schema.nameandschema.titlemutate objects coming from the catalog cache. This can leak state across consumers/tests. Prefer cloning before enrichment.♻️ Suggested refactor
const endpointDefinition = endpointsCatalog[endpointKey]; - const schema = endpointsCatalog[endpointKey].propertiesSchema as KaotoSchemaDefinition['schema']; + const sourceSchema = endpointsCatalog[endpointKey].propertiesSchema as KaotoSchemaDefinition['schema']; + const schema = sourceSchema ? { ...sourceSchema } : undefined; if (schema) { schema.name = endpointKey; schema.title = endpointDefinition.title; endpoints.push(schema); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts` around lines 16 - 23, The code mutates catalog-backed schema objects by assigning schema.name and schema.title directly from endpointsCatalog; instead, clone the schema before enrichment to avoid leaking state — locate the loop that iterates over endpointsCatalog (variables endpointKey, endpointDefinition, schema) in endpoints-entity-handler.ts and replace the in-place mutation with creating a shallow/deep copy of endpointDefinition.propertiesSchema (e.g., via structuredClone or a deep clone helper) then set copy.name and copy.title and push the copy into endpoints so the original KaotoSchemaDefinition['schema'] in the catalog remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.tsx`:
- Around line 18-30: The dropdown items are only recomputed when the local
lastUpdated state changes, so EndpointField's items become stale when endpoints
change elsewhere; update EndpointField to derive items from the actual endpoints
data or subscribe to endpoint changes instead of relying on lastUpdated —
specifically, change the useMemo that builds items (currently calling
endpointsHandler.getAllEndpointsNameAndType()) to either remove lastUpdated and
include the live endpoints result in the dependency array (e.g., use a variable
like endpoints = endpointsHandler.getAllEndpointsNameAndType() and depend on
that) or register a change listener on endpointsHandler that calls
setLastUpdated when the backing endpoints mutate; keep the mapping to {name,
description: String(type), value: String(name)} but ensure the memo dependencies
reflect actual endpoint updates so the dropdown never serves stale data.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsx`:
- Around line 36-39: onHandleSchemaChange currently only updates setEndpointType
and calls onSchemaChange, leaving the previous branch's model data in place;
update onHandleSchemaChange to also clear/reset the endpoint's model/form state
when switching oneOf branches (e.g., call the component's model reset handler or
setModel({}) / clearFormValues) so stale scalar fields from the old schema are
removed before applying the new schema; keep invoking
setEndpointType(schema?.schema.name || '') and onSchemaChange(schema) as before
but add the model-reset call within onHandleSchemaChange.
- Around line 21-34: The reset function onCleanInput currently only clears local
schema and model (onSchemaChange() and onModelChange({})), but because
endpointType remains set the useEffect (which watches endpointType and
re-applies the selected schema) will rehydrate the old selection; update
onCleanInput to also clear the parent endpointType by invoking the provided
parent handler (e.g., call onEndpointTypeChange(undefined) or the actual prop
used to set endpointType) so endpointType is cleared and the useEffect no longer
re-applies the previous schema.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.test.tsx`:
- Around line 160-165: The tests in EndpointModal.test.tsx currently wrap
assertions in "if (confirmButton)" which allows them to silently pass when the
confirm button isn't rendered; update both waitFor blocks (the one referencing
'endpoint-modal-confirm-btn' around lines ~160-165 and the similar block around
~178-183) to explicitly assert the button exists before other checks—use a query
that fails the test when missing (e.g., getByTestId or
expect(queryByTestId('endpoint-modal-confirm-btn')).not.toBeNull()) and then
assert its disabled/enabled state, removing the conditional guard so absence of
the button fails the test.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx`:
- Around line 37-46: The effect that calls selectEndpoint() runs on every render
because it lacks a dependency array and isn't gated by endpointsSchema; update
the useEffect to only run when it should by adding a dependency array and an
explicit guard: run the effect only when endpointModel is undefined/null and
endpointsSchema is defined (or on initial mount if intended) and then call
selectEndpoint() once, setting endpointType via setEndpointType and initializing
endpointModel via setEndpointModel; reference the existing endpointModel,
selectEndpoint, setEndpointType, setEndpointModel and endpointsSchema symbols to
locate and change the effect.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx`:
- Around line 78-83: handleDelete currently mutates value and deletes an
endpoint immediately; add the same safety/confirmation flow used by the rename
path: before removing, call a check for references (use the same logic that
rename uses to detect send/receive references), show a confirmation modal that
lists referencing actions and requires explicit confirm, and only on confirm
perform the removal by producing a new value array, call onChange(newValue), and
then call setItems(endpointsHandler.getDefinedEndpointsNameAndType(newValue));
if references exist optionally offer to cascade-clean or block
deletion—implement whichever behavior matches rename flow so tests/actions
aren’t broken.
- Around line 35-53: In the Update branch of EndpointsField.tsx, always refresh
the table after a successful endpointsHandler.updateEndpoint call: move or add
the setItems(endpointsHandler.getDefinedEndpointsNameAndType(value)) invocation
so it runs unconditionally after endpointsHandler.updateEndpoint(type, model as
unknown as Record<string, unknown>, prevName) (while keeping the existing rename
confirmation via renameEndpointModalContext.actionConfirmation and the
ACTION_ID_CANCEL return logic intact) so changes to endpoint type (or other
non-name fields) are reflected immediately.
In
`@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.test.ts`:
- Around line 263-276: Test doesn't actually exercise the "target not found"
branch because it never sets any existing endpoints; before calling
endpointsHandler.updateEndpoint in this test, pre-populate the model's endpoints
(e.g., testModel.endpoints) with an endpoint object whose name is different from
the update target (not 'nonExistent') so updateEndpoint('http-client', {...},
'nonExistent') runs the "target not found" path and then assert
testEntity.endpoints length/inclusion; also update the test title to reflect
"target not found" behavior and reference endpointsHandler.updateEndpoint and
testModel.endpoints when making the change.
In
`@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts`:
- Around line 31-33: getAllEndpointsNameAndType currently dereferences
testEntity.endpoints and iterates testEntity.actions without null checks; update
this method (and any other usages such as the block around lines 57-63) to first
obtain the test entity via getTestEntity() and guard against undefined/null
(e.g., if !testEntity return an empty array/object), then safely access
testEntity.endpoints and testEntity.actions only when present (use optional
chaining or explicit checks) so no runtime error occurs when
testResource/testEntity is undefined.
- Around line 110-134: updateEndpoint currently does nothing when test.endpoints
exists but no endpoint matches endpointName, silently dropping the update;
modify updateEndpoint (in packages/ui...endpoints-entity-handler.ts) so that
after computing index = test.endpoints.findIndex(...) if index > -1 you replace
as now, otherwise call this.addNewEndpoint(type, model) (or push
this.createEndpoint(type, model) into test.endpoints) to ensure the update is
applied when the name is not found; keep use of getTestEntity, getEndpointType,
and findEndpointName as-is to locate matches.
- Around line 146-152: createEndpoint currently splits type into tokens and
always nests model under tokens[1], which produces {direct: {undefined: model}}
for single-token types; update createEndpoint to check tokens.length (or whether
tokens[1] is defined) and if there is only one token assign the model directly
to endpoint[tokens[0]] = model, otherwise keep the existing nested behavior
endpoint[tokens[0]] = { [tokens[1]]: model }; use the function name
createEndpoint and the tokens variable to locate and modify the logic.
- Around line 91-93: getEndpointType can leave endpointType as an empty string
which makes subType undefined and causes the for...in loop to throw; fix by
guarding before iteration: after computing const subType =
endpoint[endpointType] as Record<string, unknown>, add a check like if
(!endpointType || subType == null || typeof subType !== 'object') return a safe
default (or handle the "no object-like root key" case), otherwise proceed with
for (const key in subType); update getEndpointType to always return a stable
value or early-return when endpointType/subType is invalid so the for...in loop
never iterates over undefined.
---
Outside diff comments:
In `@packages/ui/src/models/visualization/flows/citrus-test-visual-entity.ts`:
- Around line 213-218: The updateModel path check currently returns before
applying the incoming value to the backing test object; change updateModel so
that when path === this.getRootPath() you first merge/apply value into this.test
(e.g., shallow or project-appropriate deep merge of value into this.test), then
recompute this.id = this.test.name and return; keep the early-return behavior
only after the test object has been updated so root-form edits propagate to the
underlying test before id is recalculated.
---
Nitpick comments:
In
`@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts`:
- Around line 16-23: The code mutates catalog-backed schema objects by assigning
schema.name and schema.title directly from endpointsCatalog; instead, clone the
schema before enrichment to avoid leaking state — locate the loop that iterates
over endpointsCatalog (variables endpointKey, endpointDefinition, schema) in
endpoints-entity-handler.ts and replace the in-place mutation with creating a
shallow/deep copy of endpointDefinition.propertiesSchema (e.g., via
structuredClone or a deep clone helper) then set copy.name and copy.title and
push the copy into endpoints so the original KaotoSchemaDefinition['schema'] in
the catalog remains unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2ffe15d-c309-43e2-9f28-44cbe32e55dc
📒 Files selected for processing (11)
packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/custom-fields-factory.tspackages/ui/src/models/visualization/flows/citrus-test-visual-entity.tspackages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.test.tspackages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsx (1)
14-43: Remove redundant fragment wrapper.The fragment
<>...</>wraps only a single child (FieldWrapper), making it unnecessary. Return theFieldWrapperdirectly.♻️ Proposed fix
return ( - <> - <FieldWrapper - propName={propName} - required={required} - title={schema.title} - type="string" - description={schema.description} - defaultValue={schema.default?.toString()} - > - <InputGroup> - <InputGroupItem isFill> - <TextArea - rows={5} - type="text" - role="textbox" - id={propName} - name={propName} - placeholder={schema.default?.toString()} - value={value} - onChange={onFieldChange} - aria-describedby={`${propName}-popover`} - isDisabled={disabled} - ref={textAreaRef} - /> - </InputGroupItem> - </InputGroup> - </FieldWrapper> - </> + <FieldWrapper + propName={propName} + required={required} + title={schema.title} + type="string" + description={schema.description} + defaultValue={schema.default?.toString()} + > + <InputGroup> + <InputGroupItem isFill> + <TextArea + rows={5} + type="text" + role="textbox" + id={propName} + name={propName} + placeholder={schema.default?.toString()} + value={value} + onChange={onFieldChange} + aria-describedby={`${propName}-popover`} + isDisabled={disabled} + ref={textAreaRef} + /> + </InputGroupItem> + </InputGroup> + </FieldWrapper> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsx` around lines 14 - 43, The JSX fragment wrapper in the MessageBodyField component is redundant because it only wraps a single FieldWrapper element; remove the fragment tokens <> and </> and return the FieldWrapper directly from the component (update the return statement in MessageBodyField so it returns FieldWrapper with its children, keeping all props on FieldWrapper and preserving the InputGroup/TextArea structure and refs/handlers).packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts (2)
9-11: MarktestResourceasreadonly.The constructor parameter is never reassigned after initialization. Adding
readonlycommunicates intent and prevents accidental mutation.Suggested fix
-constructor(private testResource?: CitrusTestResource | undefined) { +constructor(private readonly testResource?: CitrusTestResource) {Note:
CitrusTestResource | undefinedis redundant with the?optional modifier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts` around lines 9 - 11, Constructor parameter testResource in the class defined in endpoints-entity-handler.ts should be marked readonly and the redundant union type removed: change the signature in the constructor (constructor(private testResource?: CitrusTestResource | undefined)) to use readonly and a single optional type (constructor(private readonly testResource?: CitrusTestResource)) so the field cannot be reassigned and the type is not duplicated; update references to this.testResource if any but no behavior change is needed.
136-144: Use nullish coalescing assignment for clarity.SonarCloud suggests using
??=instead of the if-check pattern for initializingtest.endpoints.Suggested fix
addNewEndpoint(type: string, model: Record<string, unknown>) { const test = this.getTestEntity(); - - if (!test.endpoints) { - test.endpoints = []; - } + test.endpoints ??= []; test.endpoints.push(this.createEndpoint(type, model)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts` around lines 136 - 144, Replace the manual if-check in addNewEndpoint that initializes test.endpoints with a nullish coalescing assignment to be more concise and clear: retrieve the test via getTestEntity(), use test.endpoints ??= [] to ensure endpoints is an array when undefined/null, then push the created endpoint via createEndpoint(type, model); ensure you reference addNewEndpoint, getTestEntity, test.endpoints, and createEndpoint when making the change.packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsx (1)
1-2: Consolidate duplicate imports from@kaoto/forms.SonarCloud correctly flags that
@kaoto/formsis imported on two separate lines. Merge them into a single import statement for cleaner code.Suggested fix
-import { isDefined } from '@kaoto/forms'; -import { ArrayFieldWrapper, FieldProps, KaotoForm, OneOfSchemas, SchemaList, useOneOfField } from '@kaoto/forms'; +import { ArrayFieldWrapper, FieldProps, isDefined, KaotoForm, OneOfSchemas, SchemaList, useOneOfField } from '@kaoto/forms';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsx` around lines 1 - 2, The file imports from '@kaoto/forms' on two lines (isDefined and the other symbols), so consolidate them into a single import statement that includes isDefined along with ArrayFieldWrapper, FieldProps, KaotoForm, OneOfSchemas, SchemaList, and useOneOfField; update the import to a single line "import { isDefined, ArrayFieldWrapper, FieldProps, KaotoForm, OneOfSchemas, SchemaList, useOneOfField } from '@kaoto/forms';" and remove the duplicate import.packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx (2)
33-33: Consider standarduseStatedestructuring for clarity.The pattern
const [endpointModel = endpoint, setEndpointModel] = useState<unknown>();works but is unusual. SonarCloud flags this because the default assignment happens at destructure time, not inuseState. This is intentional (usingendpointprop as initial fallback), but consider documenting or using a more explicit pattern.Alternative using explicit initializer
-const [endpointModel = endpoint, setEndpointModel] = useState<unknown>(); +const [endpointModel, setEndpointModel] = useState<unknown>(endpoint);Note: This changes behavior slightly - the prop would only be used as initial value, not as a fallback on every render. If the current behavior is intentional, add a comment explaining why.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx` at line 33, The current destructuring pattern "const [endpointModel = endpoint, setEndpointModel] = useState<unknown>();" is unusual and confuses readers and linters because the prop fallback happens at destructure time rather than via useState; replace with a clear explicit initializer like "const [endpointModel, setEndpointModel] = useState<unknown>(() => endpoint)" so the prop is used as the initial state only, or if you need the prop to act as a fallback on every render, keep the current code but add a concise comment above the line explaining the intentional behavior; reference endpointModel, setEndpointModel, useState and the endpoint prop when making the change.
1-2: Consolidate duplicate imports from@kaoto/forms.
@kaoto/formsis imported on two separate lines. Merge them into a single import statement.Suggested fix
-import { isDefined } from '@kaoto/forms'; -import { CanvasFormTabsContext, CanvasFormTabsContextResult, ModelContextProvider, SchemaProvider } from '@kaoto/forms'; +import { CanvasFormTabsContext, CanvasFormTabsContextResult, isDefined, ModelContextProvider, SchemaProvider } from '@kaoto/forms';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx` around lines 1 - 2, The file currently imports from '@kaoto/forms' on two separate lines; consolidate them into a single import that combines all referenced symbols (isDefined, CanvasFormTabsContext, CanvasFormTabsContextResult, ModelContextProvider, SchemaProvider) into one import statement from '@kaoto/forms' so there is only one import declaration for that module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.tsx`:
- Around line 47-52: The onItemChange handler uses a non-null assertion
item!.name which will crash if item is undefined; update onItemChange to guard
item before accessing .name (e.g., if (item) onChange(item.name) else
onChange('') or onChange(undefined) depending on the expected value) so you
don't dereference a possibly undefined TypeaheadItem<string>. Ensure the fix
modifies the onItemChange function only and keeps the dependency array
[onChange] unchanged.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.test.tsx`:
- Around line 57-60: The CatalogModalContext.Provider mock is missing the
required checkCompatibility property which causes TS2741; update each Provider
usage that currently passes value={{ getNewComponent: mockGetNewComponent }} to
include a checkCompatibility stub (matching the CatalogModalContextValue shape)
alongside mockGetNewComponent (e.g., value={{ getNewComponent:
mockGetNewComponent, checkCompatibility: /* stub fn */ }}), applying this change
for every Provider wrapping EndpointModal/SuggestionRegistryProvider in this
test file so TypeScript compiles.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx`:
- Around line 78-85: handleDelete currently mutates the input array
(value.splice) and then calls onChange with the same reference; change it to
create a new array without the removed item (e.g., using filter or slicing) and
pass that new array to onChange, then call setItems with
endpointsHandler.getDefinedEndpointsNameAndType(nextArray); keep the function
name handleDelete and update its usage of value, onChange, setItems, and
endpointsHandler.getDefinedEndpointsNameAndType accordingly so no in-place
mutation occurs.
---
Nitpick comments:
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsx`:
- Around line 1-2: The file imports from '@kaoto/forms' on two lines (isDefined
and the other symbols), so consolidate them into a single import statement that
includes isDefined along with ArrayFieldWrapper, FieldProps, KaotoForm,
OneOfSchemas, SchemaList, and useOneOfField; update the import to a single line
"import { isDefined, ArrayFieldWrapper, FieldProps, KaotoForm, OneOfSchemas,
SchemaList, useOneOfField } from '@kaoto/forms';" and remove the duplicate
import.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx`:
- Line 33: The current destructuring pattern "const [endpointModel = endpoint,
setEndpointModel] = useState<unknown>();" is unusual and confuses readers and
linters because the prop fallback happens at destructure time rather than via
useState; replace with a clear explicit initializer like "const [endpointModel,
setEndpointModel] = useState<unknown>(() => endpoint)" so the prop is used as
the initial state only, or if you need the prop to act as a fallback on every
render, keep the current code but add a concise comment above the line
explaining the intentional behavior; reference endpointModel, setEndpointModel,
useState and the endpoint prop when making the change.
- Around line 1-2: The file currently imports from '@kaoto/forms' on two
separate lines; consolidate them into a single import that combines all
referenced symbols (isDefined, CanvasFormTabsContext,
CanvasFormTabsContextResult, ModelContextProvider, SchemaProvider) into one
import statement from '@kaoto/forms' so there is only one import declaration for
that module.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsx`:
- Around line 14-43: The JSX fragment wrapper in the MessageBodyField component
is redundant because it only wraps a single FieldWrapper element; remove the
fragment tokens <> and </> and return the FieldWrapper directly from the
component (update the return statement in MessageBodyField so it returns
FieldWrapper with its children, keeping all props on FieldWrapper and preserving
the InputGroup/TextArea structure and refs/handlers).
In
`@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts`:
- Around line 9-11: Constructor parameter testResource in the class defined in
endpoints-entity-handler.ts should be marked readonly and the redundant union
type removed: change the signature in the constructor (constructor(private
testResource?: CitrusTestResource | undefined)) to use readonly and a single
optional type (constructor(private readonly testResource?: CitrusTestResource))
so the field cannot be reassigned and the type is not duplicated; update
references to this.testResource if any but no behavior change is needed.
- Around line 136-144: Replace the manual if-check in addNewEndpoint that
initializes test.endpoints with a nullish coalescing assignment to be more
concise and clear: retrieve the test via getTestEntity(), use test.endpoints ??=
[] to ensure endpoints is an array when undefined/null, then push the created
endpoint via createEndpoint(type, model); ensure you reference addNewEndpoint,
getTestEntity, test.endpoints, and createEndpoint when making the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af6eb37e-30be-4d3e-a48d-1a47a2b73bfc
📒 Files selected for processing (11)
packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/custom-fields-factory.tspackages/ui/src/models/visualization/flows/citrus-test-visual-entity.tspackages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.test.tspackages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts
✅ Files skipped from review due to trivial changes (1)
- packages/ui/src/models/visualization/flows/citrus-test-visual-entity.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.test.tsx
- packages/ui/src/components/Visualization/Canvas/Form/fields/custom-fields-factory.ts
- packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.test.ts
christophd
force-pushed
the
issue/add-citrus-custom-fields
branch
from
fe42daa to
ef54c20
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts (4)
31-33:⚠️ Potential issue | 🔴 CriticalDon't cast away a missing
CitrusTestResource.
getTestEntity()currently returnsundefined as Test. The next dereference ingetAllEndpointsNameAndType(),updateEndpoint(), oraddNewEndpoint()will crash whenever this handler is created without a backing resource. ReturnTest | undefinedhere and guard the callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts` around lines 31 - 33, getTestEntity currently force-casts this.testResource?.toJSON() to Test which masks undefined and causes downstream crashes; change getTestEntity() to return Test | undefined by removing the cast and returning this.testResource?.toJSON() as possibly undefined, and then update callers getAllEndpointsNameAndType, updateEndpoint, and addNewEndpoint to null-check or early-return when getTestEntity() returns undefined (or handle the undefined case) before dereferencing the Test object.
150-156:⚠️ Potential issue | 🟠 MajorHandle single-segment endpoint types.
createEndpoint('direct', model)currently serializes as{ direct: { undefined: model } }. Types without a child segment need to map directly to{ [root]: model }.Possible fix
private createEndpoint(type: string, model: Record<string, unknown>) { - const tokens = type.split('-', 2); - const endpoint = {} as Record<string, unknown>; - const subType = {} as Record<string, unknown>; - endpoint[tokens[0]] = subType; - subType[tokens[1]] = model; - return endpoint; + const [root, child] = type.split('-', 2); + if (!root) return {}; + if (!child) return { [root]: model }; + return { [root]: { [child]: model } }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts` around lines 150 - 156, createEndpoint currently splits type into tokens and always nests model under tokens[1], producing {root: {undefined: model}} for single-segment types; update createEndpoint to check whether a second segment exists (tokens[1]) and if not assign model directly as endpoint[tokens[0]] = model, otherwise preserve the existing nesting logic (keep endpoint[tokens[0]] = subType; subType[tokens[1]] = model). Reference the createEndpoint function and the tokens/subType variables when making the change.
83-100:⚠️ Potential issue | 🔴 CriticalGuard the subtype walk before iterating.
If an endpoint record has neither
typenor an object-valued root key,endpointTypestays empty andfor (const key in subType)throws. Return early before touchingsubType.Possible fix
let endpointType: string = ''; for (const key in endpoint) { if (key === 'type') { return endpoint[key] as string; } if (endpoint[key] !== undefined && typeof endpoint[key] === 'object') { endpointType = key; break; } } - const subType = endpoint[endpointType] as Record<string, unknown>; + if (!endpointType) return undefined; + const subType = endpoint[endpointType] as Record<string, unknown> | undefined; + if (!subType || typeof subType !== 'object') return endpointType; for (const key in subType) { if (subType[key] !== undefined && typeof subType[key] === 'object') { endpointType = `${endpointType}.${key}`; + break; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts` around lines 83 - 100, The code may leave endpointType empty and then access subType causing a crash; after the first loop that sets endpointType (looking at endpoint and key === 'type'), add a guard: if endpointType is falsy, return early (e.g., return '') before creating const subType, and/or verify that endpoint[endpointType] exists and is an object before the second for-in; update the checks around endpointType, endpoint and subType to avoid iterating over undefined.
122-137:⚠️ Potential issue | 🟠 MajorDon't silently drop confirmed edits when the old name is missing.
Called from
packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx, Lines 28-60, this method no-ops when no endpoint matchesprevName/model.name. The modal closes, but the user's update is lost.Possible fix
if (index > -1) { test.endpoints[index] = this.createEndpoint(type, model); + } else { + this.addNewEndpoint(type, model); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts` around lines 122 - 137, The current code silently drops edits when no endpoint matches prevName/model.name; change the logic in the block that computes index (using getEndpointType and findEndpointName) so that if index === -1 you then try a fallback match by a stable identifier (e.g., model['id'] or another unique key on the endpoints) and update that entry, and if no identifier match is found, append the new endpoint instead of no-op; use the same createEndpoint(type, model) to build the replacement/added object and update test.endpoints accordingly (refer to getEndpointType, findEndpointName, createEndpoint, and test.endpoints).packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx (3)
50-53:⚠️ Potential issue | 🟡 MinorRefresh
itemsafter every successful update.
setItems()only runs when the name changes. Editing just the endpoint type leaves the table showing the stale type until some other action rebuilds the list.Possible fix
endpointsHandler.updateEndpoint(type, model as unknown as Record<string, unknown>, prevName); - if (prevName !== model.name) { - setItems(endpointsHandler.getDefinedEndpointsNameAndType(value)); - } + setItems(endpointsHandler.getDefinedEndpointsNameAndType(value)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx` around lines 50 - 53, The items list isn't refreshed when only the endpoint type changes because setItems(...) is called only if prevName !== model.name; after calling endpointsHandler.updateEndpoint(type, model, prevName) always recompute and setItems using endpointsHandler.getDefinedEndpointsNameAndType(value) (i.e., move or add the setItems call to run unconditionally after updateEndpoint) so the table reflects type edits as well; keep the prevName check only if other logic depends on it, but ensure setItems runs after updateEndpoint.
78-83:⚠️ Potential issue | 🟡 MinorPass a new array to
onChange()when deleting.
splice()mutatesvaluein place andonChange(value)reuses the same reference. If the form layer keys off reference changes, the delete can be missed.Possible fix
const handleDelete = useCallback( (index: number) => { - value.splice(index, 1); - onChange(value); - setItems(endpointsHandler.getDefinedEndpointsNameAndType(value)); + const nextValue = [...value.slice(0, index), ...value.slice(index + 1)]; + onChange(nextValue); + setItems(endpointsHandler.getDefinedEndpointsNameAndType(nextValue)); }, [endpointsHandler, onChange, value], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx` around lines 78 - 83, handleDelete is mutating the value array in place with value.splice(...) and then calling onChange(value), which can be missed by the form layer; instead create a new array (e.g., const newValue = value.slice(0, index).concat(value.slice(index + 1)) or value.filter((_, i) => i !== index)), call onChange(newValue), and then call setItems(endpointsHandler.getDefinedEndpointsNameAndType(newValue)) so you never pass the original mutated reference to onChange.
78-83:⚠️ Potential issue | 🟠 MajorMirror the rename confirmation before deleting.
Lines 40-47 already acknowledge that endpoint names may be referenced elsewhere, but
handleDelete()removes the row immediately. One click can leave send/receive actions pointing at a missing endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx` around lines 78 - 83, In handleDelete, do not immediately splice the shared value; instead mirror the rename confirmation flow: first use the endpointsHandler to check whether the endpoint to delete is referenced (similar to the checks used in the rename flow), present a confirmation modal listing references and require explicit user confirmation, and only after the user confirms perform the deletion on a copied array (avoid mutating the original value in-place), then call onChange(updatedValue) and setItems(endpointsHandler.getDefinedEndpointsNameAndType(updatedValue)).packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsx (2)
31-34:⚠️ Potential issue | 🟠 MajorClear
endpointTypeinonCleanInput().Right now only the schema and model are reset. Because the effect at Lines 21-29 still sees the old
endpointType, the previous branch gets selected again on the next render.Possible fix
const onCleanInput = () => { + setEndpointType(''); onSchemaChange(); onModelChange({}); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsx` around lines 31 - 34, The onCleanInput handler resets schema and model but leaves endpointType set, causing the effect that reads endpointType (the conditional branch in the effect using endpointType) to re-select the previous branch on next render; update onCleanInput to also clear the endpointType by invoking the component's endpoint-type setter (e.g., call onEndpointTypeChange(undefined) or setEndpointType('') depending on the prop API) so endpointType is reset alongside onSchemaChange() and onModelChange({}).
36-39:⚠️ Potential issue | 🟠 MajorReset the model when switching oneOf branches.
onHandleSchemaChange()swaps the schema but keeps the old branch's model object. That stale data can be submitted under the new endpoint type.Possible fix
const onHandleSchemaChange = (schema?: OneOfSchemas) => { setEndpointType(schema?.schema.name || ''); + onModelChange({}); onSchemaChange(schema); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsx` around lines 36 - 39, onHandleSchemaChange swaps the oneOf schema but leaves the previous branch's model intact; update this handler to also clear the current model when the selected schema changes by calling the component's model reset API (e.g., invoke the local model setter such as setModel({}) or the prop callback onModelChange(undefined)) immediately after setEndpointType(schema?.schema.name || '') and before/after onSchemaChange(schema) so stale data from the old branch cannot be submitted under the new endpoint type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx`:
- Around line 92-95: When opening the Create modal via handleAdd, clear the
previous edit state so the modal doesn't prefill with the last-edited endpoint:
reset the editModel and endpointType state (the same state passed into
<EndpointModal>) before calling setOperation('Create') and setIsOpen(true).
Apply the same reset wherever the modal is opened around the EndpointModal usage
(the code that passes editModel and endpointType into <EndpointModal>) so Create
always starts with empty/cleared values.
---
Duplicate comments:
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsx`:
- Around line 31-34: The onCleanInput handler resets schema and model but leaves
endpointType set, causing the effect that reads endpointType (the conditional
branch in the effect using endpointType) to re-select the previous branch on
next render; update onCleanInput to also clear the endpointType by invoking the
component's endpoint-type setter (e.g., call onEndpointTypeChange(undefined) or
setEndpointType('') depending on the prop API) so endpointType is reset
alongside onSchemaChange() and onModelChange({}).
- Around line 36-39: onHandleSchemaChange swaps the oneOf schema but leaves the
previous branch's model intact; update this handler to also clear the current
model when the selected schema changes by calling the component's model reset
API (e.g., invoke the local model setter such as setModel({}) or the prop
callback onModelChange(undefined)) immediately after
setEndpointType(schema?.schema.name || '') and before/after
onSchemaChange(schema) so stale data from the old branch cannot be submitted
under the new endpoint type.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx`:
- Around line 50-53: The items list isn't refreshed when only the endpoint type
changes because setItems(...) is called only if prevName !== model.name; after
calling endpointsHandler.updateEndpoint(type, model, prevName) always recompute
and setItems using endpointsHandler.getDefinedEndpointsNameAndType(value) (i.e.,
move or add the setItems call to run unconditionally after updateEndpoint) so
the table reflects type edits as well; keep the prevName check only if other
logic depends on it, but ensure setItems runs after updateEndpoint.
- Around line 78-83: handleDelete is mutating the value array in place with
value.splice(...) and then calling onChange(value), which can be missed by the
form layer; instead create a new array (e.g., const newValue = value.slice(0,
index).concat(value.slice(index + 1)) or value.filter((_, i) => i !== index)),
call onChange(newValue), and then call
setItems(endpointsHandler.getDefinedEndpointsNameAndType(newValue)) so you never
pass the original mutated reference to onChange.
- Around line 78-83: In handleDelete, do not immediately splice the shared
value; instead mirror the rename confirmation flow: first use the
endpointsHandler to check whether the endpoint to delete is referenced (similar
to the checks used in the rename flow), present a confirmation modal listing
references and require explicit user confirmation, and only after the user
confirms perform the deletion on a copied array (avoid mutating the original
value in-place), then call onChange(updatedValue) and
setItems(endpointsHandler.getDefinedEndpointsNameAndType(updatedValue)).
In
`@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts`:
- Around line 31-33: getTestEntity currently force-casts
this.testResource?.toJSON() to Test which masks undefined and causes downstream
crashes; change getTestEntity() to return Test | undefined by removing the cast
and returning this.testResource?.toJSON() as possibly undefined, and then update
callers getAllEndpointsNameAndType, updateEndpoint, and addNewEndpoint to
null-check or early-return when getTestEntity() returns undefined (or handle the
undefined case) before dereferencing the Test object.
- Around line 150-156: createEndpoint currently splits type into tokens and
always nests model under tokens[1], producing {root: {undefined: model}} for
single-segment types; update createEndpoint to check whether a second segment
exists (tokens[1]) and if not assign model directly as endpoint[tokens[0]] =
model, otherwise preserve the existing nesting logic (keep endpoint[tokens[0]] =
subType; subType[tokens[1]] = model). Reference the createEndpoint function and
the tokens/subType variables when making the change.
- Around line 83-100: The code may leave endpointType empty and then access
subType causing a crash; after the first loop that sets endpointType (looking at
endpoint and key === 'type'), add a guard: if endpointType is falsy, return
early (e.g., return '') before creating const subType, and/or verify that
endpoint[endpointType] exists and is an object before the second for-in; update
the checks around endpointType, endpoint and subType to avoid iterating over
undefined.
- Around line 122-137: The current code silently drops edits when no endpoint
matches prevName/model.name; change the logic in the block that computes index
(using getEndpointType and findEndpointName) so that if index === -1 you then
try a fallback match by a stable identifier (e.g., model['id'] or another unique
key on the endpoints) and update that entry, and if no identifier match is
found, append the new endpoint instead of no-op; use the same
createEndpoint(type, model) to build the replacement/added object and update
test.endpoints accordingly (refer to getEndpointType, findEndpointName,
createEndpoint, and test.endpoints).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80539cf3-e754-423e-981a-0edb7d1184de
📒 Files selected for processing (11)
packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/custom-fields-factory.tspackages/ui/src/models/visualization/flows/citrus-test-visual-entity.tspackages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.test.tspackages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts
✅ Files skipped from review due to trivial changes (3)
- packages/ui/src/models/visualization/flows/citrus-test-visual-entity.ts
- packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsx
- packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.test.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.test.tsx
- packages/ui/src/components/Visualization/Canvas/Form/fields/custom-fields-factory.ts
- packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx
- packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.tsx
- packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.test.ts
christophd
force-pushed
the
issue/add-citrus-custom-fields
branch
from
ef54c20 to
a3ff383
Compare
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts (2)
33-35:⚠️ Potential issue | 🟠 MajorReturn
Test | undefinedhere and guard the callers.
getTestEntity()can returnundefined, but this cast hides that from the type system.getAllEndpointsNameAndType(),updateEndpoint(), andaddNewEndpoint()then dereference it immediately, so the optional-resource constructor path still crashes at runtime.Minimal fix
- getTestEntity() { - return this.testResource?.toJSON() as Test; + getTestEntity(): Test | undefined { + return this.testResource?.toJSON() as Test | undefined; } getAllEndpointsNameAndType(): { name: string; type: string }[] { const testEntity = this.getTestEntity(); + if (!testEntity) return []; const allEndpoints: { name: string; type: string }[] = this.getDefinedEndpointsNameAndType( - testEntity.endpoints || [], + testEntity.endpoints ?? [], ); - for (const action of testEntity.actions) { + for (const action of testEntity.actions ?? []) { if (action.createEndpoint != undefined) { // ... } } } updateEndpoint(type: string, model: Record<string, unknown>, prevName: string | undefined) { const test = this.getTestEntity(); + if (!test) return; // ... } addNewEndpoint(type: string, model: Record<string, unknown>) { const test = this.getTestEntity(); + if (!test) return; // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts` around lines 33 - 35, getTestEntity() currently casts this.testResource?.toJSON() to Test hiding that it can be undefined; change its signature/return to Test | undefined and remove the unsafe cast, then update callers getAllEndpointsNameAndType, updateEndpoint, and addNewEndpoint to guard against a missing test entity (e.g., early-return or throw a clear error) before dereferencing properties—ensure all places that used getTestEntity() handle the undefined case rather than assuming a Test.
126-141:⚠️ Potential issue | 🟠 MajorAppend when the named endpoint is missing from an existing list.
If
test.endpointsalready exists andfindIndex()returns-1, the update is dropped on the floor. That makes rename/mismatch flows lose the user's changes instead of adding the new endpoint.Suggested fix
if (index > -1) { test.endpoints[index] = this.createEndpoint(type, model); + } else { + test.endpoints.push(this.createEndpoint(type, model)); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts` around lines 126 - 141, The current logic computes endpointName and searches test.endpoints via findIndex but if findIndex returns -1 the new endpoint is silently dropped; update the handler so that when index === -1 you append a new endpoint instead of ignoring it. Specifically, in the block using endpointName, after computing index with test.endpoints.findIndex (which relies on this.getEndpointType and this.findEndpointName), add an else branch that pushes this.createEndpoint(type, model) onto test.endpoints (or initializes test.endpoints as an array first if missing) so rename/mismatch flows create the new endpoint when not found.packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.test.ts (1)
263-276:⚠️ Potential issue | 🟡 MinorSeed an existing endpoint here to cover the real “not found” branch.
This still only exercises the
!test.endpointsearly-add path. If the intent is “target name not found in an existing list”, pre-populatetestModel.endpointswith a different endpoint first and assert the new one was appended.Suggested test adjustment
it('should add endpoint if update target not found', () => { + testModel.endpoints = [ + { + direct: { + name: 'existingEndpoint', + }, + }, + ]; + endpointsHandler.updateEndpoint( 'http-client', { name: 'httpClient', requestUrl: 'http://localhost:8080', @@ const testEntity = endpointsHandler.getTestEntity(); expect(testEntity.endpoints).toBeDefined(); - expect(testEntity.endpoints!.length).toBe(1); + expect(testEntity.endpoints!.length).toBe(2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.test.ts` around lines 263 - 276, The test currently only covers the branch where test.endpoints is falsy; to cover the “target not found in an existing list” branch, pre-populate the test entity's endpoints array with a different endpoint before calling endpointsHandler.updateEndpoint (e.g., call endpointsHandler.getTestEntity(), push an endpoint object with a different name into its endpoints array, then call endpointsHandler.updateEndpoint('http-client', {...}, 'nonExistent')), and update assertions to expect the endpoints array length to increase (e.g., from 1 to 2) and that the new endpoint was appended.
🧹 Nitpick comments (9)
packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx (3)
69-75: UnnecessaryasynconhandleConfirm.The function is marked
asyncbut doesn't useawait. Remove theasynckeyword.♻️ Proposed fix
- const handleConfirm = useCallback(async () => { + const handleConfirm = useCallback(() => { if (!endpointType || endpointModel === undefined || typeof endpointModel !== 'object') { return; } onConfirm(endpointType, endpointModel); }, [endpointType, endpointModel, onConfirm]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx` around lines 69 - 75, The handleConfirm function is declared async but never uses await; remove the unnecessary async keyword from the useCallback declaration for handleConfirm (the function that checks endpointType and endpointModel and calls onConfirm(endpointType, endpointModel)) so it becomes a plain synchronous callback while keeping the dependency array [endpointType, endpointModel, onConfirm] unchanged.
33-33: Unusual useState destructuring pattern.Using a default value in useState destructuring (
const [endpointModel = endpoint, ...]) is unconventional and can cause confusion. The default is applied on every render whenendpointModelisundefined, which may not be the intended behavior.Consider using a standard pattern with explicit initialization:
♻️ Proposed fix
- const [endpointModel = endpoint, setEndpointModel] = useState<unknown>(); + const [endpointModel, setEndpointModel] = useState<unknown>(endpoint);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx` at line 33, The useState destructuring currently uses a default value in the pattern `const [endpointModel = endpoint, setEndpointModel] = useState<unknown>();` which is unconventional and reapplies the default on each render; change it to a proper initialization like `const [endpointModel, setEndpointModel] = useState<unknown>(endpoint);` inside EndpointModal.tsx to set the initial state once, and if you need to keep state synced when the incoming `endpoint` prop changes, add a useEffect that updates `setEndpointModel(endpoint)` when `endpoint` changes.
1-2: Consolidate duplicate@kaoto/formsimports.♻️ Proposed fix
-import { isDefined } from '@kaoto/forms'; -import { CanvasFormTabsContext, CanvasFormTabsContextResult, ModelContextProvider, SchemaProvider } from '@kaoto/forms'; +import { + CanvasFormTabsContext, + CanvasFormTabsContextResult, + isDefined, + ModelContextProvider, + SchemaProvider, +} from '@kaoto/forms';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx` around lines 1 - 2, There are two separate imports from '@kaoto/forms' in EndpointModal.tsx; consolidate them into a single import statement that includes all referenced symbols (isDefined, CanvasFormTabsContext, CanvasFormTabsContextResult, ModelContextProvider, SchemaProvider) to remove the duplicate import and keep imports tidy.packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsx (1)
1-2: Consolidate duplicate@kaoto/formsimports.SonarCloud flagged that
@kaoto/formsis imported twice. Consolidate into a single import statement.♻️ Proposed fix
-import { isDefined } from '@kaoto/forms'; -import { ArrayFieldWrapper, FieldProps, KaotoForm, OneOfSchemas, SchemaList, useOneOfField } from '@kaoto/forms'; +import { + ArrayFieldWrapper, + FieldProps, + isDefined, + KaotoForm, + OneOfSchemas, + SchemaList, + useOneOfField, +} from '@kaoto/forms';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsx` around lines 1 - 2, There are duplicate imports from '@kaoto/forms'—combine them into a single import statement that lists all used symbols (isDefined, ArrayFieldWrapper, FieldProps, KaotoForm, OneOfSchemas, SchemaList, useOneOfField) instead of importing from '@kaoto/forms' twice; update the import lines in EndpointFieldInner.tsx to a single consolidated import and remove the redundant import.packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsx (2)
14-43: Remove redundant fragment wrapper.The fragment (
<>...</>) wrappingFieldWrapperis unnecessary since there's only one child element. SonarCloud correctly flagged this.♻️ Proposed fix
return ( - <> - <FieldWrapper - propName={propName} - required={required} - title={schema.title} - type="string" - description={schema.description} - defaultValue={schema.default?.toString()} - > - <InputGroup> - <InputGroupItem isFill> - <TextArea - rows={5} - type="text" - role="textbox" - id={propName} - name={propName} - placeholder={schema.default?.toString()} - value={value} - onChange={onFieldChange} - aria-describedby={`${propName}-popover`} - isDisabled={disabled} - ref={textAreaRef} - /> - </InputGroupItem> - </InputGroup> - </FieldWrapper> - </> + <FieldWrapper + propName={propName} + required={required} + title={schema.title} + type="string" + description={schema.description} + defaultValue={schema.default?.toString()} + > + <InputGroup> + <InputGroupItem isFill> + <TextArea + rows={5} + type="text" + role="textbox" + id={propName} + name={propName} + placeholder={schema.default?.toString()} + value={value} + onChange={onFieldChange} + aria-describedby={`${propName}-popover`} + isDisabled={disabled} + /> + </InputGroupItem> + </InputGroup> + </FieldWrapper> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsx` around lines 14 - 43, The JSX fragment wrapping the single FieldWrapper in MessageBodyField's render is redundant; remove the surrounding <>...</> so the component returns the FieldWrapper directly. Edit the MessageBodyField (the function returning the JSX containing FieldWrapper, InputGroup, InputGroupItem, and TextArea) and return the FieldWrapper element without the fragment wrapper, keeping all existing props (propName, required, title, type, description, defaultValue), children (InputGroup -> InputGroupItem -> TextArea), event handlers (onFieldChange), refs (textAreaRef), and attributes unchanged.
8-8: UnusedtextAreaRefdeclaration.The
textAreaRefis created but never used beyond being passed to theTextAreacomponent. If there's no intention to imperatively control the textarea (e.g., focus management), consider removing this ref to reduce complexity.♻️ Proposed fix
-import { FunctionComponent, useContext, useRef } from 'react'; +import { FunctionComponent, useContext } from 'react'; export const MessageBodyField: FunctionComponent<FieldProps> = ({ propName, required }) => { const { schema } = useContext(SchemaContext); const { value = '', onChange, disabled } = useFieldValue<string>(propName); - const textAreaRef = useRef<HTMLTextAreaElement>(null);And remove
ref={textAreaRef}from the TextArea component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsx` at line 8, The declared ref textAreaRef in MessageBodyField is unused beyond being passed into the TextArea component; remove the unused declaration and the unused prop to simplify the component by deleting the line that creates textAreaRef (const textAreaRef = useRef<HTMLTextAreaElement>(null);) and removing ref={textAreaRef} from the TextArea usage so no unused variables remain.packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.test.tsx (1)
256-317: Test coverage for rename confirmation flow appears incomplete.The tests "should show confirmation modal when renaming an endpoint" and "should not update endpoint when rename is cancelled" set up mock contexts but don't complete the rename flow:
- Lines 288-290: The test just asserts that
actionConfirmationwasn't called yet, but never triggers the actual rename action that would invoke it.- Lines 314-316: The test verifies the initial state hasn't changed but doesn't actually test the cancel behavior.
Consider completing these tests by simulating the full rename flow through the modal, or clarify with comments that these are intentionally partial tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.test.tsx` around lines 256 - 317, The tests set up the mock actionConfirmation but never simulate the modal rename flow that triggers it; update both tests to open the EndpointModal via renderField and endpoint-edit-0-btn, then programmatically change the endpoint name input inside the modal and invoke the modal submit/confirm handler (the modal's onConfirm / handleCreateOrEdit flow) so that mockActionConfirmationModalContext.actionConfirmation is actually awaited and resolves to ACTION_ID_CONFIRM or ACTION_ID_CANCEL; for the confirm test assert actionConfirmation was called and onPropertyChange (or the endpoints list) was updated accordingly, and for the cancel test assert actionConfirmation was called and the endpoints array remained unchanged.packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.tsx (1)
36-37: Address or remove the TODO-like comment.The comment "Double check later different approach" suggests this implementation may need revisiting. Consider either addressing the concern or creating a tracking issue.
Would you like me to open an issue to track this for follow-up?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.tsx` around lines 36 - 37, The inline TODO-like comment in EndpointField (in the object-handling branch where the code checks "if (typeof value === 'object')") should be resolved: either remove the vague "Double check later different approach" comment and replace it with a clear, actionable note describing why objects are stringified (e.g., "stringify objects for endpoint payloads to preserve structure") or create/attach a tracking issue and reference its ID in the comment (e.g., "TODO: revisit object serialization - see ISSUE-123"). Update the comment adjacent to the "if (typeof value === 'object')" branch in the EndpointField component accordingly so intent and next steps are explicit.packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.test.tsx (1)
35-37: UsetoSorted()instead of in-placesort()for cleaner functional code.The in-place
sort()mutates theendpointsarray. Since the project targets Node.js 22.x,toSorted()can be used to return a new sorted array without mutation. Additionally, add?? 0to ensure the comparator always returns a number (in casea.name?.localeCompare()returns undefined).♻️ Proposed fix
endpointsSchema = { - oneOf: endpoints.sort((a, b) => a.name?.localeCompare(b.name)), + oneOf: endpoints.toSorted((a, b) => a.name?.localeCompare(b.name) ?? 0), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.test.tsx` around lines 35 - 37, Replace the in-place sort mutation on the endpoints array with a non-mutating toSorted call: update the endpointsSchema assignment that currently uses endpoints.sort(...) so it uses endpoints.toSorted(...) and ensure the comparator passed to toSorted returns a number by appending "?? 0" to the localeCompare call (i.e., use a.name?.localeCompare(b.name) ?? 0). This keeps endpoints unmutated and guarantees a numeric comparator for the oneOf value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts`:
- Around line 80-106: The getEndpointType function relies on iterating object
keys which can return wrong results when a 'type' property appears after other
keys; update getEndpointType to first read and return endpoint['type'] (ensuring
it's a defined string) before walking other keys (use the existing endpoint
parameter and endpointType variable), then keep the existing logic for deriving
nested types only if explicit type is absent.
---
Duplicate comments:
In
`@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.test.ts`:
- Around line 263-276: The test currently only covers the branch where
test.endpoints is falsy; to cover the “target not found in an existing list”
branch, pre-populate the test entity's endpoints array with a different endpoint
before calling endpointsHandler.updateEndpoint (e.g., call
endpointsHandler.getTestEntity(), push an endpoint object with a different name
into its endpoints array, then call
endpointsHandler.updateEndpoint('http-client', {...}, 'nonExistent')), and
update assertions to expect the endpoints array length to increase (e.g., from 1
to 2) and that the new endpoint was appended.
In
`@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts`:
- Around line 33-35: getTestEntity() currently casts this.testResource?.toJSON()
to Test hiding that it can be undefined; change its signature/return to Test |
undefined and remove the unsafe cast, then update callers
getAllEndpointsNameAndType, updateEndpoint, and addNewEndpoint to guard against
a missing test entity (e.g., early-return or throw a clear error) before
dereferencing properties—ensure all places that used getTestEntity() handle the
undefined case rather than assuming a Test.
- Around line 126-141: The current logic computes endpointName and searches
test.endpoints via findIndex but if findIndex returns -1 the new endpoint is
silently dropped; update the handler so that when index === -1 you append a new
endpoint instead of ignoring it. Specifically, in the block using endpointName,
after computing index with test.endpoints.findIndex (which relies on
this.getEndpointType and this.findEndpointName), add an else branch that pushes
this.createEndpoint(type, model) onto test.endpoints (or initializes
test.endpoints as an array first if missing) so rename/mismatch flows create the
new endpoint when not found.
---
Nitpick comments:
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.tsx`:
- Around line 36-37: The inline TODO-like comment in EndpointField (in the
object-handling branch where the code checks "if (typeof value === 'object')")
should be resolved: either remove the vague "Double check later different
approach" comment and replace it with a clear, actionable note describing why
objects are stringified (e.g., "stringify objects for endpoint payloads to
preserve structure") or create/attach a tracking issue and reference its ID in
the comment (e.g., "TODO: revisit object serialization - see ISSUE-123"). Update
the comment adjacent to the "if (typeof value === 'object')" branch in the
EndpointField component accordingly so intent and next steps are explicit.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsx`:
- Around line 1-2: There are duplicate imports from '@kaoto/forms'—combine them
into a single import statement that lists all used symbols (isDefined,
ArrayFieldWrapper, FieldProps, KaotoForm, OneOfSchemas, SchemaList,
useOneOfField) instead of importing from '@kaoto/forms' twice; update the import
lines in EndpointFieldInner.tsx to a single consolidated import and remove the
redundant import.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.test.tsx`:
- Around line 35-37: Replace the in-place sort mutation on the endpoints array
with a non-mutating toSorted call: update the endpointsSchema assignment that
currently uses endpoints.sort(...) so it uses endpoints.toSorted(...) and ensure
the comparator passed to toSorted returns a number by appending "?? 0" to the
localeCompare call (i.e., use a.name?.localeCompare(b.name) ?? 0). This keeps
endpoints unmutated and guarantees a numeric comparator for the oneOf value.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx`:
- Around line 69-75: The handleConfirm function is declared async but never uses
await; remove the unnecessary async keyword from the useCallback declaration for
handleConfirm (the function that checks endpointType and endpointModel and calls
onConfirm(endpointType, endpointModel)) so it becomes a plain synchronous
callback while keeping the dependency array [endpointType, endpointModel,
onConfirm] unchanged.
- Line 33: The useState destructuring currently uses a default value in the
pattern `const [endpointModel = endpoint, setEndpointModel] =
useState<unknown>();` which is unconventional and reapplies the default on each
render; change it to a proper initialization like `const [endpointModel,
setEndpointModel] = useState<unknown>(endpoint);` inside EndpointModal.tsx to
set the initial state once, and if you need to keep state synced when the
incoming `endpoint` prop changes, add a useEffect that updates
`setEndpointModel(endpoint)` when `endpoint` changes.
- Around line 1-2: There are two separate imports from '@kaoto/forms' in
EndpointModal.tsx; consolidate them into a single import statement that includes
all referenced symbols (isDefined, CanvasFormTabsContext,
CanvasFormTabsContextResult, ModelContextProvider, SchemaProvider) to remove the
duplicate import and keep imports tidy.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.test.tsx`:
- Around line 256-317: The tests set up the mock actionConfirmation but never
simulate the modal rename flow that triggers it; update both tests to open the
EndpointModal via renderField and endpoint-edit-0-btn, then programmatically
change the endpoint name input inside the modal and invoke the modal
submit/confirm handler (the modal's onConfirm / handleCreateOrEdit flow) so that
mockActionConfirmationModalContext.actionConfirmation is actually awaited and
resolves to ACTION_ID_CONFIRM or ACTION_ID_CANCEL; for the confirm test assert
actionConfirmation was called and onPropertyChange (or the endpoints list) was
updated accordingly, and for the cancel test assert actionConfirmation was
called and the endpoints array remained unchanged.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsx`:
- Around line 14-43: The JSX fragment wrapping the single FieldWrapper in
MessageBodyField's render is redundant; remove the surrounding <>...</> so the
component returns the FieldWrapper directly. Edit the MessageBodyField (the
function returning the JSX containing FieldWrapper, InputGroup, InputGroupItem,
and TextArea) and return the FieldWrapper element without the fragment wrapper,
keeping all existing props (propName, required, title, type, description,
defaultValue), children (InputGroup -> InputGroupItem -> TextArea), event
handlers (onFieldChange), refs (textAreaRef), and attributes unchanged.
- Line 8: The declared ref textAreaRef in MessageBodyField is unused beyond
being passed into the TextArea component; remove the unused declaration and the
unused prop to simplify the component by deleting the line that creates
textAreaRef (const textAreaRef = useRef<HTMLTextAreaElement>(null);) and
removing ref={textAreaRef} from the TextArea usage so no unused variables
remain.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75592cfc-6cf7-432b-9310-6104161fa9eb
📒 Files selected for processing (14)
packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/custom-fields-factory.test.tspackages/ui/src/components/Visualization/Canvas/Form/fields/custom-fields-factory.tspackages/ui/src/models/visualization/flows/citrus-test-visual-entity.tspackages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.test.tspackages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts
✅ Files skipped from review due to trivial changes (2)
- packages/ui/src/models/visualization/flows/citrus-test-visual-entity.ts
- packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui/src/components/Visualization/Canvas/Form/fields/custom-fields-factory.ts
- packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx
christophd
force-pushed
the
issue/add-citrus-custom-fields
branch
from
a3ff383 to
90fb69b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx (2)
78-83:⚠️ Potential issue | 🟠 MajorDo not mutate
valuein place beforeonChange.Using
splice()mutates the original array and passes the same reference back. That can break change detection.Suggested fix
const handleDelete = useCallback( (index: number) => { - value.splice(index, 1); - setItems(endpointsHandler.getDefinedEndpointsNameAndType(value)); - onChange(value); + const nextValue = [...value.slice(0, index), ...value.slice(index + 1)]; + setItems(endpointsHandler.getDefinedEndpointsNameAndType(nextValue)); + onChange(nextValue); }, [endpointsHandler, onChange, value], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx` around lines 78 - 83, handleDelete currently mutates the incoming value array in-place using splice before calling onChange, which can break change detection; instead, create a new array copy (e.g., with [...value], value.slice(), or value.filter((_, i) => i !== index)) and remove the item from that new array, then call setItems(endpointsHandler.getDefinedEndpointsNameAndType(newArray)) and onChange(newArray) so handleDelete (the useCallback) never mutates the original value reference.
50-53:⚠️ Potential issue | 🟡 MinorRefresh table items after every successful update.
setItems(...)only runs on rename. If only type/properties change, the table can remain stale until another action refreshes it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx` around lines 50 - 53, endpointsHandler.updateEndpoint(...) only triggers setItems(...) when the endpoint was renamed, leaving the table stale on type/property-only changes; modify EndpointsField so that after calling endpointsHandler.updateEndpoint(type, model, prevName) you always refresh the UI by calling setItems(endpointsHandler.getDefinedEndpointsNameAndType(value)) (i.e., move or duplicate the setItems call to run unconditionally after updateEndpoint completes/succeeds) so the table reflects any change, not just renames.packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts (1)
90-98:⚠️ Potential issue | 🟠 MajorRead explicit
typebefore object-key traversal.
getEndpointType()can still infer the wrong type when an object key is encountered beforetype(for example{ properties: {}, type: 'jms' }), because the loop breaks early.Suggested fix
getEndpointType(endpoint: Record<string, unknown>): string | undefined { if (!endpoint) { return undefined; } + if (typeof endpoint.type === 'string') { + return endpoint.type; + } + let endpointType: string = ''; for (const key in endpoint) { - if (key === 'type') { - return endpoint[key] as string; - } - if (endpoint[key] !== undefined && typeof endpoint[key] === 'object') { endpointType = key; break; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts` around lines 90 - 98, In getEndpointType(), prefer the explicit type field before inferring from object keys: check if endpoint.type (or 'type' in endpoint) exists and is a string and return it immediately; only if there is no explicit type, proceed with the for-in traversal that sets endpointType when encountering an object value. Update the logic around the for-loop and the endpointType variable to ensure explicit types win.packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx (1)
37-46:⚠️ Potential issue | 🟠 MajorGate the auto-select effect and add dependencies.
This effect currently runs on every render while
endpointModelis unset, which can repeatedly invokeselectEndpoint()and cause duplicate side effects.Suggested fix
- useEffect(() => { - if (!isDefined(endpointModel)) { - selectEndpoint().then((selectedType) => { - if (selectedType) { - setEndpointType(selectedType); - setEndpointModel({}); - } - }); - } - }); + useEffect(() => { + if (isDefined(endpointModel) || !isDefined(endpointsSchema)) { + return; + } + + let active = true; + void selectEndpoint().then((selectedType) => { + if (active && selectedType) { + setEndpointType(selectedType); + setEndpointModel({}); + } + }); + + return () => { + active = false; + }; + }, [endpointModel, endpointsSchema, selectEndpoint]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx` around lines 37 - 46, The effect currently runs on every render while endpointModel is undefined and repeatedly calls selectEndpoint(); change the useEffect to only run when endpointModel changes by adding a dependency array (e.g., [endpointModel]) and gate the body with an early return if isDefined(endpointModel) so selectEndpoint() is invoked only once when endpointModel transitions from undefined to unset; keep the existing setEndpointType and setEndpointModel calls inside the guarded block (references: useEffect, endpointModel, selectEndpoint, setEndpointType, setEndpointModel).
🧹 Nitpick comments (4)
packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsx (1)
15-42: Remove the redundant fragment wrapper.The fragment encloses only one child (
FieldWrapper) and adds no value.♻️ Suggested cleanup
- return ( - <> - <FieldWrapper + return ( + <FieldWrapper propName={propName} required={required} title={schema.title} type="string" description={schema.description} defaultValue={schema.default?.toString()} > <InputGroup> <InputGroupItem isFill> <TextArea rows={5} type="text" role="textbox" id={propName} name={propName} placeholder={schema.default?.toString()} value={value} onChange={onFieldChange} aria-describedby={`${propName}-popover`} isDisabled={disabled} ref={textAreaRef} /> </InputGroupItem> </InputGroup> </FieldWrapper> - </> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsx` around lines 15 - 42, Remove the unnecessary React fragment that wraps FieldWrapper: locate the JSX in MessageBodyField where a fragment (<></>) contains only the FieldWrapper element and delete the fragment so FieldWrapper is the top-level returned JSX node; ensure imports and surrounding JSX remain unchanged and that propName, FieldWrapper, TextArea and textAreaRef usages are preserved.packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.test.tsx (2)
15-165: Consider extracting a shared render helper to reduce repetition.Most tests duplicate the same
SchemaProvider+ModelContextProvidersetup. A localrenderMessageBodyField(...)helper would make cases shorter and easier to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.test.tsx` around lines 15 - 165, Extract a small test helper (e.g., renderMessageBodyField) to centralize the repeated render pattern: wrap MessageBodyField with SchemaProvider and ModelContextProvider and accept args for schema, model, onPropertyChange, propName, and other props like required; replace explicit render(...) calls in tests with renderMessageBodyField({ schema: ..., model: ..., onPropertyChange: ..., propName: ROOT_PATH, required: true }) so tests reference the same wrapper and reduce duplication across tests that now call SchemaProvider, ModelContextProvider and MessageBodyField directly.
119-130: Test name and assertion are misaligned forrequiredbehavior.On Line 129, the assertion only checks title presence, so this test can still pass if
requiredhandling regresses. Please assert a required-specific signal (required marker/attribute) or rename the test to reflect what it actually verifies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.test.tsx` around lines 119 - 130, The test "should render FieldWrapper with required prop" for MessageBodyField only checks the title and doesn't verify the required behavior; update the test to either assert the required indicator/attribute (e.g., check for a visible required marker text, an aria-required=true on the input, or a CSS class used for required state) on the rendered FieldWrapper when calling <MessageBodyField propName={ROOT_PATH} required={true} /> or rename the test to "should render FieldWrapper title" if you intend to only check the title; locate the test referencing MessageBodyField, FieldWrapper and ROOT_PATH and add the specific expectation for the required marker/aria attribute or change the test name accordingly.packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts (1)
20-24: Avoid mutating catalog schema objects in place.Assigning
schema.nameandschema.titledirectly mutates objects sourced from the shared catalog map. Prefer cloning before enrichment to prevent cross-consumer state bleed.Suggested refactor
- const schema = endpointsCatalog[endpointKey].propertiesSchema as KaotoSchemaDefinition['schema']; + const sourceSchema = endpointsCatalog[endpointKey].propertiesSchema as KaotoSchemaDefinition['schema']; - if (schema) { - schema.name = endpointKey; - schema.title = endpointDefinition.title; - endpoints.push(schema); + if (sourceSchema) { + endpoints.push({ + ...sourceSchema, + name: endpointKey, + title: endpointDefinition.title, + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts` around lines 20 - 24, The code mutates shared catalog objects by setting schema.name and schema.title directly on endpointsCatalog[endpointKey].propertiesSchema; instead clone the schema before enrichment (e.g., create a shallow/deep clone of propertiesSchema into a new variable like enrichedSchema), set enrichedSchema.name = endpointKey and enrichedSchema.title = endpointDefinition.title, then push the cloned enrichedSchema into endpoints so the original endpointsCatalog and propertiesSchema remain unmodified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.test.tsx`:
- Around line 308-328: The test "should open modal when selecting a custom
endpoint name" currently only asserts the input value; update it to also assert
that the endpoint creation modal opens by verifying the dialog is present or the
modal title exists after selecting the custom value (use getInput(),
renderField(...) and the onCreate/onPropertyChange hooks to trigger the flow).
Specifically, after firing the change to 'customEndpoint' add an assertion such
as checking for a dialog role or the modal header text (e.g., using
getByRole('dialog', { name: /create endpoint/i }) or getByText(/create
endpoint/i)) or assert that the onCreate callback was invoked, so the test
actually confirms the modal opened.
In
`@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.test.ts`:
- Around line 177-275: Tests call endpointsHandler.getTestEntity() (typed Test |
undefined) and then dereference properties, causing TS18048; replace those
direct dereferences by narrowing the type first: add a small test helper (e.g.,
getDefinedTestEntity) that calls endpointsHandler.getTestEntity(), asserts it is
defined (expect(...).toBeDefined()) and returns the non-undefined Test, then
update all tests that use getTestEntity() (references:
endpointsHandler.getTestEntity, getDefinedEndpointsNameAndType, and testModel
setup) to call this helper so subsequent accesses to .endpoints are type-safe
under strict mode.
---
Duplicate comments:
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsx`:
- Around line 37-46: The effect currently runs on every render while
endpointModel is undefined and repeatedly calls selectEndpoint(); change the
useEffect to only run when endpointModel changes by adding a dependency array
(e.g., [endpointModel]) and gate the body with an early return if
isDefined(endpointModel) so selectEndpoint() is invoked only once when
endpointModel transitions from undefined to unset; keep the existing
setEndpointType and setEndpointModel calls inside the guarded block (references:
useEffect, endpointModel, selectEndpoint, setEndpointType, setEndpointModel).
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsx`:
- Around line 78-83: handleDelete currently mutates the incoming value array
in-place using splice before calling onChange, which can break change detection;
instead, create a new array copy (e.g., with [...value], value.slice(), or
value.filter((_, i) => i !== index)) and remove the item from that new array,
then call setItems(endpointsHandler.getDefinedEndpointsNameAndType(newArray))
and onChange(newArray) so handleDelete (the useCallback) never mutates the
original value reference.
- Around line 50-53: endpointsHandler.updateEndpoint(...) only triggers
setItems(...) when the endpoint was renamed, leaving the table stale on
type/property-only changes; modify EndpointsField so that after calling
endpointsHandler.updateEndpoint(type, model, prevName) you always refresh the UI
by calling setItems(endpointsHandler.getDefinedEndpointsNameAndType(value))
(i.e., move or duplicate the setItems call to run unconditionally after
updateEndpoint completes/succeeds) so the table reflects any change, not just
renames.
In
`@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts`:
- Around line 90-98: In getEndpointType(), prefer the explicit type field before
inferring from object keys: check if endpoint.type (or 'type' in endpoint)
exists and is a string and return it immediately; only if there is no explicit
type, proceed with the for-in traversal that sets endpointType when encountering
an object value. Update the logic around the for-loop and the endpointType
variable to ensure explicit types win.
---
Nitpick comments:
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.test.tsx`:
- Around line 15-165: Extract a small test helper (e.g., renderMessageBodyField)
to centralize the repeated render pattern: wrap MessageBodyField with
SchemaProvider and ModelContextProvider and accept args for schema, model,
onPropertyChange, propName, and other props like required; replace explicit
render(...) calls in tests with renderMessageBodyField({ schema: ..., model:
..., onPropertyChange: ..., propName: ROOT_PATH, required: true }) so tests
reference the same wrapper and reduce duplication across tests that now call
SchemaProvider, ModelContextProvider and MessageBodyField directly.
- Around line 119-130: The test "should render FieldWrapper with required prop"
for MessageBodyField only checks the title and doesn't verify the required
behavior; update the test to either assert the required indicator/attribute
(e.g., check for a visible required marker text, an aria-required=true on the
input, or a CSS class used for required state) on the rendered FieldWrapper when
calling <MessageBodyField propName={ROOT_PATH} required={true} /> or rename the
test to "should render FieldWrapper title" if you intend to only check the
title; locate the test referencing MessageBodyField, FieldWrapper and ROOT_PATH
and add the specific expectation for the required marker/aria attribute or
change the test name accordingly.
In
`@packages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsx`:
- Around line 15-42: Remove the unnecessary React fragment that wraps
FieldWrapper: locate the JSX in MessageBodyField where a fragment (<></>)
contains only the FieldWrapper element and delete the fragment so FieldWrapper
is the top-level returned JSX node; ensure imports and surrounding JSX remain
unchanged and that propName, FieldWrapper, TextArea and textAreaRef usages are
preserved.
In
`@packages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts`:
- Around line 20-24: The code mutates shared catalog objects by setting
schema.name and schema.title directly on
endpointsCatalog[endpointKey].propertiesSchema; instead clone the schema before
enrichment (e.g., create a shallow/deep clone of propertiesSchema into a new
variable like enrichedSchema), set enrichedSchema.name = endpointKey and
enrichedSchema.title = endpointDefinition.title, then push the cloned
enrichedSchema into endpoints so the original endpointsCatalog and
propertiesSchema remain unmodified.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b895ccc-8d96-4707-8d57-fe957de438ee
📒 Files selected for processing (14)
packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointFieldInner.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointModal.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/MessageBody/MessageBodyField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/custom-fields-factory.test.tspackages/ui/src/components/Visualization/Canvas/Form/fields/custom-fields-factory.tspackages/ui/src/models/visualization/flows/citrus-test-visual-entity.tspackages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.test.tspackages/ui/src/models/visualization/metadata/citrus/endpoints-entity-handler.ts
✅ Files skipped from review due to trivial changes (2)
- packages/ui/src/models/visualization/flows/citrus-test-visual-entity.ts
- packages/ui/src/components/Visualization/Canvas/Form/fields/custom-fields-factory.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ui/src/components/Visualization/Canvas/Form/fields/custom-fields-factory.ts
- packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointField.tsx
- packages/ui/src/components/Visualization/Canvas/Form/fields/EndpointField/EndpointsField.test.tsx
christophd
force-pushed
the
issue/add-citrus-custom-fields
branch
5 times, most recently
from
c144b44 to
95ac584
Compare
Member
|
@christophd I created this PR christophd#2 to help with the cleaning a bit |
christophd
force-pushed
the
issue/add-citrus-custom-fields
branch
from
95ac584 to
6a33ba5
Compare
Contributor
Author
|
@lordrip I think I have addressed all comments and also incorporated your cleanup changes. Could you please have another look, thx |
Member
|
@christophd there are a few Sonar issues remaining: https://sonarcloud.io/project/issues?id=KaotoIO_kaoto&pullRequest=3098&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true |
christophd
force-pushed
the
issue/add-citrus-custom-fields
branch
from
2448d26 to
2c632fa
Compare
Contributor
Author
|
@lordrip thx, I think (hope) I have addressed all findings now |
christophd
force-pushed
the
issue/add-citrus-custom-fields
branch
from
2c632fa to
2f0b6c9
Compare
christophd
force-pushed
the
issue/add-citrus-custom-fields
branch
from
2f0b6c9 to
cdcfd7a
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests