feat(dyn catalog): schema migration to async#3209
Draft
PVinaches wants to merge 1 commit into
Draft
Conversation
Contributor
📝 WalkthroughWalkthroughConvert synchronous schema/validation APIs to async Promises, switch catalog lookups to DynamicCatalogRegistry with a shared test mock helper, update entity/service implementations, and make UI/components/tests await async schema/validation flows. ChangesAsync Schema and Validation Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
PVinaches
force-pushed
the
schema-migration
branch
2 times, most recently
from
5619d89 to
ce48020
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3209 +/- ##
==========================================
+ Coverage 92.23% 92.24% +0.01%
==========================================
Files 633 634 +1
Lines 24472 24527 +55
Branches 5796 5806 +10
==========================================
+ Hits 22572 22626 +54
- Misses 1790 1791 +1
Partials 110 110 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PVinaches
commented
May 13, 2026
| const result = CamelComponentSchemaService.getSchema({ processorName: 'from' as keyof ProcessorDefinition }); | ||
| it('should build the appropriate schema for entities', async () => { | ||
| const result = await CamelComponentSchemaService.getSchema({ | ||
| processorName: 'from' as keyof ProcessorDefinition, |
Member
Author
There was a problem hiding this comment.
This sonarcloud issue will be tackled separately as it is repeated in several places along the code (outside this PR).
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/ui/src/models/visualization/flows/camel-route-configuration-visual-entity.test.ts (1)
21-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove redundant CamelCatalogService setup and teardown.
The entity's
getNodeSchemamethod usesDynamicCatalogRegistry.get().getEntity()(line 101 of the entity file), notCamelCatalogService. ThesetupDynamicCatalogRegistryMockcall already provides the necessary catalog mocking for the Entity kind. Lines 23 and 29 (CamelCatalogService.setCatalogKeyandclearCatalogs) are unnecessary and should be removed to match the pattern used incamel-rest-configuration-visual-entity.test.tsand other migrated test files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/models/visualization/flows/camel-route-configuration-visual-entity.test.ts` around lines 21 - 30, Remove the redundant CamelCatalogService setup/teardown: delete the call to CamelCatalogService.setCatalogKey(...) in beforeAll and CamelCatalogService.clearCatalogs() in afterAll; keep the existing setupDynamicCatalogRegistryMock(catalogsMap) call since getNodeSchema uses DynamicCatalogRegistry.get().getEntity(), not CamelCatalogService, so no further changes are required to the test flow in camel-route-configuration-visual-entity.test.ts.packages/ui/src/models/visualization/flows/pipe-visual-entity.test.ts (1)
141-146:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
async/awaiton thisgetNodeSchematest case.This test was skipped during the async migration.
KameletSchemaService.getKameletCatalogEntryis invoked synchronously to produce the awaited Promise, so the spy assertion still passes — but the returned Promise is never awaited, leaving a floating Promise that can leak into subsequent tests, and the case is inconsistent with the surrounding test cases (lines 126–139) which were all converted.🔧 Proposed fix
- it('should return the node schema', () => { + it('should return the node schema', async () => { const spy = jest.spyOn(KameletSchemaService, 'getKameletCatalogEntry'); - pipeVisualEntity.getNodeSchema('source'); + await pipeVisualEntity.getNodeSchema('source'); expect(spy).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/models/visualization/flows/pipe-visual-entity.test.ts` around lines 141 - 146, The test "should return the node schema" leaves a floating Promise — make the test async and await the call to pipeVisualEntity.getNodeSchema('source') (or return the Promise) so the KameletSchemaService.getKameletCatalogEntry spy assertion runs after the Promise resolves; update the it(...) to be async and await the getNodeSchema call to match the other async tests.packages/ui/src/models/visualization/flows/citrus-test-visual-entity.test.ts (1)
152-159:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
async/awaiton thisgetNodeSchematest case.This test was skipped during the async migration. While
extractTestActionNameis invoked synchronously before anyawaitinsidegetNodeSchema, leaving the returned Promise unawaited creates a floating promise that can leak into other tests and is inconsistent with the surrounding test cases (lines 122–141) which were all converted to async/await.🔧 Proposed fix
- it('should return the component schema', () => { + it('should return the component schema', async () => { const spy = jest.spyOn(CitrusTestSchemaService, 'extractTestActionName'); spy.mockReturnValueOnce('print'); - citrusTestEntity.getNodeSchema('actions.0.print'); + await citrusTestEntity.getNodeSchema('actions.0.print'); expect(spy).toHaveBeenCalledWith('actions.0.print'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/models/visualization/flows/citrus-test-visual-entity.test.ts` around lines 152 - 159, The test for getNodeSchema is missing async/await and leaves a floating Promise; update the spec to declare the it callback as async and await citrusTestEntity.getNodeSchema('actions.0.print'), keeping the spy on CitrusTestSchemaService.extractTestActionName and the same expect; this ensures the Promise returned by getNodeSchema is awaited and prevents cross-test leaks.packages/ui/src/models/visualization/flows/pipe-visual-entity.ts (1)
323-334:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncomplete migration:
getRootPipeSchemastill uses synchronousCamelCatalogService.The PR objective is to shift schema retrieval to
DynamicCatalogRegistryto enable lazy-loading.KameletVisualEntity.getRootKameletSchemawas migrated (lines 153–157), butgetRootPipeSchemahere still callsCamelCatalogService.getComponent(CatalogKind.Entity, 'PipeConfiguration')synchronously. This keeps the Pipe root path bound to the eagerly-loaded catalog and undermines the lazy-loading goal.The test at lines 136–139 doesn't catch this: it only asserts
resultis defined — the empty-schema fallback ({}when the Entity catalog isn't seeded) passes that assertion. Additionally,DynamicCatalogRegistryis mocked to handle onlyCatalogKind.Kamelet(lines 33–40), notCatalogKind.Entity.🔧 Proposed fix
async getNodeSchema(path?: string): Promise<KaotoSchemaDefinition['schema'] | undefined> { if (!path) return undefined; if (path === this.getRootPath()) { - return this.getRootPipeSchema(); + return await this.getRootPipeSchema(); } ... } - private getRootPipeSchema(): KaotoSchemaDefinition['schema'] { - const rootPipeDefinition = CamelCatalogService.getComponent(CatalogKind.Entity, 'PipeConfiguration'); + private async getRootPipeSchema(): Promise<KaotoSchemaDefinition['schema']> { + const rootPipeDefinition = await DynamicCatalogRegistry.get().getEntity( + CatalogKind.Entity, + 'PipeConfiguration', + ); if (rootPipeDefinition === undefined) return {} as unknown as KaotoSchemaDefinition['schema']; let schema = {} as unknown as KaotoSchemaDefinition['schema']; if (rootPipeDefinition.propertiesSchema !== undefined) { schema = rootPipeDefinition.propertiesSchema; } return schema; }Remove the
CamelCatalogServiceimport (line 28) and extend the test mock to handleCatalogKind.Entityentries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/models/visualization/flows/pipe-visual-entity.ts` around lines 323 - 334, getRootPipeSchema still calls the synchronous CamelCatalogService; replace that with the lazy-loading DynamicCatalogRegistry lookup used by KameletVisualEntity. In getRootPipeSchema() use DynamicCatalogRegistry (the same method used in getRootKameletSchema) to fetch the CatalogKind.Entity 'PipeConfiguration' entry, handle a missing entry by returning an empty schema as before, and remove the now-unused CamelCatalogService import. Also update the unit test mock to include CatalogKind.Entity => 'PipeConfiguration' so the test exercises the lazy-loaded path rather than relying on the empty-schema fallback.
🧹 Nitpick comments (5)
packages/ui/src/components/Visualization/Custom/Node/CustomNode.tsx (1)
134-148: ⚡ Quick winConsider adding error handling for the async validation text load.
Similar to the schema loading pattern, this promise chain lacks a
.catch()handler. IfgetNodeValidationText()rejects, it will cause an unhandled promise rejection.🛡️ Proposed enhancement
useEffect(() => { let cancelled = false; if (vizNode) { vizNode.getNodeValidationText().then((text) => { if (!cancelled) { setValidationText(text); } - }); + }).catch((error) => { + console.error('Failed to load validation text:', error); + if (!cancelled) { + setValidationText(undefined); + } + }); } return () => { cancelled = true; }; }, [vizNode, lastUpdate]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/Visualization/Custom/Node/CustomNode.tsx` around lines 134 - 148, The useEffect in CustomNode.tsx that calls vizNode.getNodeValidationText() lacks error handling; update the async chain to add a .catch() (or try/catch if you convert to async/await) so rejected promises are handled — inside the catch log the error (or send to your logger) and optionally setValidationText('') or some safe default only if not cancelled; keep the cancelled guard and ensure you still return the cleanup that sets cancelled = true.packages/ui/src/components/Visualization/Canvas/Form/CanvasFormBody.test.tsx (1)
33-36: 💤 Low valueExplicit
cleanup()call may be redundant.React Testing Library automatically calls
cleanup()after each test in modern versions. While calling it explicitly is harmless, it's typically unnecessary. Thejest.restoreAllMocks()call is good practice and should be kept.Also applies to: 167-170, 295-298
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/Visualization/Canvas/Form/CanvasFormBody.test.tsx` around lines 33 - 36, The afterEach blocks in CanvasFormBody.test.tsx currently call cleanup() and jest.restoreAllMocks(); remove the explicit cleanup() call (keep jest.restoreAllMocks()) because React Testing Library auto-cleans between tests; update each afterEach occurrence that matches the pattern (e.g., the one shown and the other two similar blocks) to only call jest.restoreAllMocks(), and if cleanup was imported from '@testing-library/react' and is now unused, remove that import.packages/ui/src/pages/RestDslEditor/RestDslEditorPage.tsx (1)
40-56: ⚡ Quick winConsider adding error handling for the async schema load.
The promise chain lacks a
.catch()handler. IfgetNodeSchema()rejects, it will cause an unhandled promise rejection.🛡️ Proposed enhancement
useEffect(() => { let cancelled = false; if (selectedEntity && selectedElement?.modelPath) { selectedEntity.getNodeSchema(selectedElement.modelPath).then((loadedSchema) => { if (!cancelled) { setSchema(loadedSchema); } - }); + }).catch((error) => { + console.error('Failed to load schema:', error); + if (!cancelled) { + setSchema(undefined); + } + }); } else { setSchema(undefined); } return () => { cancelled = true; }; }, [selectedEntity, selectedElement?.modelPath]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/pages/RestDslEditor/RestDslEditorPage.tsx` around lines 40 - 56, The async call in the useEffect (when selectedEntity && selectedElement?.modelPath) calls selectedEntity.getNodeSchema(...) but has no error handling; update the effect to attach a .catch() (or try/catch if you convert to async/await) to handle rejections and avoid unhandled promise rejections, and inside the handler only call setSchema(loadedSchema) if cancelled is false; also setSchema(undefined) or log the error (e.g., console.error or a UI notification) in the catch so failures are surfaced; refer to the existing useEffect, selectedEntity.getNodeSchema, setSchema, cancelled and selectedElement?.modelPath when making the change.packages/ui/src/components/Visualization/Canvas/Form/CanvasFormBody.tsx (1)
20-32: ⚡ Quick winConsider adding error handling for the async schema load.
The promise chain lacks a
.catch()handler, so ifgetNodeSchema()rejects, it will result in an unhandled promise rejection. While the underlying method may be designed not to reject, defensive error handling would improve resilience.🛡️ Proposed enhancement with error handling
useEffect(() => { let cancelled = false; vizNode.getNodeSchema().then((loadedSchema) => { if (!cancelled) { setSchema(loadedSchema); } - }); + }).catch((error) => { + console.error('Failed to load node schema:', error); + if (!cancelled) { + setSchema(undefined); + } + }); return () => { cancelled = true; }; }, [vizNode]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/Visualization/Canvas/Form/CanvasFormBody.tsx` around lines 20 - 32, The async call to vizNode.getNodeSchema inside the useEffect has no error handling; modify the effect so rejections are caught (either append .catch(...) to the promise or convert the callback to an async IIFE with try/catch), ensure you check the cancelled flag before updating state, and handle errors by logging (or setting schema to a safe fallback) instead of allowing an unhandled rejection; update the block that calls vizNode.getNodeSchema and the setSchema usage accordingly.packages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsx (1)
74-88: ⚡ Quick winConsider adding error handling for the async validation text load.
The promise chain lacks a
.catch()handler. IfgetNodeValidationText()rejects, it will result in an unhandled promise rejection.🛡️ Proposed enhancement
useEffect(() => { let cancelled = false; if (groupVizNode) { groupVizNode.getNodeValidationText().then((text) => { if (!cancelled) { setValidationText(text); } - }); + }).catch((error) => { + console.error('Failed to load validation text:', error); + if (!cancelled) { + setValidationText(undefined); + } + }); } return () => { cancelled = true; }; }, [groupVizNode, lastUpdate]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsx` around lines 74 - 88, The effect calling groupVizNode.getNodeValidationText() lacks error handling; wrap the async call (in the useEffect that references groupVizNode, lastUpdate, and uses cancelled and setValidationText) with a try/catch (or append .catch()) so rejections are handled, and inside the catch log or handle the error (e.g., via console.error or a component logger) while still respecting the cancelled guard before calling setValidationText; ensure getNodeValidationText() errors do not produce unhandled promise rejections.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/components/Visualization/Canvas/Form/CanvasFormBody.tsx`:
- Around line 20-32: The effect in CanvasFormBody uses vizNode.getNodeSchema()
and setSchema but only lists [vizNode] as its dependency, so schema won't reload
when the node's internal model changes; update the useEffect dependency array to
include vizNode.lastUpdate (i.e., change the effect dependencies from [vizNode]
to [vizNode, vizNode.lastUpdate]) so the effect re-runs when the node's model
updates, ensuring vizNode.getNodeSchema() is called again and setSchema is
refreshed.
---
Outside diff comments:
In
`@packages/ui/src/models/visualization/flows/camel-route-configuration-visual-entity.test.ts`:
- Around line 21-30: Remove the redundant CamelCatalogService setup/teardown:
delete the call to CamelCatalogService.setCatalogKey(...) in beforeAll and
CamelCatalogService.clearCatalogs() in afterAll; keep the existing
setupDynamicCatalogRegistryMock(catalogsMap) call since getNodeSchema uses
DynamicCatalogRegistry.get().getEntity(), not CamelCatalogService, so no further
changes are required to the test flow in
camel-route-configuration-visual-entity.test.ts.
In
`@packages/ui/src/models/visualization/flows/citrus-test-visual-entity.test.ts`:
- Around line 152-159: The test for getNodeSchema is missing async/await and
leaves a floating Promise; update the spec to declare the it callback as async
and await citrusTestEntity.getNodeSchema('actions.0.print'), keeping the spy on
CitrusTestSchemaService.extractTestActionName and the same expect; this ensures
the Promise returned by getNodeSchema is awaited and prevents cross-test leaks.
In `@packages/ui/src/models/visualization/flows/pipe-visual-entity.test.ts`:
- Around line 141-146: The test "should return the node schema" leaves a
floating Promise — make the test async and await the call to
pipeVisualEntity.getNodeSchema('source') (or return the Promise) so the
KameletSchemaService.getKameletCatalogEntry spy assertion runs after the Promise
resolves; update the it(...) to be async and await the getNodeSchema call to
match the other async tests.
In `@packages/ui/src/models/visualization/flows/pipe-visual-entity.ts`:
- Around line 323-334: getRootPipeSchema still calls the synchronous
CamelCatalogService; replace that with the lazy-loading DynamicCatalogRegistry
lookup used by KameletVisualEntity. In getRootPipeSchema() use
DynamicCatalogRegistry (the same method used in getRootKameletSchema) to fetch
the CatalogKind.Entity 'PipeConfiguration' entry, handle a missing entry by
returning an empty schema as before, and remove the now-unused
CamelCatalogService import. Also update the unit test mock to include
CatalogKind.Entity => 'PipeConfiguration' so the test exercises the lazy-loaded
path rather than relying on the empty-schema fallback.
---
Nitpick comments:
In
`@packages/ui/src/components/Visualization/Canvas/Form/CanvasFormBody.test.tsx`:
- Around line 33-36: The afterEach blocks in CanvasFormBody.test.tsx currently
call cleanup() and jest.restoreAllMocks(); remove the explicit cleanup() call
(keep jest.restoreAllMocks()) because React Testing Library auto-cleans between
tests; update each afterEach occurrence that matches the pattern (e.g., the one
shown and the other two similar blocks) to only call jest.restoreAllMocks(), and
if cleanup was imported from '@testing-library/react' and is now unused, remove
that import.
In `@packages/ui/src/components/Visualization/Canvas/Form/CanvasFormBody.tsx`:
- Around line 20-32: The async call to vizNode.getNodeSchema inside the
useEffect has no error handling; modify the effect so rejections are caught
(either append .catch(...) to the promise or convert the callback to an async
IIFE with try/catch), ensure you check the cancelled flag before updating state,
and handle errors by logging (or setting schema to a safe fallback) instead of
allowing an unhandled rejection; update the block that calls
vizNode.getNodeSchema and the setSchema usage accordingly.
In
`@packages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsx`:
- Around line 74-88: The effect calling groupVizNode.getNodeValidationText()
lacks error handling; wrap the async call (in the useEffect that references
groupVizNode, lastUpdate, and uses cancelled and setValidationText) with a
try/catch (or append .catch()) so rejections are handled, and inside the catch
log or handle the error (e.g., via console.error or a component logger) while
still respecting the cancelled guard before calling setValidationText; ensure
getNodeValidationText() errors do not produce unhandled promise rejections.
In `@packages/ui/src/components/Visualization/Custom/Node/CustomNode.tsx`:
- Around line 134-148: The useEffect in CustomNode.tsx that calls
vizNode.getNodeValidationText() lacks error handling; update the async chain to
add a .catch() (or try/catch if you convert to async/await) so rejected promises
are handled — inside the catch log the error (or send to your logger) and
optionally setValidationText('') or some safe default only if not cancelled;
keep the cancelled guard and ensure you still return the cleanup that sets
cancelled = true.
In `@packages/ui/src/pages/RestDslEditor/RestDslEditorPage.tsx`:
- Around line 40-56: The async call in the useEffect (when selectedEntity &&
selectedElement?.modelPath) calls selectedEntity.getNodeSchema(...) but has no
error handling; update the effect to attach a .catch() (or try/catch if you
convert to async/await) to handle rejections and avoid unhandled promise
rejections, and inside the handler only call setSchema(loadedSchema) if
cancelled is false; also setSchema(undefined) or log the error (e.g.,
console.error or a UI notification) in the catch so failures are surfaced; refer
to the existing useEffect, selectedEntity.getNodeSchema, setSchema, cancelled
and selectedElement?.modelPath 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: b3670476-6a39-400e-8827-0f3e6862114a
⛔ Files ignored due to path filters (2)
packages/ui/src/components/Visualization/Canvas/Form/__snapshots__/CanvasForm.test.tsx.snapis excluded by!**/*.snappackages/ui/src/models/visualization/flows/__snapshots__/camel-route-configuration-visual-entity.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (38)
packages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/CanvasFormBody.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/CanvasFormBody.tsxpackages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.test.tsxpackages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNode.test.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNode.tsxpackages/ui/src/models/visualization/base-visual-entity.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.test.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.tspackages/ui/src/models/visualization/flows/camel-error-handler-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-error-handler-visual-entity.tspackages/ui/src/models/visualization/flows/camel-intercept-from-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-intercept-send-to-endpoint-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-intercept-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-on-completion-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-rest-configuration-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-rest-configuration-visual-entity.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.tspackages/ui/src/models/visualization/flows/camel-route-configuration-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-route-configuration-visual-entity.tspackages/ui/src/models/visualization/flows/camel-route-visual-entity.test.tspackages/ui/src/models/visualization/flows/citrus-test-visual-entity.test.tspackages/ui/src/models/visualization/flows/citrus-test-visual-entity.tspackages/ui/src/models/visualization/flows/dynamic-catalog-registry-mock.tspackages/ui/src/models/visualization/flows/kamelet-visual-entity.test.tspackages/ui/src/models/visualization/flows/kamelet-visual-entity.tspackages/ui/src/models/visualization/flows/pipe-visual-entity.test.tspackages/ui/src/models/visualization/flows/pipe-visual-entity.tspackages/ui/src/models/visualization/flows/support/camel-component-schema.service.test.tspackages/ui/src/models/visualization/flows/support/camel-component-schema.service.tspackages/ui/src/models/visualization/flows/support/kamelet-schema.service.tspackages/ui/src/models/visualization/flows/support/validators/model-validation.service.test.tspackages/ui/src/models/visualization/visualization-node.test.tspackages/ui/src/models/visualization/visualization-node.tspackages/ui/src/pages/RestDslEditor/RestDslEditorPage.test.tsxpackages/ui/src/pages/RestDslEditor/RestDslEditorPage.tsx
PVinaches
force-pushed
the
schema-migration
branch
from
ce48020 to
b577ace
Compare
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/models/visualization/flows/camel-on-completion-visual-entity.test.ts (1)
91-100: ⚡ Quick winRestore the spy after the test to maintain test isolation.
The spy created on line 92 is never cleaned up, which can cause test pollution and potentially affect other tests. Consider adding
validateNodeStatusSpy.mockRestore()after the assertion, or using anafterEachhook to restore all mocks.🧹 Proposed fix to add spy cleanup
it('should delegate the validation text to the ModelValidationService', async () => { const validateNodeStatusSpy = jest.spyOn(ModelValidationService, 'validateNodeStatus'); const onCompletionVisualEntity = new CamelOnCompletionVisualEntity({ onCompletion: { id: 'id', mode: 'AfterConsumer' }, }); await onCompletionVisualEntity.getNodeValidationText('onCompletion'); expect(validateNodeStatusSpy).toHaveBeenCalled(); + validateNodeStatusSpy.mockRestore(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/models/visualization/flows/camel-on-completion-visual-entity.test.ts` around lines 91 - 100, The test creates a Jest spy on ModelValidationService.validateNodeStatus but never restores it, which can leak into other tests; update the test (CamelOnCompletionVisualEntity.getNodeValidationText spec) to restore the spy by calling validateNodeStatusSpy.mockRestore() immediately after the expect, or add a shared afterEach hook that calls jest.restoreAllMocks() to ensure the spy is cleaned up and test isolation is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/ui/src/models/visualization/flows/camel-on-completion-visual-entity.test.ts`:
- Around line 91-100: The test creates a Jest spy on
ModelValidationService.validateNodeStatus but never restores it, which can leak
into other tests; update the test
(CamelOnCompletionVisualEntity.getNodeValidationText spec) to restore the spy by
calling validateNodeStatusSpy.mockRestore() immediately after the expect, or add
a shared afterEach hook that calls jest.restoreAllMocks() to ensure the spy is
cleaned up and test isolation is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7339e024-351c-466a-bcc9-544fc755cfcb
⛔ Files ignored due to path filters (2)
packages/ui/src/components/Visualization/Canvas/Form/__snapshots__/CanvasForm.test.tsx.snapis excluded by!**/*.snappackages/ui/src/models/visualization/flows/__snapshots__/camel-route-configuration-visual-entity.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (38)
packages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/CanvasFormBody.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/CanvasFormBody.tsxpackages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.test.tsxpackages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNode.test.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNode.tsxpackages/ui/src/models/visualization/base-visual-entity.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.test.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.tspackages/ui/src/models/visualization/flows/camel-error-handler-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-error-handler-visual-entity.tspackages/ui/src/models/visualization/flows/camel-intercept-from-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-intercept-send-to-endpoint-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-intercept-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-on-completion-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-rest-configuration-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-rest-configuration-visual-entity.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.tspackages/ui/src/models/visualization/flows/camel-route-configuration-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-route-configuration-visual-entity.tspackages/ui/src/models/visualization/flows/camel-route-visual-entity.test.tspackages/ui/src/models/visualization/flows/citrus-test-visual-entity.test.tspackages/ui/src/models/visualization/flows/citrus-test-visual-entity.tspackages/ui/src/models/visualization/flows/dynamic-catalog-registry-mock.tspackages/ui/src/models/visualization/flows/kamelet-visual-entity.test.tspackages/ui/src/models/visualization/flows/kamelet-visual-entity.tspackages/ui/src/models/visualization/flows/pipe-visual-entity.test.tspackages/ui/src/models/visualization/flows/pipe-visual-entity.tspackages/ui/src/models/visualization/flows/support/camel-component-schema.service.test.tspackages/ui/src/models/visualization/flows/support/camel-component-schema.service.tspackages/ui/src/models/visualization/flows/support/kamelet-schema.service.tspackages/ui/src/models/visualization/flows/support/validators/model-validation.service.test.tspackages/ui/src/models/visualization/visualization-node.test.tspackages/ui/src/models/visualization/visualization-node.tspackages/ui/src/pages/RestDslEditor/RestDslEditorPage.test.tsxpackages/ui/src/pages/RestDslEditor/RestDslEditorPage.tsx
✅ Files skipped from review due to trivial changes (2)
- packages/ui/src/models/visualization/flows/camel-intercept-from-visual-entity.test.ts
- packages/ui/src/components/Visualization/Custom/Node/CustomNode.test.tsx
🚧 Files skipped from review as they are similar to previous changes (34)
- packages/ui/src/components/Visualization/Custom/Node/CustomNode.tsx
- packages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.test.tsx
- packages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsx
- packages/ui/src/models/visualization/base-visual-entity.ts
- packages/ui/src/models/visualization/flows/abstract-camel-visual-entity.test.ts
- packages/ui/src/models/visualization/flows/camel-error-handler-visual-entity.ts
- packages/ui/src/models/visualization/flows/support/kamelet-schema.service.ts
- packages/ui/src/models/visualization/flows/camel-error-handler-visual-entity.test.ts
- packages/ui/src/models/visualization/flows/camel-intercept-send-to-endpoint-visual-entity.test.ts
- packages/ui/src/models/visualization/flows/camel-route-configuration-visual-entity.test.ts
- packages/ui/src/models/visualization/flows/camel-intercept-visual-entity.test.ts
- packages/ui/src/pages/RestDslEditor/RestDslEditorPage.test.tsx
- packages/ui/src/components/Visualization/Canvas/Form/CanvasFormBody.tsx
- packages/ui/src/models/visualization/flows/pipe-visual-entity.test.ts
- packages/ui/src/models/visualization/flows/camel-route-visual-entity.test.ts
- packages/ui/src/models/visualization/flows/support/camel-component-schema.service.test.ts
- packages/ui/src/models/visualization/visualization-node.ts
- packages/ui/src/models/visualization/flows/dynamic-catalog-registry-mock.ts
- packages/ui/src/models/visualization/flows/pipe-visual-entity.ts
- packages/ui/src/models/visualization/flows/kamelet-visual-entity.ts
- packages/ui/src/models/visualization/flows/camel-rest-configuration-visual-entity.ts
- packages/ui/src/models/visualization/flows/abstract-camel-visual-entity.ts
- packages/ui/src/models/visualization/visualization-node.test.ts
- packages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsx
- packages/ui/src/models/visualization/flows/camel-rest-configuration-visual-entity.test.ts
- packages/ui/src/components/Visualization/Canvas/Form/CanvasFormBody.test.tsx
- packages/ui/src/models/visualization/flows/camel-rest-visual-entity.ts
- packages/ui/src/models/visualization/flows/support/validators/model-validation.service.test.ts
- packages/ui/src/models/visualization/flows/citrus-test-visual-entity.test.ts
- packages/ui/src/models/visualization/flows/kamelet-visual-entity.test.ts
- packages/ui/src/models/visualization/flows/support/camel-component-schema.service.ts
- packages/ui/src/models/visualization/flows/camel-route-configuration-visual-entity.ts
- packages/ui/src/models/visualization/flows/camel-rest-visual-entity.test.ts
- packages/ui/src/pages/RestDslEditor/RestDslEditorPage.tsx
igarashitm
approved these changes
May 14, 2026
shivamG640
reviewed
May 14, 2026
| const catalogLookup = CamelCatalogService.getCatalogLookup(camelElementLookup.componentName); | ||
| const componentSchema: KaotoSchemaDefinition['schema'] = | ||
| catalogLookup.definition?.propertiesSchema ?? ({} as unknown as KaotoSchemaDefinition['schema']); | ||
| const catalogLookup = await this.getAsyncCatalogLookup(camelElementLookup.componentName); |
Contributor
There was a problem hiding this comment.
Not sure if we need to handle CamelCatalogService.getCatalogLookup() this way. In my XML parsing PR, I left this line of code unchanged.
shivamG640
reviewed
May 14, 2026
| * Async method to return whether this is a Camel Component or a Kamelet. | ||
| * Uses the dynamic catalog registry for async catalog access. | ||
| */ | ||
| private static async getAsyncCatalogLookup(componentName: string): Promise<{ |
Contributor
There was a problem hiding this comment.
Not the right place for this function, in my opinion. Better to keep the CamelCatalogService.getCatalogLookup() for now
shivamG640
reviewed
May 14, 2026
| } | ||
|
|
||
| getNodeSchema(path?: string): KaotoSchemaDefinition['schema'] | undefined { | ||
| async getNodeSchema(path?: string): Promise<KaotoSchemaDefinition['schema'] | undefined> { |
Contributor
There was a problem hiding this comment.
Is there something missed here?
there is no async code in the body of the function.
shivamG640
reviewed
May 14, 2026
| } | ||
|
|
||
| getNodeSchema(path?: string): KaotoSchemaDefinition['schema'] | undefined { | ||
| async getNodeSchema(path?: string): Promise<KaotoSchemaDefinition['schema'] | undefined> { |
Contributor
There was a problem hiding this comment.
This function and it sub-functions need to be implemented as async as well.
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
3 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.
Migrates the dynamic catalog schema retrieval system from synchronous to asynchronous
operations, enabling lazy-loading and improved performance for catalog lookups.
Key Changes:
synchronous to async methods across IVisualizationNode and BaseVisualEntity interfaces
catalog entity retrieval, replacing direct CamelCatalogService calls
handle async schema loading using React useEffect hooks with proper cleanup
Impact: All schema and validation lookups now load asynchronously, preparing the codebase for
on-demand catalog loading and reducing initial bundle size.
fix: #3195
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor