refactor: migrate CamelCatalogService catalog maps to async DynamicCatalogRegistry - #3447
Draft
lordrip wants to merge 4 commits into
Draft
refactor: migrate CamelCatalogService catalog maps to async DynamicCatalogRegistry#3447lordrip wants to merge 4 commits into
lordrip wants to merge 4 commits into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR migrates ChangesAsync validation and expression parsing migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lordrip
marked this pull request as draft
July 7, 2026 13:37
![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: 2
🧹 Nitpick comments (2)
packages/ui/src/components/Visualization/Custom/hooks/use-node-validation-text.hook.test.ts (1)
16-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
vizNodeidentity swap.Tests cover initial resolve, in-place edits via
lastUpdate, no-node, and rejection, but not switchingrenderHookto a differentvizNodeinstance mid-lifecycle — the scenario tied to the stale-state gap flagged inuse-node-validation-text.hook.ts.🤖 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/hooks/use-node-validation-text.hook.test.ts` around lines 16 - 73, The `useNodeValidationText` test suite is missing coverage for swapping to a different `vizNode` instance during the hook lifecycle, which can hide stale-state bugs. Add a test in `use-node-validation-text.hook.test.ts` using `renderHook` and `rerender` to replace the original node with a new `IVisualizationNode` object, then assert the hook re-runs `getNodeValidationText` and updates `result.current` accordingly. Use the existing `createNode`, `renderHook`, and `useNodeValidationText` setup so the new test targets the same identity-change path as the hook logic.packages/ui/src/models/visualization/flows/support/validators/model-validation.service.ts (1)
71-117: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential
awaitin independent loops could be parallelized.The
oneOfloop (Lines 72-74) and the property-iteration recursion (Lines 84-117)awaiteach recursive call one at a time even though the branches are logically independent of each other (unlike theanyOfloop, which intentionally threadsparsedModelsequentially). Since this validation runs per-node and potentially on every render via the upcominguseNodeValidationTexthook, sequential awaiting of independent, catalog-backed recursive calls adds unnecessary latency for schemas with many properties/oneOf branches.♻️ Suggested refactor using Promise.all for independent branches
- if (Array.isArray(resolvedSchema.oneOf)) { - for (const oneOfSchema of resolvedSchema.oneOf) { - answer.push(...(await this.validateRequiredProperties(oneOfSchema, model, parentPath, definitions))); - } - } + if (Array.isArray(resolvedSchema.oneOf)) { + const oneOfResults = await Promise.all( + resolvedSchema.oneOf.map((oneOfSchema) => + this.validateRequiredProperties(oneOfSchema, model, parentPath, definitions), + ), + ); + oneOfResults.forEach((result) => answer.push(...result)); + }🤖 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/support/validators/model-validation.service.ts` around lines 71 - 117, The validation in model-validation.service.ts is awaiting independent recursive branches one by one in validateRequiredProperties, which adds avoidable latency. Refactor the resolvedSchema.oneOf handling and the schema.properties iteration to build and await those independent recursive calls in parallel (for example, via Promise.all), while keeping the anyOf-style sequential logic unchanged if it depends on parsedModel updates. Use validateRequiredProperties, resolveSchemaWithRef, and the property iteration logic to locate the affected paths.
🤖 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/fields/ExpressionField/ExpressionField.tsx`:
- Around line 49-64: The ExpressionField render gate is left stuck when
ExpressionService.parseExpressionModel rejects because the catch path only logs
the error and never resolves the parsed state. Update the useEffect in
ExpressionField to handle failures by clearing or setting parsed state on the
error path as well, so isModelParsed is always settled and the component can
render instead of remaining null. Use the existing
ExpressionService.parseExpressionModel, setParsedModel, and setIsModelParsed
flow as the place to fix both success and failure handling.
In
`@packages/ui/src/components/Visualization/Custom/hooks/use-node-validation-text.hook.ts`:
- Around line 15-36: The hook useNodeValidationText is leaving stale validation
text visible when vizNode changes or becomes undefined because validationText is
only updated after getNodeValidationText resolves. Reset validationText
synchronously at the start of the effect whenever vizNode identity changes (and
before kicking off the async lookup) so CustomNode and CustomGroupExpanded don’t
briefly render the previous node’s warning state. Keep the existing cancellation
logic, and make sure the reset happens in the useEffect that depends on vizNode
and lastUpdate.
---
Nitpick comments:
In
`@packages/ui/src/components/Visualization/Custom/hooks/use-node-validation-text.hook.test.ts`:
- Around line 16-73: The `useNodeValidationText` test suite is missing coverage
for swapping to a different `vizNode` instance during the hook lifecycle, which
can hide stale-state bugs. Add a test in `use-node-validation-text.hook.test.ts`
using `renderHook` and `rerender` to replace the original node with a new
`IVisualizationNode` object, then assert the hook re-runs
`getNodeValidationText` and updates `result.current` accordingly. Use the
existing `createNode`, `renderHook`, and `useNodeValidationText` setup so the
new test targets the same identity-change path as the hook logic.
In
`@packages/ui/src/models/visualization/flows/support/validators/model-validation.service.ts`:
- Around line 71-117: The validation in model-validation.service.ts is awaiting
independent recursive branches one by one in validateRequiredProperties, which
adds avoidable latency. Refactor the resolvedSchema.oneOf handling and the
schema.properties iteration to build and await those independent recursive calls
in parallel (for example, via Promise.all), while keeping the anyOf-style
sequential logic unchanged if it depends on parsedModel updates. Use
validateRequiredProperties, resolveSchemaWithRef, and the property iteration
logic to locate the affected paths.
🪄 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: f3b7ffdb-367d-4074-aebf-f26fb8964d48
📒 Files selected for processing (34)
packages/ui/src/components/Visualization/Canvas/Canvas.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/ExpressionField/ExpressionField.test.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/ExpressionField/ExpressionField.tsxpackages/ui/src/components/Visualization/Canvas/Form/fields/ExpressionField/expression.service.test.tspackages/ui/src/components/Visualization/Canvas/Form/fields/ExpressionField/expression.service.tspackages/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/components/Visualization/Custom/hooks/use-node-validation-text.hook.test.tspackages/ui/src/components/Visualization/Custom/hooks/use-node-validation-text.hook.tspackages/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-catalog.service.test.tspackages/ui/src/models/visualization/flows/camel-catalog.service.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-route-configuration-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/pipe-visual-entity.test.tspackages/ui/src/models/visualization/flows/pipe-visual-entity.tspackages/ui/src/models/visualization/flows/support/validators/model-validation.service.test.tspackages/ui/src/models/visualization/flows/support/validators/model-validation.service.tspackages/ui/src/models/visualization/visualization-node.test.tspackages/ui/src/models/visualization/visualization-node.ts
💤 Files with no reviewable changes (2)
- packages/ui/src/models/visualization/flows/camel-catalog.service.ts
- packages/ui/src/models/visualization/flows/camel-catalog.service.test.ts
Comment on lines
+49
to
+64
| useEffect(() => { | ||
| let cancelled = false; | ||
| ExpressionService.parseExpressionModel(originalModel) | ||
| .then((model) => { | ||
| if (!cancelled) { | ||
| setParsedModel(model); | ||
| setIsModelParsed(true); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| console.error('Failed to parse expression model:', error); | ||
| }); | ||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [originalModel]); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Field permanently hidden if parseExpressionModel rejects.
The .catch() handler only logs the error and never sets isModelParsed. Since rendering returns null until isModelParsed is true (Line 66-68), a rejected parse leaves the field permanently blank with no way to recover or surface the failure to the user. Based on a related learning, async UI state (here the "parsed" gate) should be resolved consistently on both success and failure paths rather than left stuck.
🐛 Suggested fix: unblock rendering on error too
.catch((error) => {
- console.error('Failed to parse expression model:', error);
+ if (!cancelled) {
+ console.error('Failed to parse expression model:', error);
+ setIsModelParsed(true);
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| let cancelled = false; | |
| ExpressionService.parseExpressionModel(originalModel) | |
| .then((model) => { | |
| if (!cancelled) { | |
| setParsedModel(model); | |
| setIsModelParsed(true); | |
| } | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to parse expression model:', error); | |
| }); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [originalModel]); | |
| useEffect(() => { | |
| let cancelled = false; | |
| ExpressionService.parseExpressionModel(originalModel) | |
| .then((model) => { | |
| if (!cancelled) { | |
| setParsedModel(model); | |
| setIsModelParsed(true); | |
| } | |
| }) | |
| .catch((error) => { | |
| if (!cancelled) { | |
| console.error('Failed to parse expression model:', error); | |
| setIsModelParsed(true); | |
| } | |
| }); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [originalModel]); |
🤖 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/fields/ExpressionField/ExpressionField.tsx`
around lines 49 - 64, The ExpressionField render gate is left stuck when
ExpressionService.parseExpressionModel rejects because the catch path only logs
the error and never resolves the parsed state. Update the useEffect in
ExpressionField to handle failures by clearing or setting parsed state on the
error path as well, so isModelParsed is always settled and the component can
render instead of remaining null. Use the existing
ExpressionService.parseExpressionModel, setParsedModel, and setIsModelParsed
flow as the place to fix both success and failure handling.
Source: Learnings
Comment on lines
+15
to
+36
| export const useNodeValidationText = (vizNode?: IVisualizationNode): string | undefined => { | ||
| const [validationText, setValidationText] = useState<string | undefined>(undefined); | ||
| const lastUpdate = vizNode?.lastUpdate; | ||
|
|
||
| useEffect(() => { | ||
| let cancelled = false; | ||
| vizNode | ||
| ?.getNodeValidationText() | ||
| .then((text) => { | ||
| if (!cancelled) setValidationText(text); | ||
| }) | ||
| .catch((error) => { | ||
| console.error('Failed to get node validation text:', error); | ||
| }); | ||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| // `lastUpdate` is intentionally included so the text re-resolves when the node is edited in place. | ||
| }, [vizNode, lastUpdate]); | ||
|
|
||
| return validationText; | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Stale validation text can persist briefly when vizNode identity changes.
When vizNode switches to a different node instance (or becomes undefined), vizNode?.getNodeValidationText() short-circuits the whole optional chain, so setValidationText is never called until the new promise resolves. Consumers (CustomNode/CustomGroupExpanded) will render the previous node's warning state for a render or two, which is incorrect if elements/components get reused for a different vizNode. It self-corrects once the new promise settles, but the transient wrong state could still surface a misleading warning badge.
🔧 Proposed fix: reset text synchronously when the node identity changes
useEffect(() => {
let cancelled = false;
+ setValidationText(undefined);
vizNode
?.getNodeValidationText()
.then((text) => {
if (!cancelled) setValidationText(text);
})
.catch((error) => {
console.error('Failed to get node validation text:', error);
});
return () => {
cancelled = true;
};
// `lastUpdate` is intentionally included so the text re-resolves when the node is edited in place.
}, [vizNode, lastUpdate]);Note this also resets on lastUpdate bumps for the same node (brief flicker to undefined before re-resolving); if that's undesirable, key the reset off a separate effect scoped only to vizNode identity.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const useNodeValidationText = (vizNode?: IVisualizationNode): string | undefined => { | |
| const [validationText, setValidationText] = useState<string | undefined>(undefined); | |
| const lastUpdate = vizNode?.lastUpdate; | |
| useEffect(() => { | |
| let cancelled = false; | |
| vizNode | |
| ?.getNodeValidationText() | |
| .then((text) => { | |
| if (!cancelled) setValidationText(text); | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to get node validation text:', error); | |
| }); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| // `lastUpdate` is intentionally included so the text re-resolves when the node is edited in place. | |
| }, [vizNode, lastUpdate]); | |
| return validationText; | |
| }; | |
| export const useNodeValidationText = (vizNode?: IVisualizationNode): string | undefined => { | |
| const [validationText, setValidationText] = useState<string | undefined>(undefined); | |
| const lastUpdate = vizNode?.lastUpdate; | |
| useEffect(() => { | |
| let cancelled = false; | |
| setValidationText(undefined); | |
| vizNode | |
| ?.getNodeValidationText() | |
| .then((text) => { | |
| if (!cancelled) setValidationText(text); | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to get node validation text:', error); | |
| }); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| // `lastUpdate` is intentionally included so the text re-resolves when the node is edited in place. | |
| }, [vizNode, lastUpdate]); | |
| return validationText; | |
| }; |
🤖 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/hooks/use-node-validation-text.hook.ts`
around lines 15 - 36, The hook useNodeValidationText is leaving stale validation
text visible when vizNode changes or becomes undefined because validationText is
only updated after getNodeValidationText resolves. Reset validationText
synchronously at the start of the effect whenever vizNode identity changes (and
before kicking off the async lookup) so CustomNode and CustomGroupExpanded don’t
briefly render the previous node’s warning state. Keep the existing cancellation
logic, and make sure the reset happens in the useEffect that depends on vizNode
and lastUpdate.
lordrip
force-pushed
the
chore/migrate-getNodeValidationText
branch
from
July 8, 2026 15:10
08a362b to
0164b2a
Compare
Completes the sync->async migration by consuming the now-async getNodeValidationText / parseExpressionModel APIs in the React render layer and removing the unused sync catalog map helpers. - ExpressionField: parse the expression model in an effect and gate rendering on an `isModelParsed` flag. `useOneOfField` latches its selected schema on its mount render only, so mounting the inner field before the async parse resolves would latch an empty selection that never recovers (no expression controls / clear button). The flag stays true afterwards so later model changes re-parse without remounting. - CustomNode / CustomGroupExpanded: read validation text via useState/useEffect over the async getNodeValidationText, with a .catch that logs failures. - CamelCatalogService: delete the now-unused getLanguageMap, getDataFormatMap and getLoadBalancerMap helpers. - Tests: await the async initial render where needed, switch getNodeValidationText mocks to mockResolvedValue, spy on the graph prototype in Canvas.test so fit(80) is captured regardless of the fromModel instance swap or rAF timing, and refresh the snapshot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CustomNode and CustomGroupExpanded carried an identical useState/useEffect block resolving the async getNodeValidationText. Consolidate it into a shared `useNodeValidationText` hook. This also fixes a staleness bug introduced when getNodeValidationText became async: the effect was keyed on the node reference only, but edits mutate the node in place and bump `lastUpdate` without changing the reference, so the warning icon never refreshed after an edit. The hook keys on `lastUpdate` as well, restoring the pre-migration behaviour where validation was recomputed on every change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lordrip
force-pushed
the
chore/migrate-getNodeValidationText
branch
from
July 13, 2026 08:15
0164b2a to
97ad93c
Compare
|
Contributor
|
This pull request is stale because it has been open for 14 days with no activity. It will be closed if there is no activity in the next 14 days. |
github-actions
Bot
added
the
stale
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
1 participant
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.
Context
Refactors
CamelCatalogServiceper the issue: removes the unused sync catalog-map helpers and migrates the language-map consumers to the asyncDynamicCatalogRegistryAPI, propagating the resulting async change up to the React render layer.Changes
CamelCatalogService: delete unusedgetDataFormatMapandgetLoadBalancerMap, and removegetLanguageMap.ExpressionService:getLanguageNames,parseExpressionModel, andupdateExpressionFromModelare nowasync, sourcing languages fromDynamicCatalogRegistry.get().getCatalog(CatalogKind.Language)?.getAll().ModelValidationService.validateNodeStatus/validateRequiredPropertiesandgetNodeValidationText(interfaces + all visual-entity implementations +VisualizationNode) are nowasync.ExpressionFieldparses the model in an effect and gates rendering on anisModelParsedflag, so the third-partyuseOneOfFieldmounts with the resolved model (otherwise it latches an empty selection).useNodeValidationTexthook consumed byCustomNodeandCustomGroupExpanded, removing duplicated async boilerplate. The hook keys onlastUpdate, fixing a regression where the validation warning would not refresh after in-place node edits.awaitthe now-async APIs; added a unit test foruseNodeValidationText(including re-validation on edit).Closes #3442
Summary by CodeRabbit
New Features
Bug Fixes
Tests