fix(Catalog): Select appropriate Catalog for Resource#3131
Draft
lordrip wants to merge 1 commit intoKaotoIO:mainfrom
Draft
fix(Catalog): Select appropriate Catalog for Resource#3131lordrip wants to merge 1 commit intoKaotoIO:mainfrom
lordrip wants to merge 1 commit intoKaotoIO:mainfrom
Conversation
Pick the catalog based on the resource's schema type so opening a Citrus test no longer loads a Camel catalog (and vice versa). - Add `detectSchemaType()` to infer the schema from source/path without constructing a full `KaotoResource`, avoiding a chicken-and-egg problem at provider mount time. - Add `COMPATIBLE_RUNTIMES_BY_SCHEMA_TYPE` as the single source of truth for resource→runtime compatibility, replacing the per-resource `getCompatibleRuntimes()` methods. - Persist the selected catalog per schema type via `catalog-storage.ts` (keyed map) instead of one global entry, with legacy-format detection to discard old values. - Simplify `IntegrationTypeSelector`, `RuntimeSelector`, and `NewFlow` now that runtime filtering lives in one place.
Contributor
📝 WalkthroughWalkthroughThis PR refactors runtime and catalog selection to centralize schema-type-based compatibility lookup. It removes per-resource Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3131 +/- ##
==========================================
- Coverage 91.74% 91.74% -0.01%
==========================================
Files 602 605 +3
Lines 22980 23017 +37
Branches 5406 5459 +53
==========================================
+ Hits 21083 21116 +33
- Misses 1787 1898 +111
+ Partials 110 3 -107 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
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/providers/runtime.provider.tsx (1)
69-103:⚠️ Potential issue | 🟠 MajorAdd AbortController and include
props.catalogUrlin useEffect dependencies to prevent stale fetch responses from overwriting active catalog state.The effect at lines 69–102 fetches when
schemaTypeorcompatibleRuntimesKeychanges, butprops.catalogUrlis used in the fetch call without being listed in dependencies (line 102). More critically, whenschemaTypechanges, an older in-flight request can complete after a new one starts, overwriting the catalog state for the wrong schema. Additionally, ifresolvedisundefined, the previousselectedCatalogStatepersists, showing stale data for a different schema type.Proposed fix
useEffect(() => { + const controller = new AbortController(); + let cancelled = false; + setLoadingStatus(LoadingStatus.Loading); + + fetch(props.catalogUrl, { signal: controller.signal }) .then((response) => { - setLoadingStatus(LoadingStatus.Loading); return response.json(); }) .then((library: CatalogLibrary) => { + if (cancelled) return; const persisted = getPersistedCatalog(schemaType); let resolved: CatalogLibraryEntry | undefined; if ( isDefined(persisted) && isCatalogCompatible(persisted, compatibleRuntimes) && library.definitions.some((c) => c.name === persisted.name) ) { resolved = persisted; } else { resolved = findCatalog(compatibleRuntimes, library); } setCatalogLibrary(library); - if (isDefined(resolved)) { - setSelectedCatalogState(resolved); - } + setSelectedCatalogState(resolved); }) .then(() => { + if (cancelled) return; setLoadingStatus(LoadingStatus.Loaded); }) .catch((error) => { + if (error?.name === 'AbortError' || cancelled) return; setErrorMessage(error.message); setLoadingStatus(LoadingStatus.Error); }); + return () => { + cancelled = true; + controller.abort(); + }; - // eslint-disable-next-line react-hooks/exhaustive-deps -}, [schemaType, compatibleRuntimesKey]); +}, [props.catalogUrl, schemaType, compatibleRuntimesKey]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/providers/runtime.provider.tsx` around lines 69 - 103, The effect that fetches props.catalogUrl inside the useEffect (dependencies currently [schemaType, compatibleRuntimesKey]) should be made abortable and include props.catalogUrl in its dependency array; create an AbortController, pass its signal to fetch(props.catalogUrl), and in the cleanup call controller.abort() to avoid stale responses overwriting state in the component that uses setCatalogLibrary and setSelectedCatalogState. Also, when a successful fetch returns but resolved is undefined, explicitly clear selectedCatalogState (e.g., setSelectedCatalogState(undefined)) so a previous selection does not persist for a different schemaType; finally, in the catch handler treat abort errors (ignored) differently from real errors before calling setErrorMessage and setLoadingStatus. Ensure references to getPersistedCatalog, isCatalogCompatible, findCatalog, compatibleRuntimesKey, and schemaType remain the same.
🧹 Nitpick comments (8)
packages/ui/src/components/Visualization/ContextToolbar/IntegrationTypeSelector/IntegrationTypeSelector.test.tsx (1)
164-200: Test logic is correct; consider renaming for clarity.The test correctly validates that
setCodeAndNotifyis called andsetSelectedCatalogis not called directly. However, the test description "should warn the user when selecting a flow type that previously changed the catalog" may be confusing since the current behavior no longer changes the catalog. Consider a name like "should call setCodeAndNotify without directly updating catalog when switching to incompatible flow type".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/ContextToolbar/IntegrationTypeSelector/IntegrationTypeSelector.test.tsx` around lines 164 - 200, Rename the test description string in the it(...) block for the test that asserts mockSetCodeAndNotify is called and mockSetSelectedCatalog is not called (currently "should warn the user when selecting a flow type that previously changed the catalog") to a clearer name such as "should call setCodeAndNotify without directly updating catalog when switching to incompatible flow type"; update only the first argument of the it(...) call that wraps the assertions (the test with expectations on mockSetCodeAndNotify and mockSetSelectedCatalog) so the test name accurately reflects the asserted behavior.packages/ui/src/models/camel/detect-schema-type.ts (1)
66-71: Redundant path type check inisCitrusShape.Line 67 checks
pathType === SourceSchemaType.Test, but this condition is already handled by the early return at lines 25-27 indetectSchemaType. WhenisCitrusShapeis called (line 31),pathTypeis guaranteed not to beTest.♻️ Suggested simplification
const isCitrusShape = (parsed: unknown, pathType?: SourceSchemaType): boolean => { - if (pathType === SourceSchemaType.Test) return true; if (!isDefined(parsed) || Array.isArray(parsed) || typeof parsed !== 'object') return false; const actions = (parsed as { actions?: unknown }).actions; return Array.isArray(actions); };Alternatively, if you want
isCitrusShapeto remain a standalone reusable utility, keep the check but add a comment explaining it's defensive for direct calls outsidedetectSchemaType.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/camel/detect-schema-type.ts` around lines 66 - 71, Remove the redundant pathType === SourceSchemaType.Test check inside isCitrusShape since detectSchemaType already handles the Test case before calling isCitrusShape; update the isCitrusShape implementation to only validate parsed (isDefined, object not array) and return Array.isArray((parsed as { actions?: unknown }).actions); alternatively, if you intend isCitrusShape to be a standalone defensive utility, keep the check but add a clear comment above isCitrusShape explaining it’s defensive for direct callers outside detectSchemaType.packages/ui/src/components/Visualization/ContextToolbar/IntegrationTypeSelector/IntegrationTypeSelectorToggle/IntegrationTypeSelectorToggle.tsx (1)
18-29: Consider narrowing the dependency array.Using
[props]as the dependency works but causes the callback to be recreated on any prop change. Since onlyprops.onSelectis used, narrowing to[props.onSelect]would be more precise.♻️ Suggested refinement
const onSelect = useCallback( (_event: MouseEvent | undefined, flowType: string | number | undefined) => { if (!flowType) return; const integrationType = sourceSchemaConfig.config[flowType as SourceSchemaType]; setIsOpen(false); if (integrationType !== undefined) { props.onSelect?.(flowType as SourceSchemaType); } }, - [props], + [props.onSelect], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/ContextToolbar/IntegrationTypeSelector/IntegrationTypeSelectorToggle/IntegrationTypeSelectorToggle.tsx` around lines 18 - 29, The useCallback for onSelect currently lists [props] which forces recreation on any prop change; narrow it to only the actual dependencies by replacing [props] with the specific references used inside the callback (e.g., props.onSelect and the source schema used to compute integrationType). Update the dependency array for onSelect to include props.onSelect and the source schema/config reference (for example sourceSchemaConfig.config or sourceSchemaConfig) so the callback only re-creates when those values change while leaving out unrelated prop changes.packages/ui/src/models/camel/detect-schema-type.test.ts (1)
88-90: Remove redundantundefinedargument.The second
undefinedargument is unnecessary since callingdetectSchemaType(undefined)with a single argument already defaults the second parameter toundefined.🧹 Suggested fix
it('returns Route for undefined source and no path', () => { - expect(detectSchemaType(undefined, undefined)).toEqual(SourceSchemaType.Route); + expect(detectSchemaType(undefined)).toEqual(SourceSchemaType.Route); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/camel/detect-schema-type.test.ts` around lines 88 - 90, The test case "returns Route for undefined source and no path" calls detectSchemaType with two undefineds; remove the redundant second argument so it calls detectSchemaType(undefined) (or simply detectSchemaType() if preferred) to rely on the function's default for the second parameter; update the assertion in the test file so it references detectSchemaType with a single argument and keeps the expected SourceSchemaType.Route result.packages/ui/src/utils/catalog-helper.test.ts (1)
20-22: Remove redundantundefinedargument.Similar to the other test file, the explicit
undefinedis unnecessary.🧹 Suggested fix
it('returns undefined when catalogLibrary is missing', () => { - expect(findCatalog(['Main'], undefined)).toBeUndefined(); + expect(findCatalog(['Main'])).toBeUndefined(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/utils/catalog-helper.test.ts` around lines 20 - 22, The test passes an explicit undefined to findCatalog which is redundant; update the test in catalog-helper.test.ts to call findCatalog(['Main']) (remove the second argument) so it mirrors other tests and relies on the default/omitted parameter behavior for the missing catalogLibrary.packages/ui/src/components/Visualization/ContextToolbar/RuntimeSelector/RuntimeSelector.test.tsx (1)
57-65: Conditional localStorage assertion may silently pass when persistence fails.The
if (raw !== null)guard means the test will pass even if nothing is written to localStorage. Based on the context snippet showing thatRuntimeProvider.setSelectedCatalogcallssetPersistedCatalog, the persistence should occur. Consider making this assertion unconditional to catch regression if persistence breaks.💡 Suggested fix
- const raw = localStorage.getItem(LocalStorageKeys.SelectedCatalog); - if (raw !== null) { - const parsed = JSON.parse(raw); - expect(parsed).toEqual(expect.any(Object)); - expect((parsed as Record<string, unknown>).name).toBeUndefined(); - expect((parsed as Record<string, unknown>).version).toBeUndefined(); - } + const raw = localStorage.getItem(LocalStorageKeys.SelectedCatalog); + expect(raw).not.toBeNull(); + const parsed = JSON.parse(raw!); + expect(parsed).toEqual(expect.any(Object)); + expect((parsed as Record<string, unknown>).name).toBeUndefined(); + expect((parsed as Record<string, unknown>).version).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/ContextToolbar/RuntimeSelector/RuntimeSelector.test.tsx` around lines 57 - 65, The test currently guards the localStorage read with "if (raw !== null)" which lets the test pass silently if persistence didn't occur; remove the conditional and assert that localStorage.getItem(LocalStorageKeys.SelectedCatalog) is non-null (e.g., expect(raw).not.toBeNull()), then parse JSON and assert parsed is an object and its name/version are undefined. Update the test around the RuntimeProvider.setSelectedCatalog / setPersistedCatalog call to unconditionally read and validate LocalStorageKeys.SelectedCatalog so regressions in persistence fail the test.packages/ui/src/providers/runtime.provider.test.tsx (1)
145-148: UseglobalThisinstead ofglobalfor better cross-environment compatibility.The SonarCloud hint is valid. Using
globalThisis the modern, standardized way to access the global object across different JavaScript environments.🧹 Suggested fix
- (global as unknown as { fetch: jest.Mock }).fetch = jest + (globalThis as unknown as { fetch: jest.Mock }).fetch = jest .fn() .mockResolvedValue({ json: () => Promise.resolve(mockCatalogLibrary) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/providers/runtime.provider.test.tsx` around lines 145 - 148, Replace usage of the non-standard global object in the test by changing the fetch mock assignment from (global as unknown as { fetch: jest.Mock }).fetch = jest.fn().mockResolvedValue(...) to use globalThis instead; update the statement that assigns the mock to the global fetch (the block that sets .fetch = jest.fn().mockResolvedValue({ json: () => Promise.resolve(mockCatalogLibrary) })) so it references (globalThis as unknown as { fetch: jest.Mock }).fetch to ensure cross-environment compatibility.packages/ui/src/utils/catalog-storage.ts (1)
39-42: Consider read-modify-write race conditions for concurrent tabs.
setPersistedCatalogperforms a non-atomic read-modify-write on localStorage. If the user has multiple tabs open and changes the catalog in both, the last write wins and may overwrite changes from the other tab. This is a minor concern for this use case but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/utils/catalog-storage.ts` around lines 39 - 42, setPersistedCatalog does a non-atomic read-modify-write using readCatalogMap and writeCatalogMap against localStorage which can cause lost updates across concurrent tabs; change the update strategy to an atomic/optimistic approach: add a version/timestamp field to the stored catalog (returned by readCatalogMap), then in setPersistedCatalog loop: read current, merge the new entry into the map, set a new version/timestamp, and attempt to write only if the version you read is still current (retry a few times on mismatch) or fall back to a merge strategy or notify the user; alternatively replace the localStorage backing with IndexedDB transactions or use the storage event to reconcile changes across tabs—reference the functions setPersistedCatalog, readCatalogMap and writeCatalogMap when implementing the change.
🤖 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/providers/runtime.provider.tsx`:
- Around line 79-83: The code currently matches a persisted catalog entry by
name only (library.definitions.some((c) => c.name === persisted.name)), which
can allow a persisted item whose version or runtime no longer exists; update the
condition that sets selectedCatalog to verify the full identity of the persisted
entry (e.g., match name AND version AND runtime/other identity fields) against
library.definitions and compatibleRuntimes and/or use an existing identity-check
helper (or extend isCatalogCompatible) so the predicate becomes something like:
library.definitions.some(c => c.name === persisted.name && c.version ===
persisted.version && c.runtime === persisted.runtime) before accepting persisted
as the selectedCatalog.
- Line 54: The current creation of compatibleRuntimesKey uses
Array.prototype.sort() with default semantics which Sonar flags; update the sort
call to use an explicit comparator (e.g., string localeCompare) when building
compatibleRuntimesKey from compatibleRuntimes so ordering is deterministic:
convert compatibleRuntimes to an array, call .sort((a, b) =>
a.localeCompare(b)), then .join('|') to form compatibleRuntimesKey (refer to the
compatibleRuntimesKey variable and compatibleRuntimes iterable).
---
Outside diff comments:
In `@packages/ui/src/providers/runtime.provider.tsx`:
- Around line 69-103: The effect that fetches props.catalogUrl inside the
useEffect (dependencies currently [schemaType, compatibleRuntimesKey]) should be
made abortable and include props.catalogUrl in its dependency array; create an
AbortController, pass its signal to fetch(props.catalogUrl), and in the cleanup
call controller.abort() to avoid stale responses overwriting state in the
component that uses setCatalogLibrary and setSelectedCatalogState. Also, when a
successful fetch returns but resolved is undefined, explicitly clear
selectedCatalogState (e.g., setSelectedCatalogState(undefined)) so a previous
selection does not persist for a different schemaType; finally, in the catch
handler treat abort errors (ignored) differently from real errors before calling
setErrorMessage and setLoadingStatus. Ensure references to getPersistedCatalog,
isCatalogCompatible, findCatalog, compatibleRuntimesKey, and schemaType remain
the same.
---
Nitpick comments:
In
`@packages/ui/src/components/Visualization/ContextToolbar/IntegrationTypeSelector/IntegrationTypeSelector.test.tsx`:
- Around line 164-200: Rename the test description string in the it(...) block
for the test that asserts mockSetCodeAndNotify is called and
mockSetSelectedCatalog is not called (currently "should warn the user when
selecting a flow type that previously changed the catalog") to a clearer name
such as "should call setCodeAndNotify without directly updating catalog when
switching to incompatible flow type"; update only the first argument of the
it(...) call that wraps the assertions (the test with expectations on
mockSetCodeAndNotify and mockSetSelectedCatalog) so the test name accurately
reflects the asserted behavior.
In
`@packages/ui/src/components/Visualization/ContextToolbar/IntegrationTypeSelector/IntegrationTypeSelectorToggle/IntegrationTypeSelectorToggle.tsx`:
- Around line 18-29: The useCallback for onSelect currently lists [props] which
forces recreation on any prop change; narrow it to only the actual dependencies
by replacing [props] with the specific references used inside the callback
(e.g., props.onSelect and the source schema used to compute integrationType).
Update the dependency array for onSelect to include props.onSelect and the
source schema/config reference (for example sourceSchemaConfig.config or
sourceSchemaConfig) so the callback only re-creates when those values change
while leaving out unrelated prop changes.
In
`@packages/ui/src/components/Visualization/ContextToolbar/RuntimeSelector/RuntimeSelector.test.tsx`:
- Around line 57-65: The test currently guards the localStorage read with "if
(raw !== null)" which lets the test pass silently if persistence didn't occur;
remove the conditional and assert that
localStorage.getItem(LocalStorageKeys.SelectedCatalog) is non-null (e.g.,
expect(raw).not.toBeNull()), then parse JSON and assert parsed is an object and
its name/version are undefined. Update the test around the
RuntimeProvider.setSelectedCatalog / setPersistedCatalog call to unconditionally
read and validate LocalStorageKeys.SelectedCatalog so regressions in persistence
fail the test.
In `@packages/ui/src/models/camel/detect-schema-type.test.ts`:
- Around line 88-90: The test case "returns Route for undefined source and no
path" calls detectSchemaType with two undefineds; remove the redundant second
argument so it calls detectSchemaType(undefined) (or simply detectSchemaType()
if preferred) to rely on the function's default for the second parameter; update
the assertion in the test file so it references detectSchemaType with a single
argument and keeps the expected SourceSchemaType.Route result.
In `@packages/ui/src/models/camel/detect-schema-type.ts`:
- Around line 66-71: Remove the redundant pathType === SourceSchemaType.Test
check inside isCitrusShape since detectSchemaType already handles the Test case
before calling isCitrusShape; update the isCitrusShape implementation to only
validate parsed (isDefined, object not array) and return Array.isArray((parsed
as { actions?: unknown }).actions); alternatively, if you intend isCitrusShape
to be a standalone defensive utility, keep the check but add a clear comment
above isCitrusShape explaining it’s defensive for direct callers outside
detectSchemaType.
In `@packages/ui/src/providers/runtime.provider.test.tsx`:
- Around line 145-148: Replace usage of the non-standard global object in the
test by changing the fetch mock assignment from (global as unknown as { fetch:
jest.Mock }).fetch = jest.fn().mockResolvedValue(...) to use globalThis instead;
update the statement that assigns the mock to the global fetch (the block that
sets .fetch = jest.fn().mockResolvedValue({ json: () =>
Promise.resolve(mockCatalogLibrary) })) so it references (globalThis as unknown
as { fetch: jest.Mock }).fetch to ensure cross-environment compatibility.
In `@packages/ui/src/utils/catalog-helper.test.ts`:
- Around line 20-22: The test passes an explicit undefined to findCatalog which
is redundant; update the test in catalog-helper.test.ts to call
findCatalog(['Main']) (remove the second argument) so it mirrors other tests and
relies on the default/omitted parameter behavior for the missing catalogLibrary.
In `@packages/ui/src/utils/catalog-storage.ts`:
- Around line 39-42: setPersistedCatalog does a non-atomic read-modify-write
using readCatalogMap and writeCatalogMap against localStorage which can cause
lost updates across concurrent tabs; change the update strategy to an
atomic/optimistic approach: add a version/timestamp field to the stored catalog
(returned by readCatalogMap), then in setPersistedCatalog loop: read current,
merge the new entry into the map, set a new version/timestamp, and attempt to
write only if the version you read is still current (retry a few times on
mismatch) or fall back to a merge strategy or notify the user; alternatively
replace the localStorage backing with IndexedDB transactions or use the storage
event to reconcile changes across tabs—reference the functions
setPersistedCatalog, readCatalogMap and writeCatalogMap when implementing 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: d499874b-c596-44a9-af7c-1c8e4153277b
📒 Files selected for processing (27)
packages/ui/src/components/Visualization/ContextToolbar/IntegrationTypeSelector/ChangeIntegrationTypeModal/ChangeIntegrationTypeModal.tsxpackages/ui/src/components/Visualization/ContextToolbar/IntegrationTypeSelector/IntegrationTypeSelector.test.tsxpackages/ui/src/components/Visualization/ContextToolbar/IntegrationTypeSelector/IntegrationTypeSelector.tsxpackages/ui/src/components/Visualization/ContextToolbar/IntegrationTypeSelector/IntegrationTypeSelectorToggle/IntegrationTypeSelectorToggle.tsxpackages/ui/src/components/Visualization/ContextToolbar/RuntimeSelector/RuntimeSelector.test.tsxpackages/ui/src/components/Visualization/ContextToolbar/RuntimeSelector/RuntimeSelector.tsxpackages/ui/src/components/Visualization/EmptyState/FlowType/NewFlow.test.tsxpackages/ui/src/components/Visualization/EmptyState/FlowType/NewFlow.tsxpackages/ui/src/models/camel/camel-k-resource.tspackages/ui/src/models/camel/camel-route-resource.test.tspackages/ui/src/models/camel/camel-route-resource.tspackages/ui/src/models/camel/compatible-runtimes.test.tspackages/ui/src/models/camel/compatible-runtimes.tspackages/ui/src/models/camel/detect-schema-type.test.tspackages/ui/src/models/camel/detect-schema-type.tspackages/ui/src/models/camel/kamelet-binding-resource.test.tspackages/ui/src/models/camel/kamelet-resource.test.tspackages/ui/src/models/camel/pipe-resource.test.tspackages/ui/src/models/citrus/citrus-test-resource.test.tspackages/ui/src/models/citrus/citrus-test-resource.tspackages/ui/src/models/kaoto-resource.tspackages/ui/src/providers/runtime.provider.test.tsxpackages/ui/src/providers/runtime.provider.tsxpackages/ui/src/utils/catalog-helper.test.tspackages/ui/src/utils/catalog-helper.tspackages/ui/src/utils/catalog-storage.test.tspackages/ui/src/utils/catalog-storage.ts
💤 Files with no reviewable changes (10)
- packages/ui/src/models/camel/kamelet-resource.test.ts
- packages/ui/src/models/camel/camel-k-resource.ts
- packages/ui/src/models/camel/camel-route-resource.test.ts
- packages/ui/src/models/kaoto-resource.ts
- packages/ui/src/models/camel/pipe-resource.test.ts
- packages/ui/src/models/camel/kamelet-binding-resource.test.ts
- packages/ui/src/components/Visualization/EmptyState/FlowType/NewFlow.tsx
- packages/ui/src/models/citrus/citrus-test-resource.test.ts
- packages/ui/src/models/camel/camel-route-resource.ts
- packages/ui/src/models/citrus/citrus-test-resource.ts
| const [selectedCatalog, setSelectedCatalog] = useState<CatalogLibraryEntry | undefined>(localSelectedCatalog); | ||
| const schemaType = useMemo(() => detectSchemaType(sourceCode, path), [sourceCode, path]); | ||
| const compatibleRuntimes = COMPATIBLE_RUNTIMES_BY_SCHEMA_TYPE[schemaType]; | ||
| const compatibleRuntimesKey = [...compatibleRuntimes].sort().join('|'); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "runtime.provider.tsx" | head -20Repository: KaotoIO/kaoto
Length of output: 105
🏁 Script executed:
cat -n packages/ui/src/providers/runtime.provider.tsx | sed -n '40,70p'Repository: KaotoIO/kaoto
Length of output: 1654
🏁 Script executed:
cat -n packages/ui/src/providers/runtime.provider.tsx | head -80Repository: KaotoIO/kaoto
Length of output: 3853
🏁 Script executed:
find . -type f -name "*compatible-runtimes*"Repository: KaotoIO/kaoto
Length of output: 169
🏁 Script executed:
cat packages/ui/src/models/camel/compatible-runtimes.tsRepository: KaotoIO/kaoto
Length of output: 893
🏁 Script executed:
find . -type f \( -name ".eslintrc*" -o -name "sonar*" -o -name "tsconfig.json" \) | grep -E "(packages/ui|root)" | head -10Repository: KaotoIO/kaoto
Length of output: 118
🏁 Script executed:
cat packages/ui/tsconfig.json 2>/dev/null | head -50Repository: KaotoIO/kaoto
Length of output: 739
🏁 Script executed:
cat tsconfig.json 2>/dev/null | head -50Repository: KaotoIO/kaoto
Length of output: 39
🏁 Script executed:
find . -type f -name ".eslintrc*" -o -name "eslintrc*" | head -10Repository: KaotoIO/kaoto
Length of output: 39
🏁 Script executed:
find . -type f \( -name "sonar-project.properties" -o -name ".sonarrc" \) 2>/dev/nullRepository: KaotoIO/kaoto
Length of output: 39
🏁 Script executed:
cat -n packages/ui/src/providers/runtime.provider.tsx | sed -n '50,58p'Repository: KaotoIO/kaoto
Length of output: 615
Add an explicit comparator for sort() to satisfy the Sonar failure.
Line 54 currently relies on default string sort semantics; Sonar flags this as a blocker. Make ordering explicit.
💡 Proposed fix
-const compatibleRuntimesKey = [...compatibleRuntimes].sort().join('|');
+const compatibleRuntimesKey = [...compatibleRuntimes].sort((a, b) => a.localeCompare(b)).join('|');📝 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
| const compatibleRuntimesKey = [...compatibleRuntimes].sort().join('|'); | |
| const compatibleRuntimesKey = [...compatibleRuntimes].sort((a, b) => a.localeCompare(b)).join('|'); |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 54-54: Provide a compare function to avoid sorting elements alphabetically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/providers/runtime.provider.tsx` at line 54, The current
creation of compatibleRuntimesKey uses Array.prototype.sort() with default
semantics which Sonar flags; update the sort call to use an explicit comparator
(e.g., string localeCompare) when building compatibleRuntimesKey from
compatibleRuntimes so ordering is deterministic: convert compatibleRuntimes to
an array, call .sort((a, b) => a.localeCompare(b)), then .join('|') to form
compatibleRuntimesKey (refer to the compatibleRuntimesKey variable and
compatibleRuntimes iterable).
Comment on lines
+79
to
+83
| if ( | ||
| isDefined(persisted) && | ||
| isCatalogCompatible(persisted, compatibleRuntimes) && | ||
| library.definitions.some((c) => c.name === persisted.name) | ||
| ) { |
Contributor
There was a problem hiding this comment.
Match persisted catalog by full identity, not only name.
At Line 82, validating only name can accept a persisted entry whose version (or runtime) no longer exists in the current library. That can leave selectedCatalog pointing to an unavailable catalog.
💡 Proposed fix
- library.definitions.some((c) => c.name === persisted.name)
+ library.definitions.some(
+ (c) =>
+ c.name === persisted.name &&
+ c.version === persisted.version &&
+ c.runtime === persisted.runtime,
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/providers/runtime.provider.tsx` around lines 79 - 83, The
code currently matches a persisted catalog entry by name only
(library.definitions.some((c) => c.name === persisted.name)), which can allow a
persisted item whose version or runtime no longer exists; update the condition
that sets selectedCatalog to verify the full identity of the persisted entry
(e.g., match name AND version AND runtime/other identity fields) against
library.definitions and compatibleRuntimes and/or use an existing identity-check
helper (or extend isCatalogCompatible) so the predicate becomes something like:
library.definitions.some(c => c.name === persisted.name && c.version ===
persisted.version && c.runtime === persisted.runtime) before accepting persisted
as the selectedCatalog.
Member
Author
|
Turning it into draft since I can't continue working on this at the moment |
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.
Changes
Pick the catalog based on the resource's schema type so opening a Citrus test no longer loads a Camel catalog (and vice-versa).
detectSchemaType()to infer the schema from source/path without constructing a fullKaotoResource, avoiding a chicken-and-egg problem at provider mount time.COMPATIBLE_RUNTIMES_BY_SCHEMA_TYPEas the single source of truth for resource→runtime compatibility, replacing the per-resourcegetCompatibleRuntimes()methods.catalog-storage.ts(keyed map) instead of one global entry, with legacy-format detection to discard old values.IntegrationTypeSelector,RuntimeSelector, andNewFlownow that runtime filtering lives in one place.Summary by CodeRabbit
New Features
Bug Fixes
Refactor