Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Catalog landing UI and styles, refactors catalog listing into a fetch-and-render wrapper with loading/error/empty handling, extracts an RBAC-aware create-item button, updates repository creation for catalog-sync, extends navigation to accept URL params, updates app links and RBAC types, and introduces English translation keys. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
📝 Coding Plan
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 |
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsx (1)
124-124:⚠️ Potential issue | 🟡 MinorMissing
typefield when adding a new resource sync.When a new resource sync is pushed, the
typefield is omitted. While theuseEffect(lines 28-32) will reset it ifallowedRSTypesis defined, ifallowedRSTypesisundefined, the field remains unset, causing inconsistency with the initial values inutils.tswhich defaults toResourceSyncType.ResourceSyncTypeFleet.🔧 Proposed fix
- onClick={() => arrayHelpers.push({ name: '', path: '', targetRevision: '' } as ResourceSyncFormValue)} + onClick={() => arrayHelpers.push({ name: '', path: '', targetRevision: '', type: ResourceSyncType.ResourceSyncTypeFleet } as ResourceSyncFormValue)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsx` at line 124, The onClick push in CreateResourceSyncsForm is adding { name, path, targetRevision } without the required type, causing inconsistent defaults; update the arrayHelpers.push call in CreateResourceSyncsForm to include a default type (e.g., ResourceSyncType.ResourceSyncTypeFleet or the same default used in utils.ts) so new ResourceSyncFormValue entries always have a type field, and ensure the symbol allowedRSTypes/useEffect behavior remains unchanged.
🧹 Nitpick comments (3)
libs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsx (1)
38-39: Redundant double negation.
isCatalogSyncis already a boolean (line 39). The!!on line 114 is unnecessary.✏️ Suggested simplification
- if (!!isCatalogSync) { + if (isCatalogSync) {Also applies to: 114-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsx` around lines 38 - 39, The variable isCatalogSync (created via useSearchParams in CreateRepository) is already a boolean, so remove redundant double-negation usages: keep a single boolean conversion at the source (either const isCatalogSync = !!searchParams.get('isCatalogSync') or const isCatalogSync = Boolean(searchParams.get('isCatalogSync'))) and remove any additional `!!isCatalogSync` occurrences later in the file; search for and replace references that use `!!isCatalogSync` to use `isCatalogSync` directly (affecting code paths that reference isCatalogSync around the CreateRepository component).libs/ui-components/src/components/Catalog/CatalogPageToolbar.tsx (1)
16-28: Consider returningnullinstead offalsefrom component render.When
canCreateis false, line 22 returnsfalsewhich can cause React warnings in some contexts. Prefer returningnullexplicitly for cleaner component output.♻️ Suggested fix
export const CreateCatalogItemBtn = () => { const { checkPermissions } = usePermissionsContext(); const [canCreate] = checkPermissions(permissions); const { t } = useTranslation(); const navigate = useNavigate(); + + if (!canCreate) { + return null; + } + return ( - !!canCreate && ( - <Button variant="primary" onClick={() => navigate(ROUTE.CATALOG_ADD_ITEM)}> - {t('Create item')} - </Button> - ) + <Button variant="primary" onClick={() => navigate(ROUTE.CATALOG_ADD_ITEM)}> + {t('Create item')} + </Button> ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/CatalogPageToolbar.tsx` around lines 16 - 28, The CreateCatalogItemBtn component currently returns a boolean when the user lacks permission (via the canCreate value from checkPermissions), which can produce React warnings; update the render logic in CreateCatalogItemBtn to explicitly return null when !canCreate (e.g., replace the &&-expression with a conditional that returns the Button when canCreate is true and null otherwise), keeping the usePermissionsContext().checkPermissions and navigate/t translation usage unchanged.libs/ui-components/src/components/Catalog/CatalogLandingPage.tsx (1)
52-52: Consider adding keyboard accessibility to CardHeader.The
CardHeaderhasonClickandrole="button"but lacks keyboard event handlers (onKeyDown/onKeyUpfor Enter/Space). This could affect keyboard navigation.♿ Suggested enhancement
- <CardHeader onClick={onExpand} role="button" className="fctl-catalog-landing__header"> + <CardHeader + onClick={onExpand} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + onExpand(); + } + }} + role="button" + tabIndex={0} + className="fctl-catalog-landing__header" + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/CatalogLandingPage.tsx` at line 52, The CardHeader element currently has onClick and role="button" but is missing keyboard accessibility; update the CardHeader (fctl-catalog-landing__header) to be focusable (add tabIndex={0}) and add an onKeyDown handler that calls the same onExpand function when Enter or Space is pressed (handle Space by preventing default to avoid page scroll), ensuring the keyboard activation mirrors the onClick behavior while keeping role="button".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/i18n/locales/en/translation.json`:
- Line 310: The translation string in CatalogLandingPage.tsx contains a typo
("You dont" instead of "You don't") which caused incorrect entries in
libs/i18n/locales/en/translation.json; open CatalogLandingPage.tsx, correct the
three source strings (the instances producing the keys around "You dont have
permissions to create image builds" and the two other occurrences noted), then
regenerate the localization bundle by running npm run i18n so the corrected keys
are emitted into libs/i18n/locales/en/translation.json rather than editing that
JSON file directly.
In `@libs/ui-components/src/components/Catalog/CatalogLandingPage.tsx`:
- Line 140: In CatalogLandingPage (component CatalogLandingPage) fix the typo in
the tooltip translation keys used in the content={t('...')} calls by changing
"You dont have permissions to create image builds" to "You don't have
permissions to create image builds" (and apply the same change to the other two
content={t('...')} occurrences in this file); ensure the apostrophe is correctly
escaped or switch to double-quoted string for the t(...) literal so JSX/string
quoting is valid.
- Line 107: The Card is using the setter function instead of the state value
(isExpanded={!!setExpandedItem}), so replace that with the correct state or
remove the prop: either change isExpanded to use the expansion state variable
(e.g., isExpanded={!!expandedItem} or isExpanded={expandedItem ===
'build-images'} if you track per-card keys) or drop the Card's isExpanded prop
entirely and let CardSection manage expansion; locate the Card with id
"build-images-card" and update the isExpanded usage and any related logic that
reads/writes setExpandedItem accordingly.
In
`@libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx`:
- Around line 434-440: getInitValues currently doesn't accept
mustUseResourceSyncs so values.useResourceSyncs can be false even when the
checkbox is shown checked via options?.mustUseResourceSyncs; update the
getInitValues signature in utils.ts to include mustUseResourceSyncs in its
options type and ensure the returned initial state sets useResourceSyncs: true
when options.mustUseResourceSyncs is true (otherwise fall back to existing
canUseResourceSyncs/canUseRSs logic), so the form value matches the disabled
checked checkbox used in CreateRepositoryForm (where isChecked/onChange
reference values.useResourceSyncs and options?.mustUseResourceSyncs).
---
Outside diff comments:
In
`@libs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsx`:
- Line 124: The onClick push in CreateResourceSyncsForm is adding { name, path,
targetRevision } without the required type, causing inconsistent defaults;
update the arrayHelpers.push call in CreateResourceSyncsForm to include a
default type (e.g., ResourceSyncType.ResourceSyncTypeFleet or the same default
used in utils.ts) so new ResourceSyncFormValue entries always have a type field,
and ensure the symbol allowedRSTypes/useEffect behavior remains unchanged.
---
Nitpick comments:
In `@libs/ui-components/src/components/Catalog/CatalogLandingPage.tsx`:
- Line 52: The CardHeader element currently has onClick and role="button" but is
missing keyboard accessibility; update the CardHeader
(fctl-catalog-landing__header) to be focusable (add tabIndex={0}) and add an
onKeyDown handler that calls the same onExpand function when Enter or Space is
pressed (handle Space by preventing default to avoid page scroll), ensuring the
keyboard activation mirrors the onClick behavior while keeping role="button".
In `@libs/ui-components/src/components/Catalog/CatalogPageToolbar.tsx`:
- Around line 16-28: The CreateCatalogItemBtn component currently returns a
boolean when the user lacks permission (via the canCreate value from
checkPermissions), which can produce React warnings; update the render logic in
CreateCatalogItemBtn to explicitly return null when !canCreate (e.g., replace
the &&-expression with a conditional that returns the Button when canCreate is
true and null otherwise), keeping the usePermissionsContext().checkPermissions
and navigate/t translation usage unchanged.
In
`@libs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsx`:
- Around line 38-39: The variable isCatalogSync (created via useSearchParams in
CreateRepository) is already a boolean, so remove redundant double-negation
usages: keep a single boolean conversion at the source (either const
isCatalogSync = !!searchParams.get('isCatalogSync') or const isCatalogSync =
Boolean(searchParams.get('isCatalogSync'))) and remove any additional
`!!isCatalogSync` occurrences later in the file; search for and replace
references that use `!!isCatalogSync` to use `isCatalogSync` directly (affecting
code paths that reference isCatalogSync around the CreateRepository component).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1a2c13a-5077-4a9a-9165-8cef56749a83
📒 Files selected for processing (11)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/Catalog/CatalogLandingPage.csslibs/ui-components/src/components/Catalog/CatalogLandingPage.tsxlibs/ui-components/src/components/Catalog/CatalogPage.tsxlibs/ui-components/src/components/Catalog/CatalogPageToolbar.tsxlibs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsxlibs/ui-components/src/hooks/useNavigate.tsxlibs/ui-components/src/types/rbac.ts
libs/ui-components/src/components/Catalog/CatalogLandingPage.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/Catalog/CatalogLandingPage.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+434
to
+440
| isChecked={options?.mustUseResourceSyncs ? true : values.useResourceSyncs} | ||
| isDisabled={!!options?.mustUseResourceSyncs} | ||
| onChange={(_, checked) => { | ||
| // Trigger validation of the resource syncs items | ||
| return setFieldValue('useResourceSyncs', checked, true); | ||
| }} | ||
| body={values.useResourceSyncs && <CreateResourceSyncsForm />} | ||
| body={values.useResourceSyncs && <CreateResourceSyncsForm allowedRSTypes={options?.allowedRSTypes} />} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how useResourceSyncs initial value is set when mustUseResourceSyncs is true
# Check if getInitValues handles this case
ast-grep --pattern $'export const getInitValues = ($$$) => {
$$$
}'
# Also check if mustUseResourceSyncs is passed to getInitValues
rg -n "getInitValues" --type ts -A 5Repository: flightctl/flightctl-ui
Length of output: 5736
🏁 Script executed:
cat -n libs/ui-components/src/components/Repository/CreateRepository/utils.ts | sed -n '461,550p'Repository: flightctl/flightctl-ui
Length of output: 3889
🏁 Script executed:
cat -n libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx | sed -n '430,500p'Repository: flightctl/flightctl-ui
Length of output: 3029
🏁 Script executed:
cat -n libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx | sed -n '495,550p'Repository: flightctl/flightctl-ui
Length of output: 3117
Mismatch between checkbox display and form submission: getInitValues doesn't initialize useResourceSyncs when mustUseResourceSyncs is true.
When options?.mustUseResourceSyncs is true, the checkbox displays as checked and becomes disabled (lines 434–435), but the underlying form value useResourceSyncs is not initialized to true. The getInitValues function in utils.ts (line 461) only accepts canUseResourceSyncs, allowedRepoTypes, showRepoTypes, and writeAccessOnly in its options type—it lacks mustUseResourceSyncs. As a result, useResourceSyncs is initialized based only on canUseRSs logic (lines 492 and 527 in utils.ts), which may be false. During form submission (lines 500 and 543), only values.useResourceSyncs is checked, so resource syncs won't be created despite the checkbox indicating they should be.
Update getInitValues to accept mustUseResourceSyncs in its options type and set useResourceSyncs: true when this option is provided.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx`
around lines 434 - 440, getInitValues currently doesn't accept
mustUseResourceSyncs so values.useResourceSyncs can be false even when the
checkbox is shown checked via options?.mustUseResourceSyncs; update the
getInitValues signature in utils.ts to include mustUseResourceSyncs in its
options type and ensure the returned initial state sets useResourceSyncs: true
when options.mustUseResourceSyncs is true (otherwise fall back to existing
canUseResourceSyncs/canUseRSs logic), so the form value matches the disabled
checked checkbox used in CreateRepositoryForm (where isChecked/onChange
reference values.useResourceSyncs and options?.mustUseResourceSyncs).
rawagner
force-pushed
the
catalog_empty
branch
4 times, most recently
from
e33efe7 to
61832d3
Compare
rawagner
commented
Mar 16, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx (1)
434-440:⚠️ Potential issue | 🟠 MajorMismatch between checkbox display and form submission persists.
When
options?.mustUseResourceSyncsistrue, the checkbox displays as checked (line 434) and disabled (line 435), but the form'svalues.useResourceSyncsmay still befalseifgetInitValuesdoesn't account for this option.During submission (lines 500 and 543), only
values.useResourceSyncsis checked:if (values.useResourceSyncs) { // create resource syncs }This means resource syncs won't be created despite the checkbox indicating they should be.
#!/bin/bash # Verify if getInitValues handles mustUseResourceSyncs rg -n "mustUseResourceSyncs" libs/ui-components/src/components/Repository/CreateRepository/utils.ts -A 5 -B 2 # Check getInitValues function signature and logic ast-grep --pattern $'export const getInitValues = ($$$) => { $$$ }'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx` around lines 434 - 440, The checkbox in CreateRepositoryForm shows checked/disabled when options?.mustUseResourceSyncs is true but form state may not reflect that, so update getInitValues (in CreateRepositoryForm utilities) to set useResourceSyncs: true when options?.mustUseResourceSyncs is true (or ensure the form's initialValues passed into CreateRepositoryForm include useResourceSyncs=true), and in the submission paths that check values.useResourceSyncs (the resource-sync creation logic) rely on that value or alternatively read options?.mustUseResourceSyncs as a fallback (e.g., treat useResourceSyncs as values.useResourceSyncs || options?.mustUseResourceSyncs) so resource syncs are created when the checkbox is forced on; also ensure setFieldValue logic that toggles useResourceSyncs remains consistent with this initialization.
🧹 Nitpick comments (1)
libs/ui-components/src/components/Repository/CreateRepository/utils.ts (1)
30-30: Consider using greedy quantifier for clarity.The non-greedy quantifier
*?is unusual here. Since Yup's.matches()only checks for pattern existence (not capture groups), the non-greedy behavior has no effect. Using/\/.*/would be the conventional idiom and clearer to maintainers.Suggested change
-const pathRegex = /\/.*?/; +const pathRegex = /\/.*/;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Repository/CreateRepository/utils.ts` at line 30, The regex assigned to pathRegex uses a non-greedy quantifier "/\/.*?/" which is unnecessary for Yup.matches() usage; update the pattern used in pathRegex to the conventional greedy form "/\/.*/" so the intent is clearer to maintainers and behavior is unchanged (look for a leading slash followed by any characters); ensure any uses of pathRegex in the CreateRepository validation remain unchanged otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx`:
- Around line 434-440: The checkbox in CreateRepositoryForm shows
checked/disabled when options?.mustUseResourceSyncs is true but form state may
not reflect that, so update getInitValues (in CreateRepositoryForm utilities) to
set useResourceSyncs: true when options?.mustUseResourceSyncs is true (or ensure
the form's initialValues passed into CreateRepositoryForm include
useResourceSyncs=true), and in the submission paths that check
values.useResourceSyncs (the resource-sync creation logic) rely on that value or
alternatively read options?.mustUseResourceSyncs as a fallback (e.g., treat
useResourceSyncs as values.useResourceSyncs || options?.mustUseResourceSyncs) so
resource syncs are created when the checkbox is forced on; also ensure
setFieldValue logic that toggles useResourceSyncs remains consistent with this
initialization.
---
Nitpick comments:
In `@libs/ui-components/src/components/Repository/CreateRepository/utils.ts`:
- Line 30: The regex assigned to pathRegex uses a non-greedy quantifier
"/\/.*?/" which is unnecessary for Yup.matches() usage; update the pattern used
in pathRegex to the conventional greedy form "/\/.*/" so the intent is clearer
to maintainers and behavior is unchanged (look for a leading slash followed by
any characters); ensure any uses of pathRegex in the CreateRepository validation
remain unchanged otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f044822-64e2-4a31-aca8-23b599caa3f1
📒 Files selected for processing (12)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/Catalog/CatalogLandingPage.csslibs/ui-components/src/components/Catalog/CatalogLandingPage.tsxlibs/ui-components/src/components/Catalog/CatalogPage.tsxlibs/ui-components/src/components/Catalog/CatalogPageToolbar.tsxlibs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsxlibs/ui-components/src/components/Repository/CreateRepository/utils.tslibs/ui-components/src/hooks/useNavigate.tsxlibs/ui-components/src/types/rbac.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/ui-components/src/hooks/useNavigate.tsx
- libs/ui-components/src/types/rbac.ts
- libs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsx
- libs/ui-components/src/components/Catalog/CatalogLandingPage.css
- libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (3)
libs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsx (1)
114-114: Minor: Redundant double negation.
isCatalogSyncis already a boolean from line 39 (!!searchParams.get(...)), so!!isCatalogSyncis redundant.♻️ Suggested fix
- if (!!isCatalogSync) { + if (isCatalogSync) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsx` at line 114, The condition uses a redundant double negation: replace the `if (!!isCatalogSync)` check with a simple `if (isCatalogSync)` inside CreateRepository (remove the `!!`), referencing the `isCatalogSync` variable initialized from `searchParams.get(...)` so the boolean check is direct and clearer.libs/ui-components/src/components/Catalog/CatalogPage.tsx (1)
272-278: Consider removing optional chaining onmetadata.namefor API-returned catalogs.Per coding guidelines,
resource.metadata.nameshould be treated as always defined for resources returned from the API. The|| ''fallbacks are overly defensive here sincecatalogscomes from the API response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/CatalogPage.tsx` around lines 272 - 278, The code is overly defensive with optional fallbacks for catalog names; in the catalogs.map block (referencing catalogs.map, canDelete, and the returned object fields name, id, and checkProps) replace uses of `c.metadata.name || ''` and `!!c.metadata.name` with direct accesses to `c.metadata.name` (e.g., name: c.spec.displayName || c.metadata.name, id: c.metadata.name, and use c.metadata.name when building checkProps.checked and catalogFilter.includes checks) since API-returned resources guarantee metadata.name; ensure types/signatures reflect non-null if needed.libs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsx (1)
124-124: Consider setting a defaulttypewhen adding a new resource sync.When a new resource sync is added, no
typeis specified. TheuseEffecthandles this whenallowedRSTypesis provided, but when it'sundefined, the new item has no type until user interaction.♻️ Suggested improvement
- onClick={() => arrayHelpers.push({ name: '', path: '', targetRevision: '' } as ResourceSyncFormValue)} + onClick={() => arrayHelpers.push({ name: '', path: '', targetRevision: '', type: allowedRSTypes?.[0] ?? ResourceSyncType.ResourceSyncTypeFleet } as ResourceSyncFormValue)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsx` at line 124, When pushing a new ResourceSyncFormValue via arrayHelpers.push, include a default type so the item isn't created typeless; change the push payload in CreateResourceSyncsForm (the arrayHelpers.push call) to set type: allowedRSTypes?.[0] ?? defaultResourceSyncType (or a sensible constant you define), and add a small defaultResourceSyncType constant near the component so when allowedRSTypes is undefined the new item still has a valid type; this touches ResourceSyncFormValue, the arrayHelpers.push call, and uses allowedRSTypes and useEffect for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/ui-components/src/components/Catalog/CatalogPage.tsx`:
- Around line 272-278: The code is overly defensive with optional fallbacks for
catalog names; in the catalogs.map block (referencing catalogs.map, canDelete,
and the returned object fields name, id, and checkProps) replace uses of
`c.metadata.name || ''` and `!!c.metadata.name` with direct accesses to
`c.metadata.name` (e.g., name: c.spec.displayName || c.metadata.name, id:
c.metadata.name, and use c.metadata.name when building checkProps.checked and
catalogFilter.includes checks) since API-returned resources guarantee
metadata.name; ensure types/signatures reflect non-null if needed.
In
`@libs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsx`:
- Line 114: The condition uses a redundant double negation: replace the `if
(!!isCatalogSync)` check with a simple `if (isCatalogSync)` inside
CreateRepository (remove the `!!`), referencing the `isCatalogSync` variable
initialized from `searchParams.get(...)` so the boolean check is direct and
clearer.
In
`@libs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsx`:
- Line 124: When pushing a new ResourceSyncFormValue via arrayHelpers.push,
include a default type so the item isn't created typeless; change the push
payload in CreateResourceSyncsForm (the arrayHelpers.push call) to set type:
allowedRSTypes?.[0] ?? defaultResourceSyncType (or a sensible constant you
define), and add a small defaultResourceSyncType constant near the component so
when allowedRSTypes is undefined the new item still has a valid type; this
touches ResourceSyncFormValue, the arrayHelpers.push call, and uses
allowedRSTypes and useEffect for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 649220f1-2771-455e-8912-06e64337644c
📒 Files selected for processing (13)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/Catalog/CatalogLandingPage.csslibs/ui-components/src/components/Catalog/CatalogLandingPage.tsxlibs/ui-components/src/components/Catalog/CatalogPage.tsxlibs/ui-components/src/components/Catalog/CatalogPageToolbar.tsxlibs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsxlibs/ui-components/src/components/Repository/CreateRepository/utils.tslibs/ui-components/src/hooks/useAppLinks.tslibs/ui-components/src/hooks/useNavigate.tsxlibs/ui-components/src/types/rbac.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx
- libs/ui-components/src/types/rbac.ts
- libs/ui-components/src/components/Catalog/CatalogLandingPage.css
- libs/ui-components/src/components/Catalog/CatalogPageToolbar.tsx
- libs/ui-components/src/components/Repository/CreateRepository/utils.ts
- libs/ui-components/src/hooks/useNavigate.tsx
celdrake
reviewed
Mar 16, 2026
Collaborator
celdrake
left a comment
celdrake
left a comment
There was a problem hiding this comment.
Mostly comments regarding UX, and some copy changes which IMO are necessary.
libs/ui-components/src/components/Catalog/CatalogLandingPage.tsx
Outdated
Show resolved
Hide resolved
| </StackItem> | ||
| <StackItem> | ||
| <Card isExpanded={!!expandedItem}> | ||
| <CardSection |
Collaborator
There was a problem hiding this comment.
Should the first section be expanded by default?
| <GridItem> | ||
| <Stack hasGutter> | ||
| <StackItem> | ||
| <Content component="h2">{t('Available now')}</Content> |
Collaborator
There was a problem hiding this comment.
Adding the "Available now" block but not the "Coming soon" blocks seems odd to me, is that what UX expected?
libs/ui-components/src/components/Catalog/CatalogLandingPage.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/Catalog/CatalogLandingPage.tsx
Outdated
Show resolved
Hide resolved
| <Divider /> | ||
| <CardSection | ||
| title={t('Learn more')} | ||
| icon={<BookOpenIcon />} |
Collaborator
There was a problem hiding this comment.
Icon is not the one I see in the design.
| > | ||
| <Stack hasGutter> | ||
| <StackItem> | ||
| {t('Explore documentation and resources to get the most out of the Software Catalog.')} |
Collaborator
There was a problem hiding this comment.
Content is not the same I see in the design.
| </ListItem> | ||
| </List> | ||
| </StackItem> | ||
| <StackItem> |
Collaborator
There was a problem hiding this comment.
Should we even show this block if the user can't create image builds?
And what about viewer users that wouldn't have any "createX" permission?
Collaborator
Author
There was a problem hiding this comment.
IMO yes
at least they know about the possibility and could ask somebody with permissions ?
Collaborator
There was a problem hiding this comment.
IDK, perhaps the copy should be adapted in those cases?
But it's for UX to decide, feel free to dismiss if not applicable.
libs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx (1)
434-440:⚠️ Potential issue | 🟠 MajorForced checkbox state and body visibility are not aligned.
At Line 434-Line 440,
isCheckedcan be forced true bymustUseResourceSyncs, but the body still depends only onvalues.useResourceSyncs. This can hide the resource-sync form while the checkbox appears enabled/checked-by-policy.🔧 Suggested fix
- isChecked={options?.mustUseResourceSyncs ? true : values.useResourceSyncs} - isDisabled={!!options?.mustUseResourceSyncs} + isChecked={!!options?.mustUseResourceSyncs || values.useResourceSyncs} + isDisabled={!!options?.mustUseResourceSyncs} onChange={(_, checked) => { // Trigger validation of the resource syncs items return setFieldValue('useResourceSyncs', checked, true); }} - body={values.useResourceSyncs && <CreateResourceSyncsForm allowedRSTypes={options?.allowedRSTypes} />} + body={ + (!!options?.mustUseResourceSyncs || values.useResourceSyncs) && ( + <CreateResourceSyncsForm allowedRSTypes={options?.allowedRSTypes} /> + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx` around lines 434 - 440, The checkbox visual state can be forced by options?.mustUseResourceSyncs while the body rendering still uses values.useResourceSyncs, causing the resource-sync form to be hidden when the checkbox appears checked by policy; update the rendering and handler in CreateRepositoryForm so body visibility uses the effective checked state (e.g., const effectiveUseResourceSyncs = options?.mustUseResourceSyncs ? true : values.useResourceSyncs) and render CreateResourceSyncsForm when effectiveUseResourceSyncs is true, and ensure onChange only calls setFieldValue('useResourceSyncs', ...) when the control is not forced (i.e., when !options?.mustUseResourceSyncs) to avoid updating values incorrectly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@libs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsx`:
- Around line 39-40: The query-param boolean parsing for isCatalogSync is too
permissive: change the check that sets isCatalogSync (currently using
!!searchParams.get('isCatalogSync')) to a strict boolean parse (e.g., compare
the raw value from searchParams.get('isCatalogSync') against the string "true"
or explicitly accept only allowed truthy tokens) so that values like "false" do
not evaluate to true; update the logic in CreateRepository (where searchParams
and isCatalogSync are used) accordingly to rely on the strict comparison.
---
Duplicate comments:
In
`@libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx`:
- Around line 434-440: The checkbox visual state can be forced by
options?.mustUseResourceSyncs while the body rendering still uses
values.useResourceSyncs, causing the resource-sync form to be hidden when the
checkbox appears checked by policy; update the rendering and handler in
CreateRepositoryForm so body visibility uses the effective checked state (e.g.,
const effectiveUseResourceSyncs = options?.mustUseResourceSyncs ? true :
values.useResourceSyncs) and render CreateResourceSyncsForm when
effectiveUseResourceSyncs is true, and ensure onChange only calls
setFieldValue('useResourceSyncs', ...) when the control is not forced (i.e.,
when !options?.mustUseResourceSyncs) to avoid updating values incorrectly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a2ed4d9-573a-4e46-9077-9b84e3929745
📒 Files selected for processing (13)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/Catalog/CatalogLandingPage.csslibs/ui-components/src/components/Catalog/CatalogLandingPage.tsxlibs/ui-components/src/components/Catalog/CatalogPage.tsxlibs/ui-components/src/components/Catalog/CatalogPageToolbar.tsxlibs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsxlibs/ui-components/src/components/Repository/CreateRepository/utils.tslibs/ui-components/src/hooks/useAppLinks.tslibs/ui-components/src/hooks/useNavigate.tsxlibs/ui-components/src/types/rbac.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/ui-components/src/types/rbac.ts
- libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx
- libs/ui-components/src/hooks/useAppLinks.ts
- libs/ui-components/src/hooks/useNavigate.tsx
- libs/ui-components/src/components/Catalog/CatalogLandingPage.css
libs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsx
Outdated
Show resolved
Hide resolved
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)
libs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsx (1)
124-124:⚠️ Potential issue | 🟡 MinorMissing
typefield when adding a new resource sync.The pushed object is missing the
typefield, which is required byResourceSyncFormValue. While theuseEffectat lines 28-32 will correct this at runtime, the object doesn't match the TypeScript type being cast.🔧 Suggested fix
- onClick={() => arrayHelpers.push({ name: '', path: '', targetRevision: '' } as ResourceSyncFormValue)} + onClick={() => arrayHelpers.push({ name: '', path: '', targetRevision: '', type: allowedRSTypes?.[0] ?? ResourceSyncType.ResourceSyncTypeFleet } as ResourceSyncFormValue)}This also requires passing
allowedRSTypesto the callback scope or importingResourceSyncTypeif not already imported in the relevant scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsx` at line 124, The object pushed in the onClick handler for arrayHelpers.push does not include the required `type` field of `ResourceSyncFormValue`; update the pushed object to include a valid `type` (e.g., default to the first entry in `allowedRSTypes` or a specific `ResourceSyncType`) so it conforms to the `ResourceSyncFormValue` type instead of relying on the useEffect to fix it later, and ensure `allowedRSTypes` or `ResourceSyncType` is available in the handler scope (pass `allowedRSTypes` into the callback or import `ResourceSyncType`).
🧹 Nitpick comments (1)
libs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsx (1)
28-32:setValuein dependency array may cause re-renders.The
setValuefunction from Formik'suseFieldis not guaranteed to be referentially stable across renders, which could trigger unnecessary effect executions. Consider wrapping the effect body or removingsetValuefrom dependencies if the behavior is intended to run only whenallowedRSTypesorvaluechange.♻️ Suggested stabilization
React.useEffect(() => { if (allowedRSTypes?.length && !allowedRSTypes.includes(value)) { setValue(allowedRSTypes[0]); } - }, [allowedRSTypes, value, setValue]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [allowedRSTypes, value]);Alternatively, use Formik's
setFieldValuefromuseFormikContextwhich has stable identity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsx` around lines 28 - 32, The effect in CreateResourceSyncsForm uses setValue from useField in its dependency array which may be unstable and cause extra runs; change the effect to depend only on allowedRSTypes and value and call a stable setter instead—either obtain setFieldValue from useFormikContext (stable identity) and call setFieldValue(fieldName, allowedRSTypes[0]) when allowedRSTypes?.length && !allowedRSTypes.includes(value), or keep setValue out of deps by wrapping the effect body in a callback that references setValue via a ref; update references to allowedRSTypes, value and the field name used by useField so the effect triggers only when allowedRSTypes or value 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 `@libs/ui-components/src/components/Catalog/CatalogLandingPage.tsx`:
- Line 54: The CardHeader element in CatalogLandingPage uses role="button" but
lacks keyboard handlers and focusability; add tabIndex={0} and an onKeyDown
handler that listens for Enter and Space (keys 'Enter' and ' ' / keyCode 13 and
32) to invoke the existing onExpand handler (calling onExpand() and preventing
default for Space to avoid page scroll). Also consider setting aria-expanded
based on the component's expanded state so assistive tech reflects the toggle
state.
In `@libs/ui-components/src/components/Catalog/CatalogPage.tsx`:
- Around line 267-269: The code is using empty-string fallbacks (e.g., item.id
|| '' and metadata.name || '') which allows invalid empty IDs into catalogFilter
state; remove the "|| ''" fallbacks and treat resource.metadata.name and item.id
as defined by the API, updating calls to catalogFilter.catalogs.includes and
catalogFilter.setCatalogs to use item.id (or metadata.name) directly so no empty
string can be inserted—specifically update the branches that call
catalogFilter.catalogs.includes(item.id || '') and
catalogFilter.setCatalogs((catalogs) => catalogs.filter((c) => c !== item.id ||
'')) / setCatalogs((catalogs) => [...catalogs, item.id || '']) and the other
occurrences that use metadata.name || '' to use metadata.name directly.
---
Outside diff comments:
In
`@libs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsx`:
- Line 124: The object pushed in the onClick handler for arrayHelpers.push does
not include the required `type` field of `ResourceSyncFormValue`; update the
pushed object to include a valid `type` (e.g., default to the first entry in
`allowedRSTypes` or a specific `ResourceSyncType`) so it conforms to the
`ResourceSyncFormValue` type instead of relying on the useEffect to fix it
later, and ensure `allowedRSTypes` or `ResourceSyncType` is available in the
handler scope (pass `allowedRSTypes` into the callback or import
`ResourceSyncType`).
---
Nitpick comments:
In
`@libs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsx`:
- Around line 28-32: The effect in CreateResourceSyncsForm uses setValue from
useField in its dependency array which may be unstable and cause extra runs;
change the effect to depend only on allowedRSTypes and value and call a stable
setter instead—either obtain setFieldValue from useFormikContext (stable
identity) and call setFieldValue(fieldName, allowedRSTypes[0]) when
allowedRSTypes?.length && !allowedRSTypes.includes(value), or keep setValue out
of deps by wrapping the effect body in a callback that references setValue via a
ref; update references to allowedRSTypes, value and the field name used by
useField so the effect triggers only when allowedRSTypes or value change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d4386ac-12f3-415b-a89d-5e217db9c5ba
📒 Files selected for processing (13)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/Catalog/CatalogLandingPage.csslibs/ui-components/src/components/Catalog/CatalogLandingPage.tsxlibs/ui-components/src/components/Catalog/CatalogPage.tsxlibs/ui-components/src/components/Catalog/CatalogPageToolbar.tsxlibs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateRepository.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsxlibs/ui-components/src/components/Repository/CreateRepository/CreateResourceSyncsForm.tsxlibs/ui-components/src/components/Repository/CreateRepository/utils.tslibs/ui-components/src/hooks/useAppLinks.tslibs/ui-components/src/hooks/useNavigate.tsxlibs/ui-components/src/types/rbac.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/ui-components/src/components/Catalog/CatalogPageToolbar.tsx
- libs/ui-components/src/components/Catalog/CatalogLandingPage.css
- libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx
- libs/ui-components/src/hooks/useNavigate.tsx
| }>) => { | ||
| return ( | ||
| <> | ||
| <CardHeader onClick={onExpand} role="button" className="fctl-catalog-landing__header"> |
There was a problem hiding this comment.
Add keyboard support for the expandable section header.
Line 54 sets role="button" but only wires onClick; keyboard users can’t toggle with Enter/Space.
♿ Suggested fix
- <CardHeader onClick={onExpand} role="button" className="fctl-catalog-landing__header">
+ <CardHeader
+ onClick={onExpand}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ onExpand();
+ }
+ }}
+ role="button"
+ tabIndex={0}
+ aria-expanded={isExpanded}
+ className="fctl-catalog-landing__header"
+ >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-components/src/components/Catalog/CatalogLandingPage.tsx` at line 54,
The CardHeader element in CatalogLandingPage uses role="button" but lacks
keyboard handlers and focusability; add tabIndex={0} and an onKeyDown handler
that listens for Enter and Space (keys 'Enter' and ' ' / keyCode 13 and 32) to
invoke the existing onExpand handler (calling onExpand() and preventing default
for Space to avoid page scroll). Also consider setting aria-expanded based on
the component's expanded state so assistive tech reflects the toggle state.
Comment on lines
+267
to
+269
| catalogFilter.catalogs.includes(item.id || '') | ||
| ? catalogFilter.setCatalogs((catalogs) => catalogs.filter((c) => c !== item.id || '')) | ||
| : catalogFilter.setCatalogs((catalogs) => [...catalogs, item.id || '']); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid empty-string fallbacks for API metadata.name in catalog state paths.
Lines 275-279 and Line 357 use metadata.name || '', and Lines 267-269 do the same with item.id || ''. This weakens the API invariant and can pollute filter state with empty IDs.
🛠️ Suggested fix
- onCheck={(_, item) => {
- catalogFilter.catalogs.includes(item.id || '')
- ? catalogFilter.setCatalogs((catalogs) => catalogs.filter((c) => c !== item.id || ''))
- : catalogFilter.setCatalogs((catalogs) => [...catalogs, item.id || '']);
- }}
+ onCheck={(_, item) => {
+ const catalogName = item.id as string;
+ catalogFilter.setCatalogs((catalogs) =>
+ catalogs.includes(catalogName)
+ ? catalogs.filter((c) => c !== catalogName)
+ : [...catalogs, catalogName],
+ );
+ }}
@@
- name: c.spec.displayName || c.metadata.name || '',
- id: c.metadata.name || '',
+ name: c.spec.displayName || c.metadata.name,
+ id: c.metadata.name,
checkProps: {
- checked: !!c.metadata.name && catalogFilter.catalogs.includes(c.metadata.name),
+ checked: catalogFilter.catalogs.includes(c.metadata.name),
},
@@
- itemName: ci.metadata.name || '',
+ itemName: ci.metadata.name,As per coding guidelines: "Treat resource.metadata.name as always defined for resources returned from the API or created resources, and avoid optional chaining in these contexts; only handle undefined for partial or input payloads".
Also applies to: 275-279, 357-358
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-components/src/components/Catalog/CatalogPage.tsx` around lines 267 -
269, The code is using empty-string fallbacks (e.g., item.id || '' and
metadata.name || '') which allows invalid empty IDs into catalogFilter state;
remove the "|| ''" fallbacks and treat resource.metadata.name and item.id as
defined by the API, updating calls to catalogFilter.catalogs.includes and
catalogFilter.setCatalogs to use item.id (or metadata.name) directly so no empty
string can be inserted—specifically update the branches that call
catalogFilter.catalogs.includes(item.id || '') and
catalogFilter.setCatalogs((catalogs) => catalogs.filter((c) => c !== item.id ||
'')) / setCatalogs((catalogs) => [...catalogs, item.id || '']) and the other
occurrences that use metadata.name || '' to use metadata.name directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements