Jonburdo 20260422 sync#1714
Merged
openshift-merge-bot[bot] merged 13 commits intoApr 22, 2026
Merged
Conversation
Bumps [actions/cache](https://github.com/actions/cache) from 5.0.4 to 5.0.5. - [Release notes](https://github.com/actions/cache/releases) - [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md) - [Commits](actions/cache@6682284...27d5ce7) --- updated-dependencies: - dependency-name: actions/cache dependency-version: 5.0.5 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [mikefarah/yq](https://github.com/mikefarah/yq) from 4.52.5 to 4.53.2. - [Release notes](https://github.com/mikefarah/yq/releases) - [Changelog](https://github.com/mikefarah/yq/blob/master/release_notes.txt) - [Commits](mikefarah/yq@0f4fb8d...751d8ad) --- updated-dependencies: - dependency-name: mikefarah/yq dependency-version: 4.53.2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: manaswinidas <dasmanaswini10@gmail.com>
* fix pf5 to pf6 Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * start to change the texts Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * change text and add them to constants Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * fix constants text Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * fix helper text prosition Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * add fixes requested Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * fix layout issue Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * fix changes requested Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> --------- Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com>
…kubeflow#2614) Bumps [k8s.io/api](https://github.com/kubernetes/api) from 0.35.3 to 0.35.4. - [Commits](kubernetes/api@v0.35.3...v0.35.4) --- updated-dependencies: - dependency-name: k8s.io/api dependency-version: 0.35.4 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…kubeflow#2593) The operator renamed the ConfigMap from model-catalog-default-sources to default-catalog-sources in model-registry-operator#489, but the BFF and kustomize manifests were not updated, causing a 500 error on the Model Catalog Settings page. Ref: RHOAIENG-57824 Signed-off-by: Mike Turley <mike.turley@alum.cs.umass.edu> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ubeflow#2600) * add helper text for hf Signed-off-by: Taj010 <rsheen003@gmail.com> * adjust the message Signed-off-by: Taj010 <rsheen003@gmail.com> * rebase and adjust Signed-off-by: Taj010 <rsheen003@gmail.com> --------- Signed-off-by: Taj010 <rsheen003@gmail.com>
…#2624) AI-assisted: Claude Code (Opus 4.6) Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>
* fix a11y from model details Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * add changes requested Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> --------- Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com>
* hide cat toogle in case only one cat is available Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * fix changes requested in tests Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * fix changes requested Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * add changes requested Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * add changes requested Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> --------- Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com>
* truncate error msg from transfer jobs Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * add changes requested and updated tests Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> --------- Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com>
…acts the run (kubeflow#2628) Signed-off-by: Debarati Basu-Nag <dbasunag@redhat.com>
Signed-off-by: Jon Burdo <jon@jonburdo.com>
📝 WalkthroughWalkthroughThis PR refactors catalog source configuration and model catalog browsing behavior. Changes include: renaming the default catalog sources ConfigMap from Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Security & Actionable IssuesNo immediate CWE/CVE-level vulnerabilities detected in this diff. However, flag these for verification:
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Member
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalogSettings/modelCatalogSettings.cy.ts (1)
436-448:⚠️ Potential issue | 🟡 MinorStale test title "Connected".
The
it(...)description still saysshould show "Connected" status for available sources, but the assertion on line 446 now checks for'Ready'. Rename the test so greppability doesn't suffer.Proposed tweak
- it('should show "Connected" status for available sources', () => { + it('should show "Ready" status for available sources', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalogSettings/modelCatalogSettings.cy.ts` around lines 436 - 448, The test title in the it(...) block for the case using mockCatalogSource('hf-google') is stale — it says "should show \"Connected\" status for available sources" but the assertion uses row.shouldHaveValidationStatus('Ready') and checks for 'source-status-connected-hf-google'; update the test description to match the assertion (e.g., change the it(...) string to "should show 'Ready' status for available sources" or equivalent) so the test name reflects modelCatalogSettings.getRow('HuggingFace Google') expectations and keeps greppability accurate.clients/ui/frontend/src/app/pages/modelCatalogSettings/components/ModelVisibilitySection.tsx (1)
66-132:⚠️ Potential issue | 🟠 MajorRemove permanently-active error styling from both textareas.
ThemeAwareFormGroupWrappersets error state (pf-m-error) wheneverhelperTextNodeis defined:const hasError = !!helperTextNode;. Lines 70–75 and 99–104 unconditionally defineallowedModelsHelperTxtNodeandexcludedModelsHelperTxtNodeas React nodes wrapping purely informational copy (getIncludedModelsFieldHelperText/getExcludedModelsFieldHelperText). Passing these tohelperTextNodeforces both form groups into the error variant on every render—red borders and error-styled labels on fields with no validation failure.Route the informational text through
descriptionTextNodeinstead and omithelperTextNode. Merge the two text items into a single description node:🔧 Suggested fix: consolidate informational text into description node
- const allowedModelsDescriptionTxtNode = ( - <FormHelperText> - <HelperText> - <HelperTextItem>{getAllowedModelsHelp(organization)}</HelperTextItem> - </HelperText> - </FormHelperText> - ); - - const allowedModelsHelperTxtNode = ( - <FormHelperText> - <HelperText> - <HelperTextItem>{getIncludedModelsFieldHelperText}</HelperTextItem> - </HelperText> - </FormHelperText> - ); + const allowedModelsDescriptionTxtNode = ( + <FormHelperText> + <HelperText> + <HelperTextItem>{getAllowedModelsHelp(organization)}</HelperTextItem> + <HelperTextItem>{getIncludedModelsFieldHelperText}</HelperTextItem> + </HelperText> + </FormHelperText> + ); @@ - const excludedModelsDescriptionTxtNode = ( - <FormHelperText> - <HelperText> - <HelperTextItem>{getExcludedModelsHelp(organization)}</HelperTextItem> - </HelperText> - </FormHelperText> - ); - - const excludedModelsHelperTxtNode = ( - <FormHelperText> - <HelperText> - <HelperTextItem>{getExcludedModelsFieldHelperText}</HelperTextItem> - </HelperText> - </FormHelperText> - ); + const excludedModelsDescriptionTxtNode = ( + <FormHelperText> + <HelperText> + <HelperTextItem>{getExcludedModelsHelp(organization)}</HelperTextItem> + <HelperTextItem>{getExcludedModelsFieldHelperText}</HelperTextItem> + </HelperText> + </FormHelperText> + ); @@ - <ThemeAwareFormGroupWrapper - label={FORM_LABELS.ALLOWED_MODELS} - fieldId="allowed-models" - descriptionTextNode={allowedModelsDescriptionTxtNode} - helperTextNode={allowedModelsHelperTxtNode} - > + <ThemeAwareFormGroupWrapper + label={FORM_LABELS.ALLOWED_MODELS} + fieldId="allowed-models" + descriptionTextNode={allowedModelsDescriptionTxtNode} + > @@ - <ThemeAwareFormGroupWrapper - label={FORM_LABELS.EXCLUDED_MODELS} - fieldId="excluded-models" - descriptionTextNode={excludedModelsDescriptionTxtNode} - helperTextNode={excludedModelsHelperTxtNode} - > + <ThemeAwareFormGroupWrapper + label={FORM_LABELS.EXCLUDED_MODELS} + fieldId="excluded-models" + descriptionTextNode={excludedModelsDescriptionTxtNode} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/ui/frontend/src/app/pages/modelCatalogSettings/components/ModelVisibilitySection.tsx` around lines 66 - 132, The two helper nodes allowedModelsHelperTxtNode and excludedModelsHelperTxtNode are being passed into ThemeAwareFormGroupWrapper.helperTextNode which forces pf-m-error styling; delete those helper node variables, and instead merge their informational copy (getIncludedModelsFieldHelperText / getExcludedModelsFieldHelperText) into the corresponding descriptionTextNode (allowedModelsDescriptionTxtNode / excludedModelsDescriptionTxtNode) so the informational text is rendered as description, not an error. Update the two ThemeAwareFormGroupWrapper usages to remove the helperTextNode prop entirely and supply the combined descriptionTextNode (retain existing description generators like getExcludedModelsHelp(organization) and append the helper text inside the same FormHelperText/HelperText/HelperTextItem node). Ensure no other code references allowedModelsHelperTxtNode or excludedModelsHelperTxtNode after the change.clients/ui/frontend/src/app/pages/mcpCatalog/screens/McpCatalogGalleryView.tsx (1)
54-62:⚠️ Potential issue | 🟠 MajorUse
singleCategoryLabelin the server query.In single-category mode,
selectedSourceLabelcan still beundefinedon the first render, so this requests all servers while the UI is meant to show one category.Proposed fix
const filterQuery = React.useMemo(() => mcpFiltersToFilterQuery(filters), [filters]); + const effectiveSourceLabel = isSingleCategory + ? singleCategoryLabel ?? selectedSourceLabel + : selectedSourceLabel; const { mcpServers, mcpServersLoaded, mcpServersLoadError } = useMcpServersBySourceLabelWithAPI( mcpApiState, { - sourceLabel: selectedSourceLabel, + sourceLabel: effectiveSourceLabel, pageSize: PAGE_SIZE, searchQuery, filterQuery: filterQuery || undefined, @@ - const effectiveCategoryLabel = singleCategoryLabel || selectedSourceLabel || ''; + const effectiveCategoryLabel = effectiveSourceLabel || '';Also applies to: 103-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/ui/frontend/src/app/pages/mcpCatalog/screens/McpCatalogGalleryView.tsx` around lines 54 - 62, The server query currently uses selectedSourceLabel which may be undefined on first render; update the call to useMcpServersBySourceLabelWithAPI so it prefers singleCategoryLabel when present (e.g., sourceLabel: singleCategoryLabel || selectedSourceLabel) so single-category mode always queries the correct category; make the same change for the other occurrence around the use of useMcpServersBySourceLabelWithAPI later in the file (lines with the second call).clients/ui/frontend/src/app/pages/modelCatalog/screens/ModelCatalogGalleryView.tsx (1)
108-119:⚠️ Potential issue | 🟠 MajorUse the single-category label for the data request too.
singleCategoryLabelonly drives the header. In single-category mode,selectedSourceLabelcan still beCategoryName.allModelson the first render, so the hook requests all models before the auto-select effect catches up.Proposed fix
+ const effectiveSourceLabel = isSingleCategory + ? singleCategoryLabel || selectedSourceLabel + : selectedSourceLabel; + const { catalogModels, catalogModelsLoaded, catalogModelsLoadError } = useCatalogModelsBySources( '', - selectedSourceLabel === CategoryName.allModels ? undefined : selectedSourceLabel, + effectiveSourceLabel === CategoryName.allModels ? undefined : effectiveSourceLabel, 10, searchTerm, effectiveFilterData, @@ - const effectiveCategoryLabel = singleCategoryLabel || selectedSourceLabel || ''; + const effectiveCategoryLabel = effectiveSourceLabel || '';Also applies to: 281-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/ui/frontend/src/app/pages/modelCatalog/screens/ModelCatalogGalleryView.tsx` around lines 108 - 119, When calling useCatalogModelsBySources in ModelCatalogGalleryView, ensure the hook receives the single-category label when singleCategoryLabel is set so the initial request doesn't fetch all models; replace the current source arg logic with one that prefers singleCategoryLabel (e.g., use singleCategoryLabel ? singleCategoryLabel : (selectedSourceLabel === CategoryName.allModels ? undefined : selectedSourceLabel)). Apply the same change to the other useCatalogModelsBySources call later in the file (the one around the second occurrence).
🧹 Nitpick comments (6)
jobs/async-upload/tests/integration/test_integration_async_upload.py (1)
214-214: Centralize the namespace value before adding more call sites.Line 214 and Line 227 duplicate
"default"alongside delete, wait/log, and cleanup paths. A later namespace change can make apply/describe drift from the rest of the test flow; use one shared constant.Proposed refactor
+K8S_NAMESPACE = "default" + ... - "default", + K8S_NAMESPACE, ... - ["kubectl", "apply", "-k", ".", "-n", "default"], + ["kubectl", "apply", "-k", ".", "-n", K8S_NAMESPACE], ... - ["kubectl", "describe", "jobs/my-async-upload-job", "-n", "default"], + ["kubectl", "describe", "jobs/my-async-upload-job", "-n", K8S_NAMESPACE],Also applies to: 227-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jobs/async-upload/tests/integration/test_integration_async_upload.py` at line 214, The test duplicates the literal "default" namespace in multiple kubectl invocations (apply, delete, describe, wait/log and cleanup) in test_integration_async_upload.py; introduce a single constant (e.g., NAMESPACE = "default") at the top of the test module or the test function and replace all occurrences of the string literal in the kubectl command lists (the arrays used for ["kubectl", "apply", "-k", ".", "-n", "default"], ["kubectl", "delete", ... "-n", "default"], and the describe/wait/cleanup command arrays) with that constant so future namespace changes only require one edit.clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelTransferJobs/ModelTransferJobTableRow.tsx (1)
164-179: Extract inline style to a CSS class.Inline
style={{ textDecoration: 'underline dotted', textUnderlineOffset: 'var(--pf-t--global--spacer--xs)' }}on a PFButton variant="link"is repeated per row and bypasses the design-system stylesheet. Consider a dedicated class (e.g.,pf-v6-u-text-decoration-underline-dottedequivalent in your CSS module) so the rule can be restyled centrally and memoized by the browser.♻️ Suggested refactor
- <Button - variant="link" - isInline - onClick={() => setIsStatusModalOpen(true)} - data-testid="job-error-message" - style={{ - textDecoration: 'underline dotted', - textUnderlineOffset: 'var(--pf-t--global--spacer--xs)', - }} - > + <Button + variant="link" + isInline + onClick={() => setIsStatusModalOpen(true)} + data-testid="job-error-message" + className="mr-transfer-job-error-link" + > <Truncate content={job.errorMessage} /> </Button>…and add the rule to the accompanying stylesheet:
.mr-transfer-job-error-link { text-decoration: underline dotted; text-underline-offset: var(--pf-t--global--spacer--xs); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelTransferJobs/ModelTransferJobTableRow.tsx` around lines 164 - 179, Extract the inline style on the PF Button in ModelTransferJobTableRow (the Button rendered when job.status === ModelTransferJobStatus.FAILED) into a dedicated CSS class (e.g., mr-transfer-job-error-link), remove the style prop and apply the class via className on the Button, add the CSS rule (text-decoration: underline dotted; text-underline-offset: var(--pf-t--global--spacer--xs);) to the component stylesheet or CSS module, and ensure the stylesheet is imported (and class referenced) where the Button and Truncate are used so the visual rule is centralized and not repeated per row.clients/ui/frontend/src/app/pages/modelCatalogSettings/components/CatalogSourceStatus.tsx (1)
54-59: Staledata-testidafter label rename.The label text flipped from "Connected" to "Ready", but
data-testid={source-status-connected-${id}}still carries the old vocabulary. Cypress still relies on this id (seemodelCatalogSettings.cy.tsline 447), so renaming is a follow-up, but the mismatch will bite the next person grepping for "Ready". Consider aligning the testid (e.g.,source-status-ready-…) and updating the Cypress selector in lockstep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/ui/frontend/src/app/pages/modelCatalogSettings/components/CatalogSourceStatus.tsx` around lines 54 - 59, In the CatalogSourceStatus.tsx branch for CatalogSourceStatusEnum.AVAILABLE, update the stale data-testid value from `source-status-connected-${catalogSourceConfig.id}` to `source-status-ready-${catalogSourceConfig.id}` to match the label text change, and then update the Cypress selector(s) in the corresponding model catalog settings test that currently reference the old `source-status-connected-…` id to the new `source-status-ready-…` id so tests remain in sync.clients/ui/frontend/src/app/pages/modelCatalogSettings/constants.tsx (1)
134-140:get*FieldHelperTextare strings, not getters — rename to avoid misuse.Both
getIncludedModelsFieldHelperTextandgetExcludedModelsFieldHelperTextare plain string constants but carry thegetprefix idiomatic of functions/accessors (contrast with genuine gettersgetFilterInfoWithOrg/getAllowedModelsHelpin the same file). Consumers have already been tripped up — e.g.ModelVisibilitySection.tsxlines 69, 98 embed them as{getIncludedModelsFieldHelperText}which works by accident; any future refactor that tries to "call" them (getIncludedModelsFieldHelperText()) will returnundefinedat runtime.-/** Same for HF and YAML sources. */ -export const getIncludedModelsFieldHelperText = - 'Separate model names using commas. To include all models with a specific prefix, enter the prefix followed by an asterisk. Example, Llama*'; - -/** Same for HF and YAML sources. */ -export const getExcludedModelsFieldHelperText = - 'Separate model names using commas. To exclude all models with a specific prefix, enter the prefix followed by an asterisk. Example, Llama*'; +/** Same for HF and YAML sources. */ +export const INCLUDED_MODELS_FIELD_HELPER_TEXT = + 'Separate model names using commas. To include all models with a specific prefix, enter the prefix followed by an asterisk. Example, Llama*'; + +/** Same for HF and YAML sources. */ +export const EXCLUDED_MODELS_FIELD_HELPER_TEXT = + 'Separate model names using commas. To exclude all models with a specific prefix, enter the prefix followed by an asterisk. Example, Llama*';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/ui/frontend/src/app/pages/modelCatalogSettings/constants.tsx` around lines 134 - 140, Rename the misleading constants getIncludedModelsFieldHelperText and getExcludedModelsFieldHelperText to non-getter names (e.g., includedModelsFieldHelperText and excludedModelsFieldHelperText) and update all usages to the new identifiers (including JSX embeddings like where ModelVisibilitySection references them) so callers don't accidentally try to invoke them as functions; keep the string values unchanged and ensure any exports/imports are updated accordingly.clients/ui/bff/internal/integrations/kubernetes/k8mocks/base_testenv.go (1)
350-350: Use the ConfigMap name constant in the error path.Line 350 duplicates the value already sourced from
k8s.CatalogSourceDefaultConfigMapNameon Line 340, so future renames can make this diagnostic stale.Keep diagnostics tied to the source of truth
- return fmt.Errorf("failed to create default-catalog-sources configmap: %w", err) + return fmt.Errorf("failed to create %s configmap: %w", k8s.CatalogSourceDefaultConfigMapName, err)As per coding guidelines,
**: REVIEW PRIORITIES: 3. Bug-prone patterns and error handling gaps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/ui/bff/internal/integrations/kubernetes/k8mocks/base_testenv.go` at line 350, The error message currently hardcodes "default-catalog-sources" instead of referencing the shared constant; update the failing return in the configmap creation path to use k8s.CatalogSourceDefaultConfigMapName (the same constant used on the creation line) so the error text stays in sync with the source-of-truth; locate the configmap creation function/return that currently does `fmt.Errorf("failed to create default-catalog-sources configmap: %w", err)` and replace the hardcoded name with the constant.clients/ui/bff/go.mod (1)
15-17: Align direct Kubernetes module versions.Lines 15–17 mix
k8s.io/apiandk8s.io/apimachineryatv0.35.4withk8s.io/client-goatv0.35.3.v0.35.4ofclient-gois available; upgrade it for consistent API machinery and client versioning.Version alignment patch
k8s.io/api v0.35.4 k8s.io/apimachinery v0.35.4 - k8s.io/client-go v0.35.3 + k8s.io/client-go v0.35.4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/ui/bff/go.mod` around lines 15 - 17, Update the Kubernetes module versions so they are consistent: change the k8s.io/client-go module version from v0.35.3 to v0.35.4 to match k8s.io/api and k8s.io/apimachinery (both v0.35.4); after updating the k8s.io/client-go version in go.mod run go mod tidy (or the equivalent dependency refresh) to ensure the dependency tree is reconciled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@catalog/clients/python/patches/include-readonly-timestamps.patch`:
- Around line 4-11: Remove the duplicated docstring line in BaseResource and
make BaseResource.to_dict()'s excluded_fields consistent with CatalogModel by
ensuring create_time_since_epoch and last_update_time_since_epoch are not in the
excluded_fields set (so timestamps are serialized on reads); update the
excluded_fields variable in BaseResource.to_dict() accordingly and keep only one
instance of the "OpenAPI `readOnly` fields are excluded." sentence in the
docstring.
In
`@clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/modelCatalog.cy.ts`:
- Around line 550-558: The negative assertions run before the catalog finishes
rendering; after calling initIntercepts(...) and modelCatalog.visit(), wait for
a positive "catalog loaded" signal before asserting absence of the toggles —
e.g., use an existing helper or element (for example call
modelCatalog.waitForLoad() if available or assert
cy.findByTestId('model-catalog-list').should('exist')) and only then check
cy.findByTestId('label-Provider one').should('not.exist') and
cy.findByTestId('all').should('not.exist').
In
`@clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelTransferJobs.cy.ts`:
- Around line 572-589: The test registers an intercept alias '@retryJob' but
never triggers or asserts it; update the test in the it('should show retry
button...') block to actually submit the retry and wait for the PUT request:
after opening the retry modal via modelTransferJobsPage.findRetryModal() and
verifying the submit button
(modelTransferJobsPage.findRetryModalSubmitButton()), click that submit button,
then cy.wait('@retryJob') and assert the request/response (status or request
body) as needed; alternatively remove the cy.intercept(...) alias if you choose
not to exercise the retry path—update the code around
interceptWithSingleFailedJob, the cy.intercept(...) alias '@retryJob', and
modelTransferJobsPage.findRetryModalSubmitButton() usage accordingly.
In `@clients/ui/frontend/src/app/pages/modelCatalog/screens/ModelDetailsView.tsx`:
- Around line 168-175: ModelDetailsView renders an actionable ExternalLink for
the license even when model.licenseLink is falsy, causing window.open('') and a
blank tab; update the JSX in the ModelDetailsView component to conditionally
render the ExternalLink only when model.licenseLink is truthy and otherwise
render a plain textual fallback (as done in McpServerDetailsView), using the
same testId "model-license-link" for the link case and ensuring no clickable
element is produced when licenseLink is missing.
In `@clients/ui/frontend/src/app/pages/modelCatalog/utils/modelCatalogUtils.ts`:
- Around line 753-766: getActiveSourceLabels is losing sources because
getUniqueSourceLabels treats sources with enabled === undefined as inactive
while filterEnabledCatalogSources already treats only enabled === false as
disabled; update getUniqueSourceLabels so it treats any source with enabled !==
false as active (i.e., consider undefined as enabled) when collecting labels,
ensuring behavior matches filterEnabledCatalogSources and preserving labeled
sources for hasSourcesWithoutLabels and ordering via orderLabelsByPriority.
In
`@clients/ui/frontend/src/app/pages/modelCatalogSettings/components/CredentialsSection.tsx`:
- Around line 130-147: accessTokenHelperTxtNode is always passed into
ThemeAwareFormGroupWrapper causing hasError to be true; move the static HF token
hint out of helperTextNode and into descriptionTextNode (or render it as a
sibling FormHelperText outside the wrapper) and only pass helperTextNode when
there is a real validation error. Update accessTokenFormGroup to use
accessTokenDescriptionTxtNode (or a combined description node that includes the
static hint) for the static message and only supply helperTextNode conditionally
(e.g., when validationError exists), leaving ThemeAwareFormGroupWrapper,
accessTokenHelperTxtNode, accessTokenDescriptionTxtNode and accessTokenFormGroup
as the reference points to change.
In `@clients/ui/frontend/src/app/pages/modelCatalogSettings/constants.tsx`:
- Around line 98-104: EMPTY_STATE_TEXT has inconsistent terminal punctuation
between NO_MODELS_INCLUDED_BODY (ends with a period) and NO_MODELS_EXCLUDED_BODY
(no period); update the NO_MODELS_EXCLUDED_BODY string in the EMPTY_STATE_TEXT
constant so it ends with a period to match NO_MODELS_INCLUDED_BODY and ensure
consistent presentation in PreviewPanel rendering.
---
Outside diff comments:
In
`@clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalogSettings/modelCatalogSettings.cy.ts`:
- Around line 436-448: The test title in the it(...) block for the case using
mockCatalogSource('hf-google') is stale — it says "should show \"Connected\"
status for available sources" but the assertion uses
row.shouldHaveValidationStatus('Ready') and checks for
'source-status-connected-hf-google'; update the test description to match the
assertion (e.g., change the it(...) string to "should show 'Ready' status for
available sources" or equivalent) so the test name reflects
modelCatalogSettings.getRow('HuggingFace Google') expectations and keeps
greppability accurate.
In
`@clients/ui/frontend/src/app/pages/mcpCatalog/screens/McpCatalogGalleryView.tsx`:
- Around line 54-62: The server query currently uses selectedSourceLabel which
may be undefined on first render; update the call to
useMcpServersBySourceLabelWithAPI so it prefers singleCategoryLabel when present
(e.g., sourceLabel: singleCategoryLabel || selectedSourceLabel) so
single-category mode always queries the correct category; make the same change
for the other occurrence around the use of useMcpServersBySourceLabelWithAPI
later in the file (lines with the second call).
In
`@clients/ui/frontend/src/app/pages/modelCatalog/screens/ModelCatalogGalleryView.tsx`:
- Around line 108-119: When calling useCatalogModelsBySources in
ModelCatalogGalleryView, ensure the hook receives the single-category label when
singleCategoryLabel is set so the initial request doesn't fetch all models;
replace the current source arg logic with one that prefers singleCategoryLabel
(e.g., use singleCategoryLabel ? singleCategoryLabel : (selectedSourceLabel ===
CategoryName.allModels ? undefined : selectedSourceLabel)). Apply the same
change to the other useCatalogModelsBySources call later in the file (the one
around the second occurrence).
In
`@clients/ui/frontend/src/app/pages/modelCatalogSettings/components/ModelVisibilitySection.tsx`:
- Around line 66-132: The two helper nodes allowedModelsHelperTxtNode and
excludedModelsHelperTxtNode are being passed into
ThemeAwareFormGroupWrapper.helperTextNode which forces pf-m-error styling;
delete those helper node variables, and instead merge their informational copy
(getIncludedModelsFieldHelperText / getExcludedModelsFieldHelperText) into the
corresponding descriptionTextNode (allowedModelsDescriptionTxtNode /
excludedModelsDescriptionTxtNode) so the informational text is rendered as
description, not an error. Update the two ThemeAwareFormGroupWrapper usages to
remove the helperTextNode prop entirely and supply the combined
descriptionTextNode (retain existing description generators like
getExcludedModelsHelp(organization) and append the helper text inside the same
FormHelperText/HelperText/HelperTextItem node). Ensure no other code references
allowedModelsHelperTxtNode or excludedModelsHelperTxtNode after the change.
---
Nitpick comments:
In `@clients/ui/bff/go.mod`:
- Around line 15-17: Update the Kubernetes module versions so they are
consistent: change the k8s.io/client-go module version from v0.35.3 to v0.35.4
to match k8s.io/api and k8s.io/apimachinery (both v0.35.4); after updating the
k8s.io/client-go version in go.mod run go mod tidy (or the equivalent dependency
refresh) to ensure the dependency tree is reconciled.
In `@clients/ui/bff/internal/integrations/kubernetes/k8mocks/base_testenv.go`:
- Line 350: The error message currently hardcodes "default-catalog-sources"
instead of referencing the shared constant; update the failing return in the
configmap creation path to use k8s.CatalogSourceDefaultConfigMapName (the same
constant used on the creation line) so the error text stays in sync with the
source-of-truth; locate the configmap creation function/return that currently
does `fmt.Errorf("failed to create default-catalog-sources configmap: %w", err)`
and replace the hardcoded name with the constant.
In
`@clients/ui/frontend/src/app/pages/modelCatalogSettings/components/CatalogSourceStatus.tsx`:
- Around line 54-59: In the CatalogSourceStatus.tsx branch for
CatalogSourceStatusEnum.AVAILABLE, update the stale data-testid value from
`source-status-connected-${catalogSourceConfig.id}` to
`source-status-ready-${catalogSourceConfig.id}` to match the label text change,
and then update the Cypress selector(s) in the corresponding model catalog
settings test that currently reference the old `source-status-connected-…` id to
the new `source-status-ready-…` id so tests remain in sync.
In `@clients/ui/frontend/src/app/pages/modelCatalogSettings/constants.tsx`:
- Around line 134-140: Rename the misleading constants
getIncludedModelsFieldHelperText and getExcludedModelsFieldHelperText to
non-getter names (e.g., includedModelsFieldHelperText and
excludedModelsFieldHelperText) and update all usages to the new identifiers
(including JSX embeddings like where ModelVisibilitySection references them) so
callers don't accidentally try to invoke them as functions; keep the string
values unchanged and ensure any exports/imports are updated accordingly.
In
`@clients/ui/frontend/src/app/pages/modelRegistry/screens/ModelTransferJobs/ModelTransferJobTableRow.tsx`:
- Around line 164-179: Extract the inline style on the PF Button in
ModelTransferJobTableRow (the Button rendered when job.status ===
ModelTransferJobStatus.FAILED) into a dedicated CSS class (e.g.,
mr-transfer-job-error-link), remove the style prop and apply the class via
className on the Button, add the CSS rule (text-decoration: underline dotted;
text-underline-offset: var(--pf-t--global--spacer--xs);) to the component
stylesheet or CSS module, and ensure the stylesheet is imported (and class
referenced) where the Button and Truncate are used so the visual rule is
centralized and not repeated per row.
In `@jobs/async-upload/tests/integration/test_integration_async_upload.py`:
- Line 214: The test duplicates the literal "default" namespace in multiple
kubectl invocations (apply, delete, describe, wait/log and cleanup) in
test_integration_async_upload.py; introduce a single constant (e.g., NAMESPACE =
"default") at the top of the test module or the test function and replace all
occurrences of the string literal in the kubectl command lists (the arrays used
for ["kubectl", "apply", "-k", ".", "-n", "default"], ["kubectl", "delete", ...
"-n", "default"], and the describe/wait/cleanup command arrays) with that
constant so future namespace changes only require one edit.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 81faf559-86e5-46d0-ad30-718d5a33ebe7
⛔ Files ignored due to path filters (1)
clients/ui/bff/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (36)
.github/workflows/go-mod-tidy-diff-check.ymlcatalog/clients/python/patches/include-readonly-timestamps.patchclients/ui/bff/go.modclients/ui/bff/internal/integrations/kubernetes/client.goclients/ui/bff/internal/integrations/kubernetes/k8mocks/base_testenv.goclients/ui/frontend/src/__mocks__/mockCatalogSourceList.tsclients/ui/frontend/src/__tests__/cypress/cypress/pages/modelCatalog.tsclients/ui/frontend/src/__tests__/cypress/cypress/pages/modelCatalogSettings.tsclients/ui/frontend/src/__tests__/cypress/cypress/pages/modelRegistryView/modelTransferJobs.tsclients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/modelCatalog.cy.tsclients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/modelCatalogDetails.cy.tsclients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalogSettings/modelCatalogSettings.cy.tsclients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelTransferJobs.cy.tsclients/ui/frontend/src/app/pages/mcpCatalog/screens/McpCatalog.tsxclients/ui/frontend/src/app/pages/mcpCatalog/screens/McpCatalogGalleryView.tsxclients/ui/frontend/src/app/pages/mcpCatalog/screens/McpCatalogSourceLabelBlocks.tsxclients/ui/frontend/src/app/pages/modelCatalog/screens/ModelCatalog.tsxclients/ui/frontend/src/app/pages/modelCatalog/screens/ModelCatalogGalleryView.tsxclients/ui/frontend/src/app/pages/modelCatalog/screens/ModelCatalogSourceLabelBlocks.tsxclients/ui/frontend/src/app/pages/modelCatalog/screens/ModelDetailsView.tsxclients/ui/frontend/src/app/pages/modelCatalog/utils/__tests__/modelCatalogUtils.spec.tsclients/ui/frontend/src/app/pages/modelCatalog/utils/modelCatalogUtils.tsclients/ui/frontend/src/app/pages/modelCatalogSettings/components/CatalogSourceStatus.tsxclients/ui/frontend/src/app/pages/modelCatalogSettings/components/CatalogSourceStatusErrorModal.tsxclients/ui/frontend/src/app/pages/modelCatalogSettings/components/CredentialsSection.tsxclients/ui/frontend/src/app/pages/modelCatalogSettings/components/ManageSourceForm.tsxclients/ui/frontend/src/app/pages/modelCatalogSettings/components/ManageSourceFormFooter.tsxclients/ui/frontend/src/app/pages/modelCatalogSettings/components/ModelVisibilitySection.tsxclients/ui/frontend/src/app/pages/modelCatalogSettings/components/PreviewPanel.tsxclients/ui/frontend/src/app/pages/modelCatalogSettings/components/YamlSection.tsxclients/ui/frontend/src/app/pages/modelCatalogSettings/components/__tests__/PreviewPanel.spec.tsxclients/ui/frontend/src/app/pages/modelCatalogSettings/constants.tsxclients/ui/frontend/src/app/pages/modelCatalogSettings/screens/CatalogSourceConfigsTableColumns.tsxclients/ui/frontend/src/app/pages/modelRegistry/screens/ModelTransferJobs/ModelTransferJobTableRow.tsxjobs/async-upload/tests/integration/test_integration_async_upload.pymanifests/kustomize/options/catalog/overlays/odh/kustomization.yaml
Comment on lines
+4
to
+11
| * OpenAPI `readOnly` fields are excluded. | ||
| * OpenAPI `readOnly` fields are excluded. | ||
| """ | ||
| - excluded_fields: Set[str] = set([ | ||
| - "create_time_since_epoch", | ||
| - "last_update_time_since_epoch", | ||
| - ]) | ||
| + excluded_fields: Set[str] = set([]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Find all call sites of CatalogModel.to_dict / model serialization into requests.
rg -nP -C3 '\bCatalogModel\b' --type=py
rg -nP -C2 'obj\.to_dict\(\)|\.to_dict\(\)' catalog/clients/python --type=py
# 2. Check whether other models in the generated client still exclude these fields.
rg -nP -C2 '"(create_time_since_epoch|last_update_time_since_epoch)"' catalog/clients/python --type=py
# 3. Look at the OpenAPI spec to confirm readOnly status.
fd -t f -e yaml -e yml -e json . catalog | xargs rg -nP -l 'create_time_since_epoch|last_update_time_since_epoch' 2>/dev/nullRepository: opendatahub-io/model-registry
Length of output: 50387
🏁 Script executed:
# Check the actual CatalogModel.to_dict implementation (after patch applied)
head -150 catalog/clients/python/src/catalog_openapi/models/catalog_model.pyRepository: opendatahub-io/model-registry
Length of output: 6657
🏁 Script executed:
# Check BaseResource.to_dict to see if it excludes create_time_since_epoch and last_update_time_since_epoch
cat -n catalog/clients/python/src/catalog_openapi/models/base_resource.py | head -100Repository: opendatahub-io/model-registry
Length of output: 5245
🏁 Script executed:
# Search for excluded_fields in BaseResource specifically
rg -A10 "excluded_fields" catalog/clients/python/src/catalog_openapi/models/base_resource.pyRepository: opendatahub-io/model-registry
Length of output: 845
🏁 Script executed:
# Look for the OpenAPI spec file(s) with timestamp definitions
fd -t f \( -name "*.yaml" -o -name "*.yml" -o -name "*.json" \) | head -20Repository: opendatahub-io/model-registry
Length of output: 303
🏁 Script executed:
# Find OpenAPI spec files
find catalog -type f \( -name "*.yaml" -o -name "*.yml" -o -name "*.json" \) | grep -i -E "(openapi|spec|swagger)" | head -10Repository: opendatahub-io/model-registry
Length of output: 139
🏁 Script executed:
# Search for OpenAPI spec with timestamp fields
rg -l "create_time_since_epoch|createTimeSinceEpoch" catalog --type yaml --type jsonRepository: opendatahub-io/model-registry
Length of output: 441
🏁 Script executed:
# Check class hierarchy - does CatalogModel inherit from BaseResource?
rg -B5 "class CatalogModel" catalog/clients/python/src/catalog_openapi/models/catalog_model.pyRepository: opendatahub-io/model-registry
Length of output: 205
🏁 Script executed:
# Search for API client usage: how is CatalogModel.to_dict() used in request bodies?
rg -B3 -A3 "\.to_dict\(\)" catalog/clients/python/src/catalog_openapi/api_client.py | head -50Repository: opendatahub-io/model-registry
Length of output: 334
🏁 Script executed:
# Find the main OpenAPI spec definition
find catalog -type f -name "*.yaml" -o -name "*.yml" | xargs rg -l "createTimeSinceEpoch|create_time_since_epoch" 2>/dev/null | head -5Repository: opendatahub-io/model-registry
Length of output: 441
🏁 Script executed:
# Check the openapitools.json configuration
cat catalog/openapitools.json | head -50Repository: opendatahub-io/model-registry
Length of output: 226
🏁 Script executed:
# Search for readOnly in OpenAPI schema definition
rg "readOnly|read_only" catalog --type yaml --type json -B2 -A2 | head -100Repository: opendatahub-io/model-registry
Length of output: 55
🏁 Script executed:
# Look for the actual OpenAPI spec in common locations
find catalog -type f \( -name "*.proto" -o -name "*.proto.yaml" \) | head -10Repository: opendatahub-io/model-registry
Length of output: 55
🏁 Script executed:
# Search for go server files that might define the schema
find catalog -path "*/server/*" -name "*.go" | head -5Repository: opendatahub-io/model-registry
Length of output: 370
🏁 Script executed:
# Check if there's a go service definition
find catalog -name "*service*.go" | head -5Repository: opendatahub-io/model-registry
Length of output: 383
🏁 Script executed:
# Look at the patch file directly to understand context
cat -n catalog/clients/python/patches/include-readonly-timestamps.patchRepository: opendatahub-io/model-registry
Length of output: 698
🏁 Script executed:
# Check the Go OpenAPI API definition for CatalogModel/Model schema
grep -A30 "type.*Model" catalog/internal/server/openapi/api_model_catalog_service.go | head -60Repository: opendatahub-io/model-registry
Length of output: 1266
🏁 Script executed:
# Search for the protobuf or swagger definition
find catalog -name "*.proto" | head -5Repository: opendatahub-io/model-registry
Length of output: 55
🏁 Script executed:
# Look for generated swagger/OpenAPI spec
find catalog -type f -name "swagger.json" -o -name "openapi.json" -o -name "openapi.yaml"Repository: opendatahub-io/model-registry
Length of output: 55
🏁 Script executed:
# Check git log for context on this patch
git log --oneline --all --grep="readOnly\|readonly\|timestamp" 2>/dev/null | head -10Repository: opendatahub-io/model-registry
Length of output: 685
🏁 Script executed:
# Search for server validation logic that might reject readOnly fields
rg "readOnly|readonly" catalog/internal --type go -B2 -A2 | head -50Repository: opendatahub-io/model-registry
Length of output: 4698
🏁 Script executed:
# Check the actual patch hunk context - what's shown in lines 4-6 before the change
git show HEAD:catalog/clients/python/src/catalog_openapi/models/catalog_model.py | sed -n '75,90p'Repository: opendatahub-io/model-registry
Length of output: 768
🏁 Script executed:
# Verify CatalogModel and BaseResource are separate classes (not inheritance)
rg -A2 "class BaseResource|class CatalogModel" catalog/clients/python/src/catalog_openapi/models/*.py | grep -E "(class|inherit)"Repository: opendatahub-io/model-registry
Length of output: 706
🏁 Script executed:
# Look for API request body generation - does it call to_dict on CatalogModel?
rg "request_body|body.*to_dict|to_dict.*body" catalog/clients/python/src/catalog_openapi --type py -B2 -A2 | head -30Repository: opendatahub-io/model-registry
Length of output: 1979
🏁 Script executed:
# Check if there's any validation logic in Go service that rejects readOnly fields
rg "create_time_since_epoch|createTimeSinceEpoch" catalog/internal --type go -B5 -A5 | head -60Repository: opendatahub-io/model-registry
Length of output: 5509
🏁 Script executed:
# Look at the api_client code where body is prepared for requests
cat -n catalog/clients/python/src/catalog_openapi/api_client.py | sed -n '200,250p'Repository: opendatahub-io/model-registry
Length of output: 1888
🏁 Script executed:
# Search for any request validation or field stripping in the Go backend
rg "readOnly|read.only" catalog/internal/server --type go -B3 -A3 | head -50Repository: opendatahub-io/model-registry
Length of output: 4772
CatalogModel and BaseResource now serialize timestamps differently, and BaseResource docstring is duplicated.
The patch intentionally includes create_time_since_epoch and last_update_time_since_epoch in CatalogModel's serialization for e2e test coverage (docstring explains this at lines 82–86). However, BaseResource's to_dict() still excludes both fields. Since CatalogModel and BaseResource are independent Pydantic models (not parent-child), this behavioral divergence is not an inheritance issue, but it creates type-dependent serialization that can surprise users: the same timestamp fields serialize or exclude depending on which model type holds them.
Additionally, BaseResource's docstring at lines 68–69 duplicates "OpenAPI readOnly fields are excluded."—remove one instance.
If the goal is exposing timestamps on reads while excluding them on writes, consider whether CatalogModel and BaseResource should have consistent behavior, or document clearly why they differ.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@catalog/clients/python/patches/include-readonly-timestamps.patch` around
lines 4 - 11, Remove the duplicated docstring line in BaseResource and make
BaseResource.to_dict()'s excluded_fields consistent with CatalogModel by
ensuring create_time_since_epoch and last_update_time_since_epoch are not in the
excluded_fields set (so timestamps are serialized on reads); update the
excluded_fields variable in BaseResource.to_dict() accordingly and keep only one
instance of the "OpenAPI `readOnly` fields are excluded." sentence in the
docstring.
Comment on lines
+550
to
+558
| it('should hide category toggle when only one category is active', () => { | ||
| initIntercepts({ | ||
| sources: [mockCatalogSource({ labels: ['Provider one'] })], | ||
| }); | ||
| modelCatalog.visit(); | ||
|
|
||
| cy.findByTestId('label-Provider one').should('not.exist'); | ||
| cy.findByTestId('all').should('not.exist'); | ||
| }); |
There was a problem hiding this comment.
Gate the negative assertions on a loaded catalog.
These not.exist checks can pass before the selector renders. Wait for a positive loaded signal first so the test actually verifies the toggle is hidden after data loads.
Proposed fix
});
modelCatalog.visit();
+ modelCatalog.findModelCatalogCards().should('have.length.at.least', 1);
cy.findByTestId('label-Provider one').should('not.exist');
cy.findByTestId('all').should('not.exist');📝 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
| it('should hide category toggle when only one category is active', () => { | |
| initIntercepts({ | |
| sources: [mockCatalogSource({ labels: ['Provider one'] })], | |
| }); | |
| modelCatalog.visit(); | |
| cy.findByTestId('label-Provider one').should('not.exist'); | |
| cy.findByTestId('all').should('not.exist'); | |
| }); | |
| it('should hide category toggle when only one category is active', () => { | |
| initIntercepts({ | |
| sources: [mockCatalogSource({ labels: ['Provider one'] })], | |
| }); | |
| modelCatalog.visit(); | |
| modelCatalog.findModelCatalogCards().should('have.length.at.least', 1); | |
| cy.findByTestId('label-Provider one').should('not.exist'); | |
| cy.findByTestId('all').should('not.exist'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/modelCatalog.cy.ts`
around lines 550 - 558, The negative assertions run before the catalog finishes
rendering; after calling initIntercepts(...) and modelCatalog.visit(), wait for
a positive "catalog loaded" signal before asserting absence of the toggles —
e.g., use an existing helper or element (for example call
modelCatalog.waitForLoad() if available or assert
cy.findByTestId('model-catalog-list').should('exist')) and only then check
cy.findByTestId('label-Provider one').should('not.exist') and
cy.findByTestId('all').should('not.exist').
Comment on lines
+572
to
+589
| it('should show retry button for failed jobs and open retry modal on click', () => { | ||
| interceptWithSingleFailedJob('job-retry-test', 'Connection error'); | ||
|
|
||
| cy.intercept( | ||
| 'PUT', | ||
| `**/api/${MODEL_REGISTRY_API_VERSION}/model_registry/${modelRegistryName}/model_transfer_jobs/job-retry-test*`, | ||
| { statusCode: 200, body: {} }, | ||
| ).as('retryJob'); | ||
|
|
||
| modelTransferJobsPage.visit(modelRegistryName); | ||
|
|
||
| const row = modelTransferJobsPage.getRow('job-retry-test'); | ||
| row.findRetryButton().should('be.visible').click(); | ||
|
|
||
| modelTransferJobsPage.findRetryModal().should('be.visible'); | ||
| modelTransferJobsPage.findRetryModal().contains('Retry model transfer job?').should('exist'); | ||
| modelTransferJobsPage.findRetryModalSubmitButton().should('be.visible'); | ||
| }); |
There was a problem hiding this comment.
Retry test does not assert the PUT request is fired.
@retryJob is registered on line 579 but never cy.wait('@retryJob')-ed. The test only opens the modal and checks the submit button is visible — it never clicks it, so the intercept is dead weight and the actual retry path is untested. Either drop the intercept or click the submit button and assert the request payload/status.
🧪 Suggested extension
modelTransferJobsPage.findRetryModal().should('be.visible');
modelTransferJobsPage.findRetryModal().contains('Retry model transfer job?').should('exist');
- modelTransferJobsPage.findRetryModalSubmitButton().should('be.visible');
+ modelTransferJobsPage.findRetryModalSubmitButton().should('be.visible').click();
+ cy.wait('@retryJob').its('response.statusCode').should('eq', 200);
+ modelTransferJobsPage.findRetryModal().should('not.exist');
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelTransferJobs.cy.ts`
around lines 572 - 589, The test registers an intercept alias '@retryJob' but
never triggers or asserts it; update the test in the it('should show retry
button...') block to actually submit the retry and wait for the PUT request:
after opening the retry modal via modelTransferJobsPage.findRetryModal() and
verifying the submit button
(modelTransferJobsPage.findRetryModalSubmitButton()), click that submit button,
then cy.wait('@retryJob') and assert the request/response (status or request
body) as needed; alternatively remove the cy.intercept(...) alias if you choose
not to exercise the retry path—update the code around
interceptWithSingleFailedJob, the cy.intercept(...) alias '@retryJob', and
modelTransferJobsPage.findRetryModalSubmitButton() usage accordingly.
Comment on lines
168
to
+175
| <DescriptionListTerm>License</DescriptionListTerm> | ||
| <ExternalLink | ||
| text="Agreement" | ||
| to={model.licenseLink || ''} | ||
| testId="model-license-link" | ||
| /> | ||
| <DescriptionListDescription> | ||
| <ExternalLink | ||
| text="Agreement" | ||
| to={model.licenseLink || ''} | ||
| testId="model-license-link" | ||
| /> | ||
| </DescriptionListDescription> |
There was a problem hiding this comment.
Broken "Agreement" link when licenseLink is missing.
to={model.licenseLink || ''} still renders a clickable "Agreement" button even when no license URL exists; clicking invokes window.open(''), which opens a blank tab — a confusing UX regression and an accessibility issue (an actionable control that does nothing meaningful). Mirror the pattern already used in McpServerDetailsView.tsx (lines 160-171): render the link only when licenseLink is truthy, otherwise show a textual fallback.
🔧 Proposed fix
<DescriptionListGroup>
<DescriptionListTerm>License</DescriptionListTerm>
<DescriptionListDescription>
- <ExternalLink
- text="Agreement"
- to={model.licenseLink || ''}
- testId="model-license-link"
- />
+ {model.licenseLink ? (
+ <ExternalLink
+ text="Agreement"
+ to={model.licenseLink}
+ testId="model-license-link"
+ />
+ ) : (
+ <span data-testid="model-license">N/A</span>
+ )}
</DescriptionListDescription>
</DescriptionListGroup>📝 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
| <DescriptionListTerm>License</DescriptionListTerm> | |
| <ExternalLink | |
| text="Agreement" | |
| to={model.licenseLink || ''} | |
| testId="model-license-link" | |
| /> | |
| <DescriptionListDescription> | |
| <ExternalLink | |
| text="Agreement" | |
| to={model.licenseLink || ''} | |
| testId="model-license-link" | |
| /> | |
| </DescriptionListDescription> | |
| <DescriptionListTerm>License</DescriptionListTerm> | |
| <DescriptionListDescription> | |
| {model.licenseLink ? ( | |
| <ExternalLink | |
| text="Agreement" | |
| to={model.licenseLink} | |
| testId="model-license-link" | |
| /> | |
| ) : ( | |
| <span data-testid="model-license">N/A</span> | |
| )} | |
| </DescriptionListDescription> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/ui/frontend/src/app/pages/modelCatalog/screens/ModelDetailsView.tsx`
around lines 168 - 175, ModelDetailsView renders an actionable ExternalLink for
the license even when model.licenseLink is falsy, causing window.open('') and a
blank tab; update the JSX in the ModelDetailsView component to conditionally
render the ExternalLink only when model.licenseLink is truthy and otherwise
render a plain textual fallback (as done in McpServerDetailsView), using the
same testId "model-license-link" for the link case and ensuring no clickable
element is produced when licenseLink is missing.
Comment on lines
+753
to
+766
| export const getActiveSourceLabels = ( | ||
| catalogSources: CatalogSourceList | null, | ||
| catalogLabels: CatalogLabelList | null, | ||
| ): string[] => { | ||
| const enabledSources = filterEnabledCatalogSources(catalogSources); | ||
| const uniqueLabels = getUniqueSourceLabels(enabledSources); | ||
| const orderedLabels = orderLabelsByPriority(uniqueLabels, catalogLabels); | ||
|
|
||
| if (hasSourcesWithoutLabels(enabledSources)) { | ||
| return [...orderedLabels, SourceLabel.other]; | ||
| } | ||
|
|
||
| return orderedLabels; | ||
| }; |
There was a problem hiding this comment.
Treat omitted enabled as active consistently.
filterEnabledCatalogSources keeps sources unless enabled === false, but getUniqueSourceLabels later drops sources where enabled is undefined. Since enabled is optional, available labeled sources can disappear from activeCategories, causing false empty states or missed single-category selection.
Proposed fix
- if (source.enabled && isSourceStatusWithModels(source.status) && source.labels.length > 0) {
+ if (source.enabled !== false && isSourceStatusWithModels(source.status) && source.labels.length > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/ui/frontend/src/app/pages/modelCatalog/utils/modelCatalogUtils.ts`
around lines 753 - 766, getActiveSourceLabels is losing sources because
getUniqueSourceLabels treats sources with enabled === undefined as inactive
while filterEnabledCatalogSources already treats only enabled === false as
disabled; update getUniqueSourceLabels so it treats any source with enabled !==
false as active (i.e., consider undefined as enabled) when collecting labels,
ensuring behavior matches filterEnabledCatalogSources and preserving labeled
sources for hasSourcesWithoutLabels and ordering via orderLabelsByPriority.
Comment on lines
+130
to
+147
| const accessTokenHelperTxtNode = ( | ||
| <FormHelperText> | ||
| <HelperText> | ||
| <HelperTextItem>{HELPER_TEXT.ACCESS_TOKEN}</HelperTextItem> | ||
| </HelperText> | ||
| </FormHelperText> | ||
| ); | ||
|
|
||
| const accessTokenFormGroup = ( | ||
| <> | ||
| <FormGroup label={FORM_LABELS.ACCESS_TOKEN} fieldId="access-token"> | ||
| <FormHelperText> | ||
| <HelperText> | ||
| <HelperTextItem>{HELP_TEXT.ACCESS_TOKEN}</HelperTextItem> | ||
| </HelperText> | ||
| </FormHelperText> | ||
| <FormFieldset component={accessTokenInput} field="Access token" /> | ||
| </FormGroup> | ||
| <ThemeAwareFormGroupWrapper | ||
| label={FORM_LABELS.ACCESS_TOKEN} | ||
| fieldId="access-token" | ||
| descriptionTextNode={accessTokenDescriptionTxtNode} | ||
| helperTextNode={accessTokenHelperTxtNode} | ||
| > | ||
| {accessTokenInput} | ||
| </ThemeAwareFormGroupWrapper> |
There was a problem hiding this comment.
Access-token field will be stuck in error styling.
accessTokenHelperTxtNode is defined unconditionally (lines 130–136) and passed as helperTextNode to the wrapper at line 144. Per ThemeAwareFormGroupWrapper, hasError = !!helperTextNode — so the access-token FormGroup will always receive pf-m-error regardless of validation state, mis-communicating a failure to the user and conflicting with the separate validationError Alert below.
Use descriptionTextNode (or a combined description node) for the static HF token hint, and only populate helperTextNode on an actual error condition.
🔧 Suggested fix
- const accessTokenHelperTxtNode = (
- <FormHelperText>
- <HelperText>
- <HelperTextItem>{HELPER_TEXT.ACCESS_TOKEN}</HelperTextItem>
- </HelperText>
- </FormHelperText>
- );
-
const accessTokenFormGroup = (
<>
<ThemeAwareFormGroupWrapper
label={FORM_LABELS.ACCESS_TOKEN}
fieldId="access-token"
descriptionTextNode={accessTokenDescriptionTxtNode}
- helperTextNode={accessTokenHelperTxtNode}
>
{accessTokenInput}
</ThemeAwareFormGroupWrapper>
+ <FormHelperText>
+ <HelperText>
+ <HelperTextItem>{HELPER_TEXT.ACCESS_TOKEN}</HelperTextItem>
+ </HelperText>
+ </FormHelperText>Note: the organization block (lines 90–107) already follows this pattern correctly — ORGANIZATION_SLUG is rendered as a sibling FormHelperText outside the wrapper, and organizationHelperTxtNode is conditional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clients/ui/frontend/src/app/pages/modelCatalogSettings/components/CredentialsSection.tsx`
around lines 130 - 147, accessTokenHelperTxtNode is always passed into
ThemeAwareFormGroupWrapper causing hasError to be true; move the static HF token
hint out of helperTextNode and into descriptionTextNode (or render it as a
sibling FormHelperText outside the wrapper) and only pass helperTextNode when
there is a real validation error. Update accessTokenFormGroup to use
accessTokenDescriptionTxtNode (or a combined description node that includes the
static hint) for the static message and only supply helperTextNode conditionally
(e.g., when validationError exists), leaving ThemeAwareFormGroupWrapper,
accessTokenHelperTxtNode, accessTokenDescriptionTxtNode and accessTokenFormGroup
as the reference points to change.
Comment on lines
+98
to
+104
| export const EMPTY_STATE_TEXT = { | ||
| NO_MODELS_INCLUDED: 'No models included', | ||
| NO_MODELS_INCLUDED_BODY: | ||
| 'No models from this source are visible in the model catalog. To include models, edit the model visibility settings of this source.', | ||
| NO_MODELS_EXCLUDED: 'No models excluded', | ||
| NO_MODELS_EXCLUDED_BODY: 'No models from this source are excluded by this filter', | ||
| } as const; |
There was a problem hiding this comment.
Inconsistent terminal punctuation in EMPTY_STATE_TEXT.
NO_MODELS_INCLUDED_BODY ends with a period; NO_MODELS_EXCLUDED_BODY does not. Minor, but since these are rendered side-by-side via PreviewPanel.tsx (lines 189–191), the inconsistency will be visible to users.
- NO_MODELS_EXCLUDED_BODY: 'No models from this source are excluded by this filter',
+ NO_MODELS_EXCLUDED_BODY: 'No models from this source are excluded by this filter.',📝 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 EMPTY_STATE_TEXT = { | |
| NO_MODELS_INCLUDED: 'No models included', | |
| NO_MODELS_INCLUDED_BODY: | |
| 'No models from this source are visible in the model catalog. To include models, edit the model visibility settings of this source.', | |
| NO_MODELS_EXCLUDED: 'No models excluded', | |
| NO_MODELS_EXCLUDED_BODY: 'No models from this source are excluded by this filter', | |
| } as const; | |
| export const EMPTY_STATE_TEXT = { | |
| NO_MODELS_INCLUDED: 'No models included', | |
| NO_MODELS_INCLUDED_BODY: | |
| 'No models from this source are visible in the model catalog. To include models, edit the model visibility settings of this source.', | |
| NO_MODELS_EXCLUDED: 'No models excluded', | |
| NO_MODELS_EXCLUDED_BODY: 'No models from this source are excluded by this filter.', | |
| } as const; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/ui/frontend/src/app/pages/modelCatalogSettings/constants.tsx` around
lines 98 - 104, EMPTY_STATE_TEXT has inconsistent terminal punctuation between
NO_MODELS_INCLUDED_BODY (ends with a period) and NO_MODELS_EXCLUDED_BODY (no
period); update the NO_MODELS_EXCLUDED_BODY string in the EMPTY_STATE_TEXT
constant so it ends with a period to match NO_MODELS_INCLUDED_BODY and ensure
consistent presentation in PreviewPanel rendering.
Member
Author
|
/ok-to-test |
|
@jonburdo: you cannot LGTM your own PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
dbasunag
approved these changes
Apr 22, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dbasunag, jonburdo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 754a8fb
into
opendatahub-io:main
36 of 38 checks passed
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
6 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.
supersedes #1713
to fix merge conflicts
Description
How Has This Been Tested?
Merge criteria:
DCOcheck)ok-to-testhas been added to the PR.If you have UI changes
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
UI/UX Improvements