RHOAIENG-52233: Unified Models Tab: Merge MaaS and AI Models into Single View#6628
Open
Lucifergene wants to merge 7 commits intoopendatahub-io:mainfrom
Open
RHOAIENG-52233: Unified Models Tab: Merge MaaS and AI Models into Single View#6628Lucifergene wants to merge 7 commits intoopendatahub-io:mainfrom
Lucifergene wants to merge 7 commits intoopendatahub-io:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request consolidates MaaS models into the main AI models interface by removing the separate MaaS tab and merging data sources. Backend changes add Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Security ConsiderationsCWE-200 (Exposure of Sensitive Information): Backend creates endpoint labels with "internal" and "external" prefixes. Verify cluster-local URL detection logic in CWE-89 (SQL Injection / Query Injection): Filter implementation in CWE-400 (Uncontrolled Resource Consumption): New CWE-327 (Use of Broken/Risky Cryptographic Algorithm): Backend migrates URLs from Logical IssuesRemoved Deduplication: Filter State Mutation: Endpoint Parsing Reliance: New MaaS Model Conversion Loss: 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationModal.tsx (1)
56-65:⚠️ Potential issue | 🟠 MajorHandle potential duplicate models in merged array.
The previous filtering logic that excluded MaaS models already present in
aiModelshas been removed. TheallModelsarray now simply concatenates both sources without deduplication, which could result in duplicate entries if a model exists in bothaiModelsandmaasModels.The
useMergedModelshook (which is used elsewhere in the codebase) explicitly does not deduplicate either. Ensure that either:
- The upstream sources (via
useFetchAIModelsanduseFetchMaaSModels) are guaranteed to have no overlapping models, or- Deduplication logic is restored here as a defensive measure to prevent duplicate entries in the UI
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationModal.tsx` around lines 56 - 65, The merged models array currently concatenates aiModels and maasAsAIModels into allModels and can produce duplicates; modify the logic around allModels (and keep maasAsAIModels/convertMaaSModelToAIModel references) to deduplicate entries defensively — e.g., build allModels from [...aiModels, ...maasAsAIModels] but filter by a unique key (model id or name) to remove duplicates already present in aiModels (or consult useFetchAIModels/useFetchMaaSModels invariants if you prefer to rely on upstream guarantees); ensure the dedupe uses the same identity field used elsewhere (matching useMergedModels behavior) so the UI shows each model once.packages/gen-ai/bff/internal/repositories/external_models.go (1)
57-71:⚠️ Potential issue | 🟠 MajorPopulate
model_typein the create response.
AAModelnow exposesmodel_type, but this constructor still returns it unset. That makes the create response inconsistent with the unified models list until the next refetch.Suggested fix
return &models.AAModel{ ModelName: req.ModelID, ModelID: req.ModelID, ServingRuntime: string(req.ProviderType), APIProtocol: "REST", Version: "", Usecase: req.UseCases, Description: "", Endpoints: []string{endpoint}, Status: "Running", DisplayName: req.ModelDisplayName, SAToken: models.SAToken{}, ModelSourceType: sourceType, + ModelType: models.DeriveModelTypeFromUsecase(req.UseCases), }, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/bff/internal/repositories/external_models.go` around lines 57 - 71, In the AAModel return in external_models.go, populate the newly added ModelType field so the create response matches the unified models list; set models.AAModel.ModelType to the value from the request (e.g., req.ModelType) or a sensible fallback (empty string) if absent, keeping ModelSourceType as sourceType to preserve the current source mapping.packages/gen-ai/bff/openapi/src/gen-ai.yaml (1)
552-566:⚠️ Potential issue | 🟡 MinorMake the
model_typecontract explicit.This PR promotes
model_typeto first-class UI metadata, but both response schemas still leave it out ofrequired. That keeps generated clients and downstream types ambiguous right when the merged table starts depending on the field.Also applies to: 2043-2052, 2085-2094
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/bff/openapi/src/gen-ai.yaml` around lines 552 - 566, The OpenAPI response schemas declare the model_type property but don't mark it required, causing generated clients to treat it as optional; update each response schema that defines model_type (the schemas near the blocks where model_type is declared and the two other occurrences referenced in the review) by adding "model_type" to their required arrays so the property is treated as mandatory in generated types and clients (look for the schema objects that contain the model_type property and add "model_type" to their required list alongside id/object/created/owned_by/ready/url).
🧹 Nitpick comments (12)
packages/gen-ai/frontend/src/app/AIAssets/data/filterOptions.ts (1)
25-29: Tighten this map's type to the keys that actually have select values.
Record<string, string[]>says every lookup returns an array, butNAMEandUSE_CASEare not present here. A generic consumer can index those keys and getundefinedat runtime with no type warning.♻️ Proposed type-safe change
-export const assetsFilterSelectOptions: Record<string, string[]> = { +export const assetsFilterSelectOptions: Partial<Record<AssetsFilterOptions, string[]>> = { [AssetsFilterOptions.SOURCE]: ['MaaS', 'External', 'Public route', 'Internal'], [AssetsFilterOptions.STATUS]: ['Active', 'Inactive'], [AssetsFilterOptions.MODEL_TYPE]: ['Inferencing', 'Embedding'], };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/data/filterOptions.ts` around lines 25 - 29, The map's type is too broad (Record<string, string[]>) and allows indexing with AssetsFilterOptions.NAME or USE_CASE without a type error; tighten the type of assetsFilterSelectOptions to only the keys that actually have select values by replacing Record<string, string[]> with a keyed Record using the specific enum members (e.g. Record<AssetsFilterOptions.SOURCE | AssetsFilterOptions.STATUS | AssetsFilterOptions.MODEL_TYPE, string[]>), so assetsFilterSelectOptions and any consumers referencing AssetsFilterOptions.NAME or USE_CASE will get a compile-time error instead of possible undefined at runtime.packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationTableRow.tsx (1)
131-141: Consider extracting the source label to avoid redundant calls.
getSourceLabel(model)is called three times within this render block. While likely not a performance issue in practice, extracting it to a local variable would improve readability and avoid redundant computation.♻️ Suggested improvement
+ const sourceLabel = getSourceLabel(model); - {getSourceLabel(model) !== 'Internal' && ( + {sourceLabel !== 'Internal' && ( <FlexItem> <Label - color={getSourceLabelColor(getSourceLabel(model))} + color={getSourceLabelColor(sourceLabel)} isCompact data-testid="model-source-badge" > - {getSourceLabel(model)} + {sourceLabel} </Label> </FlexItem> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationTableRow.tsx` around lines 131 - 141, In ChatbotConfigurationTableRow, avoid calling getSourceLabel(model) multiple times in the render block: compute const sourceLabel = getSourceLabel(model) once above the JSX and replace the three calls (the conditional check, the color lookup getSourceLabelColor(...), and the rendered label text) with sourceLabel and getSourceLabelColor(sourceLabel) to improve readability and avoid redundant computation.packages/gen-ai/bff/internal/models/maas_models.go (1)
17-21: Minor: Comment says "Returns" but method has no return value.The comment states "Returns 'embedding' if usecase contains..." but the method mutates the receiver and returns nothing. Consider updating the comment to reflect the actual behavior.
📝 Suggested comment fix
-// DeriveModelType sets ModelType based on the Usecase field. -// Returns "embedding" if usecase contains "embedding" (case-insensitive), otherwise "llm". +// DeriveModelType sets ModelType based on the Usecase field. +// Sets ModelType to "embedding" if usecase contains "embedding" (case-insensitive), otherwise "llm". func (m *MaaSModel) DeriveModelType() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/bff/internal/models/maas_models.go` around lines 17 - 21, The comment for MaaSModel.DeriveModelType is misleading because it says "Returns 'embedding'..." even though the method mutates the receiver and returns nothing; update the docstring for DeriveModelType to describe the mutation (e.g., "Sets ModelType to 'embedding' if Usecase contains 'embedding' (case-insensitive), otherwise sets it to 'llm'") and reference that it uses DeriveModelTypeFromUsecase to compute the value so readers understand behavior.packages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/aiAssets/modelsTab.cy.ts (1)
89-95: Cover the embedding-model playground restriction here.This row already exercises
model_type: 'embedding', but the spec never checks the playground cell. Add an assertion for the disabled/unavailable playground state so that behavior can’t regress silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/aiAssets/modelsTab.cy.ts` around lines 89 - 95, The test verifies the embedding model row but misses asserting the playground restriction; update the block using the existing embeddingRow variable to assert the playground is disabled/unavailable (e.g., call embeddingRow.findPlaygroundCell().should('contain.text', 'Unavailable') or embeddingRow.findPlaygroundButton().should('be.disabled') depending on available helpers). Ensure you use the same row helper (embeddingRow) and add the playground assertion after the existing status check so the embedding-model playground restriction is covered.packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationModal.spec.tsx (1)
231-250: This doesn't actually verify the serialized MaaS request.The test name says “serialized”, but it only inspects the mocked table output. Please add one submit-path assertion on
mockInstallLSDfor a MaaS selection so the request shape is locked down too, especially the identifier andis_maas_model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationModal.spec.tsx` around lines 231 - 250, The test claims to verify serialized MaaS requests but only inspects the UI JSON; update the 'MaaS models are properly serialized in selected models' test to actually trigger the submit path and assert mockInstallLSD was called with the correct request shape: after rendering with createAIModel(..., isMaaSModel: true, maasModelId: 'llama-2-7b-chat') and selecting/confirming the modal via renderModal, simulate the submit (e.g., click the modal confirm/submit control used elsewhere in this suite) and add an assertion that mockInstallLSD was called once with a payload containing the model identifier 'llama-2-7b-chat' and is_maas_model: true (and any expected surrounding structure), so the install request shape is locked down.packages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useAIModelsFilter.spec.ts (1)
33-38: Add coverage for the newSTATUSandMODEL_TYPEfilters.This suite now initializes those keys, but it never exercises their filtering logic or clears them after being set. Since both are part of the merged-model UX, regressions in either branch would currently go unnoticed.
Also applies to: 103-119, 223-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useAIModelsFilter.spec.ts` around lines 33 - 38, The tests in useAIModelsFilter.spec.ts initialize STATUS and MODEL_TYPE keys but never exercise their behavior; add unit test cases that set AssetsFilterOptions.STATUS and AssetsFilterOptions.MODEL_TYPE via the hook (useAIModelsFilter) to verify filtering logic updates filterData and the returned filtered list, then clear each filter and assert filterData values return to undefined; specifically extend existing test blocks around the initial assertions (near the current expect of filterData) and the suites around lines referenced (the blocks around 103-119 and 223-228) to (1) dispatch filter changes for STATUS and MODEL_TYPE, (2) assert result.current.filterData reflects the set values, (3) assert the filtered results reflect those constraints, and (4) reset the filters and assert they become undefined again.packages/gen-ai/frontend/src/app/AIAssets/__tests__/AIAssetsModelsTab.spec.tsx (1)
113-139: Cover the partial-failure path with surviving models.The behavior change in this PR is “warn, but keep rendering what we have” when one source fails. Both warning tests use
models: [], so a regression that drops the table wheneveraiErrorormaasErroris set would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/__tests__/AIAssetsModelsTab.spec.tsx` around lines 113 - 139, The tests for partial failure currently set models: [] so they don't verify the component still renders surviving models; update the two tests that mock useMergedModels (mockUseMergedModels) for AIAssetsModelsTab to return a non-empty models array (e.g., include a stub model object) while keeping aiError or maasError set, then after render(<AIAssetsModelsTab />, { wrapper: TestWrapper }) assert that the warning texts still appear and also assert that the model table or the stub model's name is rendered (e.g., query by role="table" or the stub model's display name) to confirm the component continues rendering available models.packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx (1)
32-43: UsemodelSourceas the source of truth for MaaS detection.The merged-model flow introduces
modelSource, but this modal still gates token UI and analytics onisMaaSModelonly. A row normalized withmodelSource: 'maas'and no legacy flag will render like a regular model here.Also applies to: 132-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx` around lines 32 - 43, The modal currently detects MaaS via the legacy isMaaSModel flag causing rows where modelSource === 'maas' to be treated as regular models; update the MaaS detection to consider modelSource ('maas') in addition to isMaaSModel (e.g., derive a boolean like isMaaS = model.modelSource === 'maas' || !!model.isMaaSModel) and use that everywhere (hasExternal/hasInternal logic, token UI hooks like useGenerateMaaSToken usage, generateToken/tokenData rendering between lines ~132-215, and analytics payloads in copyToClipboardWithTracking/handleEndpointCopy where assetType is set) so both legacy and merged-model flows behave correctly. Ensure all references to model.isMaaSModel are replaced/augmented with the new isMaaS derived variable and that assetType uses 'maas_model' when modelSource === 'maas' as well.packages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.ts (1)
35-39: Potential duplicate namespace in mock data.The mock namespace with
name: namespaceis prepended tomockNamespaces().data, which might already contain a namespace with the same name, leading to duplicates in tests.Consider filtering out duplicates:
🛡️ Proposed fix
const namespacesData = [ mockNamespace({ name: namespace, display_name: namespace }), - ...mockNamespaces().data, + ...mockNamespaces().data.filter((ns) => ns.name !== namespace), ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.ts` around lines 35 - 39, The test builds namespacesData by prepending mockNamespace({ name: namespace, display_name: namespace }) to mockNamespaces().data which can create duplicate entries; update the construction of namespacesData (the variable used in cy.interceptGenAi('GET /api/v1/namespaces', ...)) to deduplicate by namespace name (e.g., merge the arrays and filter by unique name) so mockNamespace wins or is deduped, ensuring no duplicate namespace objects are passed to cy.interceptGenAi; locate usages of namespacesData, mockNamespace, mockNamespaces, and cy.interceptGenAi to implement the filter.packages/gen-ai/frontend/src/app/AIAssets/hooks/useAIModelsFilter.ts (1)
32-37: Type assertion onprev[filterType]may be fragile.The cast
prev[filterType]to an array assumes it's always an array for select filters, but the type isstring | string[] | undefined. If somehow a non-array value exists,current.includes(value)would fail at runtime.Consider adding explicit type narrowing:
🛡️ Proposed defensive fix
if (isSelectFilter(filterType) && typeof value === 'string') { - const current = Array.isArray(prev[filterType]) ? prev[filterType] : []; + const prevValue = prev[filterType]; + const current: string[] = Array.isArray(prevValue) ? prevValue : []; const updated = current.includes(value) ? current.filter((v) => v !== value) : [...current, value]; return { ...prev, [filterType]: updated.length > 0 ? updated : undefined }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/hooks/useAIModelsFilter.ts` around lines 32 - 37, The code in useAIModelsFilter assumes prev[filterType] is an array for select filters which can be a string, array, or undefined; fix by explicitly narrowing/coercing the value: in the branch where isSelectFilter(filterType) and typeof value === 'string', compute current with a safe check such as const current = Array.isArray(prev[filterType]) ? prev[filterType] : (typeof prev[filterType] === 'string' ? [prev[filterType]] : []); then derive updated as before and return { ...prev, [filterType]: updated.length > 0 ? updated : undefined } so includes() never runs on a non-array. Ensure you reference this change inside the useAIModelsFilter handler where filterType and prev are used.packages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsx (1)
26-34: Consider showing both error messages when both sources fail.Currently only
aiError.messageis displayed when both errors exist. The MaaS error details are lost, which could hinder debugging.💡 Proposed enhancement
if (aiError && maasError) { return ( <Bullseye> <Alert variant="danger" isInline isPlain title="Failed to load models"> - {aiError.message} + <ul> + <li>AI Models: {aiError.message}</li> + <li>MaaS Models: {maasError.message}</li> + </ul> </Alert> </Bullseye> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsx` around lines 26 - 34, AIAssetsModelsTab currently only displays aiError.message when both aiError and maasError are truthy, losing MaaS error details; update the error rendering in the conditional that checks aiError && maasError to include both error messages (e.g., combine aiError.message and maasError.message or render them as separate lines) inside the Alert component so both failures are visible; modify the JSX within the Bullseye/Alert block in AIAssetsModelsTab to show both aiError and maasError messages (ensure Alert title/aria attributes remain appropriate).packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx (1)
114-127: Asymmetric disabling for embedding models - verify intent.
- Line 114: "Try in playground" is disabled when
model.model_type === 'embedding'- Line 123: "Add to playground" is NOT disabled for embedding models
This allows users to add embedding models to the playground configuration but not use them. If this is intentional (e.g., embedding models can be configured but not chatted with), consider adding a tooltip explaining why "Try in playground" is disabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx` around lines 114 - 127, The two buttons use inconsistent disable logic: "Try in playground" is disabled when model.model_type === 'embedding' but "Add to playground" still opens the configuration modal; decide which behaviour you want and implement it consistently: if embedding models should not be usable at all, apply the same disable condition to the Add button (update its isDisabled to model.status !== 'Running' || model.model_type === 'embedding' and prevent calling setIsConfigurationModalOpen when disabled); if embedding models may be configured but not tried, keep the Add button enabled but add an explanatory tooltip (or aria-describedby) on the disabled "Try in playground" button explaining why it's unavailable. Use the existing identifiers model.status, model.model_type, the Button elements and setIsConfigurationModalOpen when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/gen-ai/bff/internal/repositories/external_models.go`:
- Around line 49-55: The endpoint formatting currently uses "external: <url>" /
"internal: <url>" in repositories (see variable endpoint and logic around
helper.IsClusterLocalURL and
models.ModelSourceTypeExternalProvider/ModelSourceTypeExternalCluster); update
codebase and tests to standardize on the space-delimited format ("external: %s"
/ "internal: %s") and make the endpoint parser tolerant to optional whitespace
by trimming or using a regex that allows an optional space after the prefix so
both "external:<url>" and "external: <url>" parse correctly; apply this change
to the parser used by token_k8s_client.go and any endpoint-parsing utilities and
update the Cypress fixtures (modelsTab.cy.ts) to the space format to keep tests
consistent.
In `@packages/gen-ai/bff/openapi/src/gen-ai.yaml`:
- Around line 2007-2010: The example value for model_display_name contains a
typo; update the YAML entry for the model_display_name property to change the
example from 'Gemini 2.5 Flash Liter' to 'Gemini 2.5 Flash Lite' so generated
docs and examples use the correct display name.
In `@packages/gen-ai/bff/README.md`:
- Around line 323-331: Update the example external model route URLs in the
README by marking them explicitly as placeholders or by templating them (e.g.,
replace the concrete "https://llama-2-7b-chat.apps.example.openshift.com/v1" and
"https://mistral-7b-instruct.apps.example.openshift.com/v1" url values with a
template like "<model>.apps.<cluster-domain>/v1" and add a short clarifying note
that these are illustrative sample external routes for OpenShift; apply the same
change to the other examples referenced around lines 1080-1104 so all sample
MaaS route URLs are clearly labelled as placeholders.
In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx`:
- Around line 129-144: The ActionsColumn "Remove asset" item currently only
calls fireMiscTrackingEvent and lacks deletion behavior; update the onClick to
perform the actual removal by invoking the app's deletion flow (e.g., call a
passed-in handler prop like onRemoveAsset or dispatch the delete API function),
include a user confirmation step (confirm modal or prompt), perform the API call
using model.model_id and assetType, update local state/UI optimistically or
refetch list on success, and log success/error (still calling
fireMiscTrackingEvent for telemetry). Ensure the component (AIModelTableRow)
accepts and uses a removal callback prop (e.g., onRemoveAsset) if not present,
and handle errors with user feedback and proper cleanup.
In
`@packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx`:
- Around line 53-130: The modal claims it includes authentication details but
never renders model.sa_token, so users (namespace/external_cluster) only get
URLs; update EndpointDetailModal to show the auth token when present: add a
dedicated ClipboardCopy (or include in existing endpoint blocks) that displays
model.sa_token (or a masked version with reveal) and uses data-testid like
"endpoint-modal-auth-token", and call handleEndpointCopy(model.sa_token ?? '',
'auth') so copying works; also update the intro Content text in the ModalBody to
conditionally mention that authentication details are included only when a token
exists (or remove the absolute claim) to keep copy accurate.
In `@packages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsx`:
- Around line 98-108: The lint warning is caused by the expression-bodied inner
arrow returning the result of filters.push in the value.forEach callback inside
the React.useMemo that defines activeFilters; change the inner callback to a
block-bodied function (or replace the inner .forEach with a for...of loop) so
the callback does not implicitly return the push value — update the
value.forEach((v) => filters.push(...)) usage in the activeFilters computation
that reads filterData to use a block like (v) => { filters.push({ filterType,
value: v }); } or an equivalent loop.
In `@packages/gen-ai/frontend/src/app/AIAssets/data/columns.tsx`:
- Around line 27-34: The "Public route" help text in the Content block
(component={ContentVariants.dd}) is too specific to Model Registry; update the
string to describe cluster-local or cross-namespace exposure instead of implying
registration via Model Registry — e.g., mention that the route is accessible
across namespaces or is a cluster-local external endpoint rather than stating
"Registered via Model Registry and accessible across namespaces within the
cluster." Locate the Content/Label pair for "Public route" and replace the
help-copy accordingly so it covers both registry-registered models and manually
configured cluster-local external endpoints.
In
`@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationTableRow.spec.tsx`:
- Around line 144-151: Update the test so it uses an explicit normalized
modelSource value instead of relying on undefined fallback: replace the mock
model construction in ChatbotConfigurationTableRow.spec.tsx by calling
createMockAIModel({ modelSource: 'namespace' }) (the test that renders
<ChatbotConfigurationTableRow {...defaultProps} />) to ensure the case for
internal/namespace-sourced models is asserted and aligns with how
useFetchAIModels normalizes modelSource.
In `@packages/gen-ai/frontend/src/app/hooks/useMergedModels.ts`:
- Around line 21-23: The merged models array created in useMergedModels (the
React.useMemo that builds `models` from `aiModels` and
`maasModels.map(convertMaaSModelToAIModel)`) must be deduplicated before
returning to avoid duplicate rows/keys; change the memo to collapse entries by a
stable identity (e.g., model.id, falling back to model.name) by building a Map
or object keyed by that identity and only keeping the first occurrence, then
return the Map.values() as the memoized array so duplicates from aiModels and
MaaS are removed.
---
Outside diff comments:
In `@packages/gen-ai/bff/internal/repositories/external_models.go`:
- Around line 57-71: In the AAModel return in external_models.go, populate the
newly added ModelType field so the create response matches the unified models
list; set models.AAModel.ModelType to the value from the request (e.g.,
req.ModelType) or a sensible fallback (empty string) if absent, keeping
ModelSourceType as sourceType to preserve the current source mapping.
In `@packages/gen-ai/bff/openapi/src/gen-ai.yaml`:
- Around line 552-566: The OpenAPI response schemas declare the model_type
property but don't mark it required, causing generated clients to treat it as
optional; update each response schema that defines model_type (the schemas near
the blocks where model_type is declared and the two other occurrences referenced
in the review) by adding "model_type" to their required arrays so the property
is treated as mandatory in generated types and clients (look for the schema
objects that contain the model_type property and add "model_type" to their
required list alongside id/object/created/owned_by/ready/url).
In
`@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationModal.tsx`:
- Around line 56-65: The merged models array currently concatenates aiModels and
maasAsAIModels into allModels and can produce duplicates; modify the logic
around allModels (and keep maasAsAIModels/convertMaaSModelToAIModel references)
to deduplicate entries defensively — e.g., build allModels from [...aiModels,
...maasAsAIModels] but filter by a unique key (model id or name) to remove
duplicates already present in aiModels (or consult
useFetchAIModels/useFetchMaaSModels invariants if you prefer to rely on upstream
guarantees); ensure the dedupe uses the same identity field used elsewhere
(matching useMergedModels behavior) so the UI shows each model once.
---
Nitpick comments:
In `@packages/gen-ai/bff/internal/models/maas_models.go`:
- Around line 17-21: The comment for MaaSModel.DeriveModelType is misleading
because it says "Returns 'embedding'..." even though the method mutates the
receiver and returns nothing; update the docstring for DeriveModelType to
describe the mutation (e.g., "Sets ModelType to 'embedding' if Usecase contains
'embedding' (case-insensitive), otherwise sets it to 'llm'") and reference that
it uses DeriveModelTypeFromUsecase to compute the value so readers understand
behavior.
In
`@packages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.ts`:
- Around line 35-39: The test builds namespacesData by prepending
mockNamespace({ name: namespace, display_name: namespace }) to
mockNamespaces().data which can create duplicate entries; update the
construction of namespacesData (the variable used in cy.interceptGenAi('GET
/api/v1/namespaces', ...)) to deduplicate by namespace name (e.g., merge the
arrays and filter by unique name) so mockNamespace wins or is deduped, ensuring
no duplicate namespace objects are passed to cy.interceptGenAi; locate usages of
namespacesData, mockNamespace, mockNamespaces, and cy.interceptGenAi to
implement the filter.
In
`@packages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/aiAssets/modelsTab.cy.ts`:
- Around line 89-95: The test verifies the embedding model row but misses
asserting the playground restriction; update the block using the existing
embeddingRow variable to assert the playground is disabled/unavailable (e.g.,
call embeddingRow.findPlaygroundCell().should('contain.text', 'Unavailable') or
embeddingRow.findPlaygroundButton().should('be.disabled') depending on available
helpers). Ensure you use the same row helper (embeddingRow) and add the
playground assertion after the existing status check so the embedding-model
playground restriction is covered.
In
`@packages/gen-ai/frontend/src/app/AIAssets/__tests__/AIAssetsModelsTab.spec.tsx`:
- Around line 113-139: The tests for partial failure currently set models: [] so
they don't verify the component still renders surviving models; update the two
tests that mock useMergedModels (mockUseMergedModels) for AIAssetsModelsTab to
return a non-empty models array (e.g., include a stub model object) while
keeping aiError or maasError set, then after render(<AIAssetsModelsTab />, {
wrapper: TestWrapper }) assert that the warning texts still appear and also
assert that the model table or the stub model's name is rendered (e.g., query by
role="table" or the stub model's display name) to confirm the component
continues rendering available models.
In `@packages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsx`:
- Around line 26-34: AIAssetsModelsTab currently only displays aiError.message
when both aiError and maasError are truthy, losing MaaS error details; update
the error rendering in the conditional that checks aiError && maasError to
include both error messages (e.g., combine aiError.message and maasError.message
or render them as separate lines) inside the Alert component so both failures
are visible; modify the JSX within the Bullseye/Alert block in AIAssetsModelsTab
to show both aiError and maasError messages (ensure Alert title/aria attributes
remain appropriate).
In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx`:
- Around line 114-127: The two buttons use inconsistent disable logic: "Try in
playground" is disabled when model.model_type === 'embedding' but "Add to
playground" still opens the configuration modal; decide which behaviour you want
and implement it consistently: if embedding models should not be usable at all,
apply the same disable condition to the Add button (update its isDisabled to
model.status !== 'Running' || model.model_type === 'embedding' and prevent
calling setIsConfigurationModalOpen when disabled); if embedding models may be
configured but not tried, keep the Add button enabled but add an explanatory
tooltip (or aria-describedby) on the disabled "Try in playground" button
explaining why it's unavailable. Use the existing identifiers model.status,
model.model_type, the Button elements and setIsConfigurationModalOpen when
making the change.
In
`@packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx`:
- Around line 32-43: The modal currently detects MaaS via the legacy isMaaSModel
flag causing rows where modelSource === 'maas' to be treated as regular models;
update the MaaS detection to consider modelSource ('maas') in addition to
isMaaSModel (e.g., derive a boolean like isMaaS = model.modelSource === 'maas'
|| !!model.isMaaSModel) and use that everywhere (hasExternal/hasInternal logic,
token UI hooks like useGenerateMaaSToken usage, generateToken/tokenData
rendering between lines ~132-215, and analytics payloads in
copyToClipboardWithTracking/handleEndpointCopy where assetType is set) so both
legacy and merged-model flows behave correctly. Ensure all references to
model.isMaaSModel are replaced/augmented with the new isMaaS derived variable
and that assetType uses 'maas_model' when modelSource === 'maas' as well.
In `@packages/gen-ai/frontend/src/app/AIAssets/data/filterOptions.ts`:
- Around line 25-29: The map's type is too broad (Record<string, string[]>) and
allows indexing with AssetsFilterOptions.NAME or USE_CASE without a type error;
tighten the type of assetsFilterSelectOptions to only the keys that actually
have select values by replacing Record<string, string[]> with a keyed Record
using the specific enum members (e.g. Record<AssetsFilterOptions.SOURCE |
AssetsFilterOptions.STATUS | AssetsFilterOptions.MODEL_TYPE, string[]>), so
assetsFilterSelectOptions and any consumers referencing AssetsFilterOptions.NAME
or USE_CASE will get a compile-time error instead of possible undefined at
runtime.
In
`@packages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useAIModelsFilter.spec.ts`:
- Around line 33-38: The tests in useAIModelsFilter.spec.ts initialize STATUS
and MODEL_TYPE keys but never exercise their behavior; add unit test cases that
set AssetsFilterOptions.STATUS and AssetsFilterOptions.MODEL_TYPE via the hook
(useAIModelsFilter) to verify filtering logic updates filterData and the
returned filtered list, then clear each filter and assert filterData values
return to undefined; specifically extend existing test blocks around the initial
assertions (near the current expect of filterData) and the suites around lines
referenced (the blocks around 103-119 and 223-228) to (1) dispatch filter
changes for STATUS and MODEL_TYPE, (2) assert result.current.filterData reflects
the set values, (3) assert the filtered results reflect those constraints, and
(4) reset the filters and assert they become undefined again.
In `@packages/gen-ai/frontend/src/app/AIAssets/hooks/useAIModelsFilter.ts`:
- Around line 32-37: The code in useAIModelsFilter assumes prev[filterType] is
an array for select filters which can be a string, array, or undefined; fix by
explicitly narrowing/coercing the value: in the branch where
isSelectFilter(filterType) and typeof value === 'string', compute current with a
safe check such as const current = Array.isArray(prev[filterType]) ?
prev[filterType] : (typeof prev[filterType] === 'string' ? [prev[filterType]] :
[]); then derive updated as before and return { ...prev, [filterType]:
updated.length > 0 ? updated : undefined } so includes() never runs on a
non-array. Ensure you reference this change inside the useAIModelsFilter handler
where filterType and prev are used.
In
`@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationModal.spec.tsx`:
- Around line 231-250: The test claims to verify serialized MaaS requests but
only inspects the UI JSON; update the 'MaaS models are properly serialized in
selected models' test to actually trigger the submit path and assert
mockInstallLSD was called with the correct request shape: after rendering with
createAIModel(..., isMaaSModel: true, maasModelId: 'llama-2-7b-chat') and
selecting/confirming the modal via renderModal, simulate the submit (e.g., click
the modal confirm/submit control used elsewhere in this suite) and add an
assertion that mockInstallLSD was called once with a payload containing the
model identifier 'llama-2-7b-chat' and is_maas_model: true (and any expected
surrounding structure), so the install request shape is locked down.
In
`@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationTableRow.tsx`:
- Around line 131-141: In ChatbotConfigurationTableRow, avoid calling
getSourceLabel(model) multiple times in the render block: compute const
sourceLabel = getSourceLabel(model) once above the JSX and replace the three
calls (the conditional check, the color lookup getSourceLabelColor(...), and the
rendered label text) with sourceLabel and getSourceLabelColor(sourceLabel) to
improve readability and avoid redundant computation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95f2cedf-e766-4c2d-975c-49112ee19620
📒 Files selected for processing (66)
packages/gen-ai/bff/README.mdpackages/gen-ai/bff/internal/api/external_models_handler_test.gopackages/gen-ai/bff/internal/api/maas_middleware_test.gopackages/gen-ai/bff/internal/api/maas_models_handler_test.gopackages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client.gopackages/gen-ai/bff/internal/integrations/maas/maasmocks/maas_client_mock.gopackages/gen-ai/bff/internal/models/aaa_models.gopackages/gen-ai/bff/internal/models/maas_models.gopackages/gen-ai/bff/internal/repositories/external_models.gopackages/gen-ai/bff/internal/repositories/maas_models.gopackages/gen-ai/bff/openapi/src/gen-ai.yamlpackages/gen-ai/frontend/config/moduleFederation.jspackages/gen-ai/frontend/src/__tests__/cypress/cypress/__mocks__/index.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/__mocks__/mockMaaSModels.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/pages/aiAssetsPage.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/pages/modelsTabPage.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/aiAssets/modelsTab.cy.tspackages/gen-ai/frontend/src/app/AIAssets/AIAssetsMaaSTab.tsxpackages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsxpackages/gen-ai/frontend/src/app/AIAssets/AIAssetsPage.tsxpackages/gen-ai/frontend/src/app/AIAssets/__tests__/AIAssetsMaaSTab.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/__tests__/AIAssetsModelsTab.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTable.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTableRowEndpoint.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRow.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRowEndpoint.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTable.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTableRowInfo.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/TierInfoPopover.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelTableRow.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelsTable.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelsTableRowEndpoint.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelsTableRowEndpoint.tracking.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/EndpointDetailModal.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelTableRow.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelTableRowEndpoint.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelTableRowEndpoint.tracking.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelsTable.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelsTableRowInfo.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/ModelsListToolbar.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/TierInfoPopover.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/data/columns.tspackages/gen-ai/frontend/src/app/AIAssets/data/columns.tsxpackages/gen-ai/frontend/src/app/AIAssets/data/filterOptions.tspackages/gen-ai/frontend/src/app/AIAssets/data/maasColumns.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useAIModelsFilter.spec.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useMaaSModelsFilter.spec.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/useAIModelsFilter.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/useMaaSModelsFilter.tspackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationModal.tsxpackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationTableRow.tsxpackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationModal.spec.tsxpackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationTableRow.spec.tsxpackages/gen-ai/frontend/src/app/hooks/__tests__/useMergedModels.spec.tspackages/gen-ai/frontend/src/app/hooks/useFetchAIModels.tspackages/gen-ai/frontend/src/app/hooks/useMergedModels.tspackages/gen-ai/frontend/src/app/services/__tests__/llamaStackService.fixtures.tspackages/gen-ai/frontend/src/app/types.tspackages/gen-ai/frontend/src/app/utilities/__tests__/utils.spec.tspackages/gen-ai/frontend/src/app/utilities/routes.tspackages/gen-ai/frontend/src/app/utilities/utils.tspackages/gen-ai/frontend/src/odh/extension-points/maas.ts
💤 Files with no reviewable changes (21)
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelsTable.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTable.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/TierInfoPopover.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTableRowInfo.tsx
- packages/gen-ai/frontend/src/app/AIAssets/hooks/tests/useMaaSModelsFilter.spec.ts
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/TierInfoPopover.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/data/columns.ts
- packages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTableRowEndpoint.tsx
- packages/gen-ai/frontend/src/app/AIAssets/AIAssetsMaaSTab.tsx
- packages/gen-ai/frontend/src/app/AIAssets/hooks/useMaaSModelsFilter.ts
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelsTableRowInfo.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/AIModelsTableRowEndpoint.spec.tsx
- packages/gen-ai/frontend/config/moduleFederation.js
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelTableRowEndpoint.tracking.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRow.tsx
- packages/gen-ai/frontend/src/app/AIAssets/data/maasColumns.ts
- packages/gen-ai/frontend/src/app/AIAssets/tests/AIAssetsMaaSTab.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRowEndpoint.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/AIModelsTableRowEndpoint.tracking.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelTableRowEndpoint.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelTableRow.spec.tsx
packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx
Show resolved
Hide resolved
packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx
Show resolved
Hide resolved
packages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsx
Show resolved
Hide resolved
.../app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationTableRow.spec.tsx
Show resolved
Hide resolved
NickGagan
reviewed
Mar 9, 2026
Contributor
NickGagan
left a comment
NickGagan
left a comment
There was a problem hiding this comment.
Conceptually the changes made sense to me, but to do a PR review on the code we'll need it broken up into smaller chunks.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6628 +/- ##
==========================================
+ Coverage 64.02% 64.12% +0.10%
==========================================
Files 2499 2496 -3
Lines 75200 75316 +116
Branches 18956 19000 +44
==========================================
+ Hits 48149 48299 +150
+ Misses 27051 27017 -34 see 34 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/gen-ai/bff/internal/repositories/external_models.go (1)
58-72:⚠️ Potential issue | 🟠 MajorReturn the requested
model_typein the create response.Line 71 re-derives
ModelTypefromreq.UseCases, but the model we persist usesreq.ModelType. If those ever diverge, the POST response returns a different type than the model that will be listed on the next refresh.💡 Suggested fix
- ModelType: models.DeriveModelTypeFromUsecase(req.UseCases), + ModelType: req.ModelType,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/bff/internal/repositories/external_models.go` around lines 58 - 72, The POST response returns a ModelType derived from req.UseCases instead of the actual persisted value req.ModelType, causing inconsistency; update the creation return of models.AAModel (constructed in the function that builds the create response) to set ModelType to req.ModelType rather than models.DeriveModelTypeFromUsecase(req.UseCases) so the response matches the stored model.packages/gen-ai/bff/openapi/src/gen-ai.yaml (1)
3986-3987:⚠️ Potential issue | 🟡 MinorSync the response example with the new endpoint format.
Lines 3986-3987 still show a raw URL, but
AAModel.endpointsnow usesexternal: .../internal: .... The generated docs will otherwise advertise the old payload shape.💡 Suggested fix
endpoints: - - 'https://generativelanguage.googleapis.com/v1beta/openai/' + - 'external: https://generativelanguage.googleapis.com/v1beta/openai/'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/bff/openapi/src/gen-ai.yaml` around lines 3986 - 3987, Update the example response in the OpenAPI file so it matches the new AAModel.endpoints shape (use an object with external and internal keys) instead of the old raw endpoints URL string; find the example payload that references "endpoints" and replace the string/array value with the new structure (e.g., endpoints: { external: "<external-url>", internal: "<internal-url>" }) so generated docs reflect AAModel.endpoints, and ensure any references to the old plain URL in the response example are removed or replaced accordingly.
♻️ Duplicate comments (3)
packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx (1)
131-146:⚠️ Potential issue | 🟡 Minor"Remove asset" action still only fires tracking event without actual removal logic.
This was flagged in a previous review and remains unresolved. The
onClickhandler only callsfireMiscTrackingEventbut does not perform any actual asset removal. If this is intentional placeholder behavior for a future implementation, consider:
- Adding a TODO comment explaining the planned implementation
- Showing a "not yet implemented" toast/notification to users who click it
- Or removing the action entirely until the backend support is ready
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx` around lines 131 - 146, The "Remove asset" ActionsColumn onClick currently only calls fireMiscTrackingEvent and lacks removal behavior; update AIModelTableRow so the onClick either invokes the real removal flow (call the existing remove handler or API delete function for the asset using model.model_id and assetType) or, if backend support isn't ready, add a clear TODO comment on the onClick and show a user-facing "Not yet implemented" toast/notification in addition to the fireMiscTrackingEvent; ensure you reference the same symbols (ActionsColumn, onClick, fireMiscTrackingEvent, model.model_id) so caller code is easy to replace with the real delete call later.packages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsx (1)
98-103:⚠️ Potential issue | 🟡 MinorFix the implicit return inside the inner
forEach.Line 102 still implicitly returns the result of
filters.push(...), so Biome will keep reportinglint/suspicious/useIterableCallbackReturn.Proposed fix
Object.entries(filterData).forEach(([filterType, value]) => { if (Array.isArray(value)) { - value.forEach((v) => filters.push({ filterType, value: v })); + value.forEach((v) => { + filters.push({ filterType, value: v }); + }); } else if (value && value !== '') { filters.push({ filterType, value }); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsx` around lines 98 - 103, The inner forEach callback in activeFilters currently implicitly returns the result of filters.push(...) which triggers the lint rule; update the callback for the array case so it does not return a value (e.g., replace the concise arrow body with a block body or use a for...of loop) when iterating value and calling filters.push, keeping the rest of activeFilters/filterData logic unchanged.packages/gen-ai/bff/openapi/src/gen-ai.yaml (1)
2008-2010:⚠️ Potential issue | 🟡 MinorFix the
model_display_nameexample typo.Line 2010 should be
Gemini 2.5 Flash Lite; the currentLitertypo will leak into generated docs and examples.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/bff/openapi/src/gen-ai.yaml` around lines 2008 - 2010, The example for the OpenAPI property model_display_name contains a typo ("Gemini 2.5 Flash Liter"); update the example value for model_display_name in gen-ai.yaml to "Gemini 2.5 Flash Lite" so generated docs and examples show the correct product name.
🧹 Nitpick comments (4)
packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx (1)
47-55: Consider extracting inline styles to a shared style constant or CSS class.The inline style object on
Truncateworks correctly, but extracting it to a reusable constant would improve maintainability if similar description styling is used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx` around lines 47 - 55, The Truncate usage passes an inline style object for the model description; extract that object into a shared constant (e.g., DESCRIPTION_STYLE) or a CSS class (e.g., .ai-model-description) and replace the inline style prop on Truncate with style={DESCRIPTION_STYLE} or className="ai-model-description" so the same styling can be reused across components; update the file's imports/exports as needed and ensure any other description usages reference the new constant/class.packages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsx (1)
23-24: Keep this filter contract anchored toAssetsFilterOptions.The component now widens
onFilterUpdate,filterOptions, andcurrentFilterTypeto rawstrings. That makes it easy for an unexpected dropdown key to compile and just create inert entries infilterData. Since this toolbar only works with the known filter enum, keep these types tied toAssetsFilterOptionsso additions/removals stay exhaustive.Also applies to: 27-29, 46-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsx` around lines 23 - 24, The toolbar widened several props to plain strings causing invalid keys to slip into filterData; update the ModelsListToolbar prop and internal types so onFilterUpdate, filterOptions, and currentFilterType use the AssetsFilterOptions enum (not string) and keep assetsFilterSelectOptions and any selection keys typed to AssetsFilterOptions; also ensure FilterData entries and the useAIModelsFilter hook signatures remain keyed by AssetsFilterOptions so additions/removals are exhaustive and the compiler catches invalid dropdown keys.packages/gen-ai/frontend/src/app/AIAssets/data/filterOptions.ts (1)
25-28: Avoid maintaining these select labels in a second place.
useAIModelsFiltermatches selected values againstgetSourceLabel()/getModelTypeLabel()outputs frompackages/gen-ai/frontend/src/app/utilities/utils.ts:125-148. If those label maps change without this list changing too, the UI will render options that never match. Consider derivingassetsFilterSelectOptionsfrom the shared label maps or exporting a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/data/filterOptions.ts` around lines 25 - 28, assetsFilterSelectOptions is a hardcoded mirror of label maps used by useAIModelsFilter, which causes mismatches if getSourceLabel() or getModelTypeLabel() change; update assetsFilterSelectOptions to derive its arrays from the shared label maps instead of hardcoding strings (e.g., build the SOURCE array from getSourceLabel() values and MODEL_TYPE from getModelTypeLabel()), keep STATUS either centralized or exported from the same shared module, and ensure consumers still use AssetsFilterOptions keys (SOURCE, MODEL_TYPE, STATUS) so matching in useAIModelsFilter remains correct.packages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useAIModelsFilter.spec.ts (1)
103-223: Add coverage for the new multi-select toggle semantics.These cases only prove single-option matches, but
useAIModelsFilter.ts:30-40now stores SOURCE/STATUS/MODEL_TYPE as arrays and toggles values in and out. A regression in OR-within-key behavior or deselection would still leave this suite green. Please add one case that selects two values under the same filter and another that selects the same value twice to clear it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useAIModelsFilter.spec.ts` around lines 103 - 223, Add two tests to useAIModelsFilter.spec.ts covering multi-select toggle semantics: (1) under the "Filter by source" or similar block, call result.current.onFilterUpdate(AssetsFilterOptions.SOURCE, 'Internal') then again with a second value (e.g., 'MaaS') and assert filteredModels contains models matching either source (OR behavior); (2) add a test that calls onFilterUpdate(AssetsFilterOptions.MODEL_TYPE, 'Inferencing') twice (toggle on then off) and assert filteredModels returns to the unfiltered set (or the expected cleared state). Use the existing helpers createMockAIModel, testHook(useAIModelsFilter), and inspect result.current.filteredModels to verify the multi-select add/remove behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client.go`:
- Around line 584-593: The goroutine is currently swallowing all errors from
kc.GetAAModelsFromExternalModels by logging a warning and returning nil, which
hides real failures; change the anonymous g.Go handler to propagate
non-NotFound/non-absent-ConfigMap errors instead of converting them to an empty
slice: call kc.GetAAModelsFromExternalModels(gCtx, identity, namespace) into
aaModelsFromExternal and if err != nil check whether the error represents
“absent ConfigMap” (the function already handles that) — if it is the expected
not-found case you may set aaModelsFromExternal = []models.AAModel{} and
continue, otherwise return err from the goroutine (or surface it separately) so
the caller receives the failure; update the logging in the g.Go block
(kc.Logger.Warn) to only run for recoverable/missing-ConfigMap cases and not for
real errors.
In `@packages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsx`:
- Around line 76-92: The current render in AIAssetsModelsTab shows the
deployment onboarding emptyState whenever models.length === 0 even if warnings
exist; update the conditional render so that when models.length === 0 and
warnings.length > 0 you render a neutral empty state (e.g., neutralEmptyState or
a modified emptyStateNeutral) instead of the deploy-onboarding emptyState.
Locate the JSX in AIAssetsModelsTab where models and warnings are used and
change the ternary to pick the neutral empty UI when warnings.length > 0 (or
adjust emptyState creation to accept a flag isSourceFailed based on warnings) so
users aren’t prompted to “deploy a model” when a source failure may be hiding
existing models.
---
Outside diff comments:
In `@packages/gen-ai/bff/internal/repositories/external_models.go`:
- Around line 58-72: The POST response returns a ModelType derived from
req.UseCases instead of the actual persisted value req.ModelType, causing
inconsistency; update the creation return of models.AAModel (constructed in the
function that builds the create response) to set ModelType to req.ModelType
rather than models.DeriveModelTypeFromUsecase(req.UseCases) so the response
matches the stored model.
In `@packages/gen-ai/bff/openapi/src/gen-ai.yaml`:
- Around line 3986-3987: Update the example response in the OpenAPI file so it
matches the new AAModel.endpoints shape (use an object with external and
internal keys) instead of the old raw endpoints URL string; find the example
payload that references "endpoints" and replace the string/array value with the
new structure (e.g., endpoints: { external: "<external-url>", internal:
"<internal-url>" }) so generated docs reflect AAModel.endpoints, and ensure any
references to the old plain URL in the response example are removed or replaced
accordingly.
---
Duplicate comments:
In `@packages/gen-ai/bff/openapi/src/gen-ai.yaml`:
- Around line 2008-2010: The example for the OpenAPI property model_display_name
contains a typo ("Gemini 2.5 Flash Liter"); update the example value for
model_display_name in gen-ai.yaml to "Gemini 2.5 Flash Lite" so generated docs
and examples show the correct product name.
In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx`:
- Around line 131-146: The "Remove asset" ActionsColumn onClick currently only
calls fireMiscTrackingEvent and lacks removal behavior; update AIModelTableRow
so the onClick either invokes the real removal flow (call the existing remove
handler or API delete function for the asset using model.model_id and assetType)
or, if backend support isn't ready, add a clear TODO comment on the onClick and
show a user-facing "Not yet implemented" toast/notification in addition to the
fireMiscTrackingEvent; ensure you reference the same symbols (ActionsColumn,
onClick, fireMiscTrackingEvent, model.model_id) so caller code is easy to
replace with the real delete call later.
In `@packages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsx`:
- Around line 98-103: The inner forEach callback in activeFilters currently
implicitly returns the result of filters.push(...) which triggers the lint rule;
update the callback for the array case so it does not return a value (e.g.,
replace the concise arrow body with a block body or use a for...of loop) when
iterating value and calling filters.push, keeping the rest of
activeFilters/filterData logic unchanged.
---
Nitpick comments:
In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx`:
- Around line 47-55: The Truncate usage passes an inline style object for the
model description; extract that object into a shared constant (e.g.,
DESCRIPTION_STYLE) or a CSS class (e.g., .ai-model-description) and replace the
inline style prop on Truncate with style={DESCRIPTION_STYLE} or
className="ai-model-description" so the same styling can be reused across
components; update the file's imports/exports as needed and ensure any other
description usages reference the new constant/class.
In `@packages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsx`:
- Around line 23-24: The toolbar widened several props to plain strings causing
invalid keys to slip into filterData; update the ModelsListToolbar prop and
internal types so onFilterUpdate, filterOptions, and currentFilterType use the
AssetsFilterOptions enum (not string) and keep assetsFilterSelectOptions and any
selection keys typed to AssetsFilterOptions; also ensure FilterData entries and
the useAIModelsFilter hook signatures remain keyed by AssetsFilterOptions so
additions/removals are exhaustive and the compiler catches invalid dropdown
keys.
In `@packages/gen-ai/frontend/src/app/AIAssets/data/filterOptions.ts`:
- Around line 25-28: assetsFilterSelectOptions is a hardcoded mirror of label
maps used by useAIModelsFilter, which causes mismatches if getSourceLabel() or
getModelTypeLabel() change; update assetsFilterSelectOptions to derive its
arrays from the shared label maps instead of hardcoding strings (e.g., build the
SOURCE array from getSourceLabel() values and MODEL_TYPE from
getModelTypeLabel()), keep STATUS either centralized or exported from the same
shared module, and ensure consumers still use AssetsFilterOptions keys (SOURCE,
MODEL_TYPE, STATUS) so matching in useAIModelsFilter remains correct.
In
`@packages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useAIModelsFilter.spec.ts`:
- Around line 103-223: Add two tests to useAIModelsFilter.spec.ts covering
multi-select toggle semantics: (1) under the "Filter by source" or similar
block, call result.current.onFilterUpdate(AssetsFilterOptions.SOURCE,
'Internal') then again with a second value (e.g., 'MaaS') and assert
filteredModels contains models matching either source (OR behavior); (2) add a
test that calls onFilterUpdate(AssetsFilterOptions.MODEL_TYPE, 'Inferencing')
twice (toggle on then off) and assert filteredModels returns to the unfiltered
set (or the expected cleared state). Use the existing helpers createMockAIModel,
testHook(useAIModelsFilter), and inspect result.current.filteredModels to verify
the multi-select add/remove behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c8488d0-8379-409c-988e-e0c617789fec
📒 Files selected for processing (13)
packages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client.gopackages/gen-ai/bff/internal/models/maas_models.gopackages/gen-ai/bff/internal/repositories/external_models.gopackages/gen-ai/bff/openapi/src/gen-ai.yamlpackages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.tspackages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsxpackages/gen-ai/frontend/src/app/AIAssets/__tests__/AIAssetsModelsTab.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsxpackages/gen-ai/frontend/src/app/AIAssets/data/filterOptions.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useAIModelsFilter.spec.tspackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationTableRow.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/gen-ai/bff/internal/models/maas_models.go
- packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx
- packages/gen-ai/frontend/src/tests/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.ts
- packages/gen-ai/frontend/src/app/AIAssets/tests/AIAssetsModelsTab.spec.tsx
packages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client.go
Show resolved
Hide resolved
Lucifergene
force-pushed
the
unify-models-tab
branch
from
93ddee3 to
965dd5c
Compare
Contributor
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 (2)
packages/gen-ai/frontend/src/app/AIAssets/AIAssetsPage.tsx (1)
89-96:⚠️ Potential issue | 🔴 CriticalThis drops the
maasmodelstab content.
packages/maas/frontend/src/odh/genAiExtensions/genAiExtensions.ts:32-42still registersGenAiMaaSTabWrapperfor the MaaS AI Assets tab, andpackages/maas/frontend/src/odh/genAiExtensions/GenAiMaaSTabWrapper.tsx:1-12still renders onlychildren.packages/gen-ai/frontend/src/odh/extension-points/ai-assets.ts:4-12also still types these tab extensions asComponentType<{ children?: React.ReactNode }>. With this render path no longer passing any children, selecting that tab now produces an empty body. Please either keep injecting the tab content here or move the MaaS content ownership into the extension component itself.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/AIAssetsPage.tsx` around lines 89 - 96, The MaaS tab content is lost because LazyCodeRefComponent is rendered without children while GenAiMaaSTabWrapper (and the ai-assets extension typing ComponentType<{ children?: React.ReactNode }>) expects children; fix by either passing the tab body as children into the dynamically loaded component (e.g., render <LazyCodeRefComponent component={extension.properties.component} children={tabContent} fallback={...} /> or equivalently include {tabContent} between the component tags) so existing GenAiMaaSTabWrapper receives and renders it, or update the MaaS extension (GenAiMaaSTabWrapper / the extension component registered in extension.properties.component) to own and render its own content instead of expecting children—choose one approach and make the corresponding change to LazyCodeRefComponent usage or the GenAiMaaSTabWrapper implementation and the ai-assets extension typing.packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationModal.tsx (1)
56-64:⚠️ Potential issue | 🟠 MajorRestore ID-based de-duplication in this modal.
convertMaaSModelToAIModel()keeps the raw MaaSidasmodel_id, so removing the filter means an AI asset and MaaS model with the same identifier now both land inallModels. In create mode this modal preselectsallModels, so the duplicate pair is submitted toinstallLSD()immediately. The previous filter was preventing that collision.Proposed fix
const maasAsAIModels: AIModel[] = React.useMemo( - () => maasModels.map(convertMaaSModelToAIModel), - [maasModels], + () => { + const aiModelIds = new Set(aiModels.map((model) => model.model_id)); + return maasModels + .filter((model) => !aiModelIds.has(model.id)) + .map(convertMaaSModelToAIModel); + }, + [aiModels, maasModels], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationModal.tsx` around lines 56 - 64, Restore ID-based de-duplication when building allModels: ensure the merged list produced in the React.useMemo that constructs allModels deduplicates entries by the model identifier (model_id) coming from convertMaaSModelToAIModel so an AI asset and a MaaS model with the same id don't both appear; implement a filter or Map that keeps the first (or preferred) entry per model_id when merging aiModels and maasAsAIModels, so the modal’s preselection in create mode won't submit duplicate pairs to installLSD(); update the React.useMemo that defines allModels to perform this de-duplication.
♻️ Duplicate comments (4)
packages/gen-ai/frontend/src/app/AIAssets/data/columns.tsx (1)
27-34:⚠️ Potential issue | 🟡 MinorBroaden the “Public route” help text.
This source also covers cluster-local / cross-namespace endpoints that were manually configured, not just models registered via Model Registry. The current description will misclassify part of the unified list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/data/columns.tsx` around lines 27 - 34, Update the "Public route" help text in the UI component that renders Content with ContentVariants.dt/dd and the Label "Public route" (in columns.tsx) so it no longer implies exclusivity to Model Registry entries; change the description text under ContentVariants.dd to a broader phrasing that covers both Model Registry-registered endpoints and manually configured cluster-local or cross-namespace endpoints (e.g., "Endpoints accessible across namespaces within the cluster, including registry-registered and manually configured routes"). Ensure you modify only the string content rendered by the Content component and keep the existing structure using ContentVariants.dt and ContentVariants.dd.packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx (1)
53-215:⚠️ Potential issue | 🟠 MajorExpose non-MaaS auth details or update the copy.
For
namespace/external_clustermodels this modal only shows URLs, but the intro still says authentication details are included.AIModelalready carriessa_token, so users can leave this dialog with an unusable endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx` around lines 53 - 215, The modal claims "authentication details" but only shows tokens for MaaS (isMaaS); for namespace/external_cluster models that have AIModel.sa_token either display that token (add a section like the existing isMaaS token block: label it "Service account token", show ClipboardCopy for model.sa_token with copyToClipboardWithTracking and an appropriate testid and aria-label) or if no sa_token is available change the intro Content (the paragraph under ModalBody) to remove "along with the authentication details" so it only mentions the endpoint URL; update logic around hasExternal/hasInternal and isMaaS to surface model.sa_token where present and reuse existing UI patterns (Alert info, ClipboardCopy, copy tracking) so users can actually authenticate.packages/gen-ai/bff/openapi/src/gen-ai.yaml (1)
2008-2011:⚠️ Potential issue | 🟡 MinorFix the example typo.
Gemini 2.5 Flash Literis still misspelled, so generated docs/examples will keep publishing the wrong display name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/bff/openapi/src/gen-ai.yaml` around lines 2008 - 2011, Update the example value for the model_display_name field to fix the typo: replace the current example 'Gemini 2.5 Flash Liter' with the correct display name (e.g., 'Gemini 2.5 Flash Lighter') in the OpenAPI schema so generated docs/examples use the corrected model_display_name.packages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsx (1)
78-90:⚠️ Potential issue | 🟠 MajorStill renders the onboarding empty state after a partial source failure.
When one source fails and the other returns zero models, this still tells the user “To begin you must deploy a model,” even though the failed source may contain models that were not loaded. Render a neutral failure empty state when
warnings.length > 0 && models.length === 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsx` around lines 78 - 90, The component AIAssetsModelsTab currently renders emptyState whenever models.length === 0 even if there are warnings; change the conditional so that when warnings.length > 0 && models.length === 0 you render a neutral failure empty state (create or reuse a fallback like failureEmptyState) instead of the onboarding emptyState. Locate the render block that uses warnings, Alert, models and emptyState and update the ternary (models.length === 0 ? emptyState : ...) to first check for warnings (if warnings.length > 0 && models.length === 0 return the neutral failureEmptyState; else if models.length === 0 return emptyState; otherwise render the models list).
🧹 Nitpick comments (2)
packages/gen-ai/frontend/src/app/hooks/__tests__/useMergedModels.spec.ts (1)
21-37: Honormodel_idoverrides in the fixture helper.
createAIModelcurrently derivesmodel_idfromoverrides.model_name, so callers that override onlymodel_idget the wrong fixture. That can hide merge or dedup regressions in later tests.Suggested fix
const createAIModel = (overrides: Partial<AIModel>): AIModel => ({ model_name: 'model-name', - model_id: overrides.model_name || 'model-name', + model_id: overrides.model_id ?? overrides.model_name ?? 'model-name', display_name: 'Display Name', description: 'desc',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/hooks/__tests__/useMergedModels.spec.ts` around lines 21 - 37, The fixture helper createAIModel sets model_id incorrectly from overrides.model_name which ignores callers that provide overrides.model_id; update createAIModel so model_id is assigned from overrides.model_id if present, otherwise fall back to overrides.model_name, and finally to the default 'model-name' (i.e., compute model_id from overrides.model_id ?? overrides.model_name ?? 'model-name') while still spreading ...overrides at the end so other override fields apply.packages/gen-ai/bff/internal/api/maas_models_handler_test.go (1)
57-64: Avoid binding this test to response order.These assertions depend on the target model staying at
response.Data[0], but the contract here is the returned content, not the slice position. Find the model byIDbefore assertingURLandModelTypeso this does not fail on harmless reordering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/bff/internal/api/maas_models_handler_test.go` around lines 57 - 64, The test currently assumes the target model is at response.Data[0]; update the assertion to first locate the model with ID "llama-2-7b-chat" in response.Data (e.g., loop or helper to find by model.ID) and assign that to firstModel, then assert firstModel.URL, firstModel.ModelType and other fields; also add an assertion that the model was found to avoid nil/index errors. Use the existing symbols response.Data, model.ID, firstModel, URL and ModelType to locate and validate the correct item instead of relying on slice order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/gen-ai/bff/internal/models/aaa_models.go`:
- Line 39: The struct field ModelType in aaa_models.go currently uses the tag
`json:"model_type,omitempty"`, which suppresses the field in JSON and violates
the API schema; remove `omitempty` so the tag becomes `json:"model_type"` on the
ModelType ModelTypeEnum `json:"model_type"` field; also scan
constructors or builders that create this struct (functions referencing the
ModelType field) to ensure they set a valid ModelType value so the required
field is always populated when marshaling.
In `@packages/gen-ai/bff/internal/models/maas_models.go`:
- Around line 14-20: The json tag for MaaSModel.ModelType must not be optional:
remove `omitempty` so `ModelType` is always serialized (change
`json:"model_type,omitempty"` to `json:"model_type"`); keep the DeriveModelType
method (which calls DeriveModelTypeFromUsecase) to populate ModelType when
needed and ensure any code paths that construct MaaSModel call DeriveModelType
before serialization so the required `model_type` is present.
In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTable.tsx`:
- Around line 31-40: The RegisterExternalEndpointButton currently only calls
fireSimpleTrackingEvent and leaves the user stranded; update its onClick to
start the actual registration flow (e.g., navigate to the external-endpoint
registration route or open the registration modal) instead of only tracking.
Locate the RegisterExternalEndpointButton component and replace the noop
tracking-only handler with a call to the real flow (for example, call your
router's navigate('/...') or dispatch/open the RegisterExternalEndpoint modal
component) while still firing fireSimpleTrackingEvent; if the flow doesn't exist
yet, disable or hide the Button (Button variant usage and data-testid should
remain) to avoid presenting a dead-end primary CTA.
In
`@packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx`:
- Around line 144-156: The generate button is hidden when an error is set
because the render condition uses "!tokenData && !error"; update the render
logic in EndpointDetailModal so the button displays whenever there is no token
(i.e., use "!tokenData" instead of "!tokenData && !error") so users can retry
generateToken() after failures, and ensure the existing error Alert is rendered
alongside the button (leave the error Alert rendering intact). Also apply the
same change to the other identical block that renders the generate button in
this component.
---
Outside diff comments:
In `@packages/gen-ai/frontend/src/app/AIAssets/AIAssetsPage.tsx`:
- Around line 89-96: The MaaS tab content is lost because LazyCodeRefComponent
is rendered without children while GenAiMaaSTabWrapper (and the ai-assets
extension typing ComponentType<{ children?: React.ReactNode }>) expects
children; fix by either passing the tab body as children into the dynamically
loaded component (e.g., render <LazyCodeRefComponent
component={extension.properties.component} children={tabContent} fallback={...}
/> or equivalently include {tabContent} between the component tags) so existing
GenAiMaaSTabWrapper receives and renders it, or update the MaaS extension
(GenAiMaaSTabWrapper / the extension component registered in
extension.properties.component) to own and render its own content instead of
expecting children—choose one approach and make the corresponding change to
LazyCodeRefComponent usage or the GenAiMaaSTabWrapper implementation and the
ai-assets extension typing.
In
`@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationModal.tsx`:
- Around line 56-64: Restore ID-based de-duplication when building allModels:
ensure the merged list produced in the React.useMemo that constructs allModels
deduplicates entries by the model identifier (model_id) coming from
convertMaaSModelToAIModel so an AI asset and a MaaS model with the same id don't
both appear; implement a filter or Map that keeps the first (or preferred) entry
per model_id when merging aiModels and maasAsAIModels, so the modal’s
preselection in create mode won't submit duplicate pairs to installLSD(); update
the React.useMemo that defines allModels to perform this de-duplication.
---
Duplicate comments:
In `@packages/gen-ai/bff/openapi/src/gen-ai.yaml`:
- Around line 2008-2011: Update the example value for the model_display_name
field to fix the typo: replace the current example 'Gemini 2.5 Flash Liter' with
the correct display name (e.g., 'Gemini 2.5 Flash Lighter') in the OpenAPI
schema so generated docs/examples use the corrected model_display_name.
In `@packages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsx`:
- Around line 78-90: The component AIAssetsModelsTab currently renders
emptyState whenever models.length === 0 even if there are warnings; change the
conditional so that when warnings.length > 0 && models.length === 0 you render a
neutral failure empty state (create or reuse a fallback like failureEmptyState)
instead of the onboarding emptyState. Locate the render block that uses
warnings, Alert, models and emptyState and update the ternary (models.length ===
0 ? emptyState : ...) to first check for warnings (if warnings.length > 0 &&
models.length === 0 return the neutral failureEmptyState; else if models.length
=== 0 return emptyState; otherwise render the models list).
In
`@packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx`:
- Around line 53-215: The modal claims "authentication details" but only shows
tokens for MaaS (isMaaS); for namespace/external_cluster models that have
AIModel.sa_token either display that token (add a section like the existing
isMaaS token block: label it "Service account token", show ClipboardCopy for
model.sa_token with copyToClipboardWithTracking and an appropriate testid and
aria-label) or if no sa_token is available change the intro Content (the
paragraph under ModalBody) to remove "along with the authentication details" so
it only mentions the endpoint URL; update logic around hasExternal/hasInternal
and isMaaS to surface model.sa_token where present and reuse existing UI
patterns (Alert info, ClipboardCopy, copy tracking) so users can actually
authenticate.
In `@packages/gen-ai/frontend/src/app/AIAssets/data/columns.tsx`:
- Around line 27-34: Update the "Public route" help text in the UI component
that renders Content with ContentVariants.dt/dd and the Label "Public route" (in
columns.tsx) so it no longer implies exclusivity to Model Registry entries;
change the description text under ContentVariants.dd to a broader phrasing that
covers both Model Registry-registered endpoints and manually configured
cluster-local or cross-namespace endpoints (e.g., "Endpoints accessible across
namespaces within the cluster, including registry-registered and manually
configured routes"). Ensure you modify only the string content rendered by the
Content component and keep the existing structure using ContentVariants.dt and
ContentVariants.dd.
---
Nitpick comments:
In `@packages/gen-ai/bff/internal/api/maas_models_handler_test.go`:
- Around line 57-64: The test currently assumes the target model is at
response.Data[0]; update the assertion to first locate the model with ID
"llama-2-7b-chat" in response.Data (e.g., loop or helper to find by model.ID)
and assign that to firstModel, then assert firstModel.URL, firstModel.ModelType
and other fields; also add an assertion that the model was found to avoid
nil/index errors. Use the existing symbols response.Data, model.ID, firstModel,
URL and ModelType to locate and validate the correct item instead of relying on
slice order.
In `@packages/gen-ai/frontend/src/app/hooks/__tests__/useMergedModels.spec.ts`:
- Around line 21-37: The fixture helper createAIModel sets model_id incorrectly
from overrides.model_name which ignores callers that provide overrides.model_id;
update createAIModel so model_id is assigned from overrides.model_id if present,
otherwise fall back to overrides.model_name, and finally to the default
'model-name' (i.e., compute model_id from overrides.model_id ??
overrides.model_name ?? 'model-name') while still spreading ...overrides at the
end so other override fields apply.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d3563ab-8ec6-4a35-989e-b438586f5a78
📒 Files selected for processing (66)
packages/gen-ai/bff/README.mdpackages/gen-ai/bff/internal/api/external_models_handler_test.gopackages/gen-ai/bff/internal/api/maas_middleware_test.gopackages/gen-ai/bff/internal/api/maas_models_handler_test.gopackages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client.gopackages/gen-ai/bff/internal/integrations/maas/maasmocks/maas_client_mock.gopackages/gen-ai/bff/internal/models/aaa_models.gopackages/gen-ai/bff/internal/models/maas_models.gopackages/gen-ai/bff/internal/repositories/external_models.gopackages/gen-ai/bff/internal/repositories/maas_models.gopackages/gen-ai/bff/openapi/src/gen-ai.yamlpackages/gen-ai/frontend/config/moduleFederation.jspackages/gen-ai/frontend/src/__tests__/cypress/cypress/__mocks__/index.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/__mocks__/mockMaaSModels.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/pages/aiAssetsPage.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/pages/modelsTabPage.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/aiAssets/modelsTab.cy.tspackages/gen-ai/frontend/src/app/AIAssets/AIAssetsMaaSTab.tsxpackages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsxpackages/gen-ai/frontend/src/app/AIAssets/AIAssetsPage.tsxpackages/gen-ai/frontend/src/app/AIAssets/__tests__/AIAssetsMaaSTab.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/__tests__/AIAssetsModelsTab.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTable.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTableRowEndpoint.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRow.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRowEndpoint.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTable.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTableRowInfo.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/TierInfoPopover.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelTableRow.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelsTable.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelsTableRowEndpoint.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelsTableRowEndpoint.tracking.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/EndpointDetailModal.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelTableRow.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelTableRowEndpoint.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelTableRowEndpoint.tracking.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelsTable.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelsTableRowInfo.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/ModelsListToolbar.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/TierInfoPopover.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/data/columns.tspackages/gen-ai/frontend/src/app/AIAssets/data/columns.tsxpackages/gen-ai/frontend/src/app/AIAssets/data/filterOptions.tspackages/gen-ai/frontend/src/app/AIAssets/data/maasColumns.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useAIModelsFilter.spec.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useMaaSModelsFilter.spec.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/useAIModelsFilter.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/useMaaSModelsFilter.tspackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationModal.tsxpackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationTableRow.tsxpackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationModal.spec.tsxpackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationTableRow.spec.tsxpackages/gen-ai/frontend/src/app/hooks/__tests__/useMergedModels.spec.tspackages/gen-ai/frontend/src/app/hooks/useFetchAIModels.tspackages/gen-ai/frontend/src/app/hooks/useMergedModels.tspackages/gen-ai/frontend/src/app/services/__tests__/llamaStackService.fixtures.tspackages/gen-ai/frontend/src/app/types.tspackages/gen-ai/frontend/src/app/utilities/__tests__/utils.spec.tspackages/gen-ai/frontend/src/app/utilities/routes.tspackages/gen-ai/frontend/src/app/utilities/utils.tspackages/gen-ai/frontend/src/odh/extension-points/maas.ts
💤 Files with no reviewable changes (21)
- packages/gen-ai/frontend/src/app/AIAssets/hooks/tests/useMaaSModelsFilter.spec.ts
- packages/gen-ai/frontend/config/moduleFederation.js
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/AIModelsTableRowEndpoint.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/AIModelsTableRowEndpoint.tracking.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTableRowEndpoint.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/TierInfoPopover.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRowEndpoint.tsx
- packages/gen-ai/frontend/src/app/AIAssets/tests/AIAssetsMaaSTab.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelTableRowEndpoint.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRow.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelTableRowEndpoint.tracking.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/hooks/useMaaSModelsFilter.ts
- packages/gen-ai/frontend/src/app/AIAssets/data/maasColumns.ts
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTableRowInfo.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTable.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelTableRow.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/TierInfoPopover.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelsTableRowInfo.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/AIAssetsMaaSTab.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelsTable.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/data/columns.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationTableRow.tsx
- packages/gen-ai/bff/internal/api/external_models_handler_test.go
- packages/gen-ai/frontend/src/odh/extension-points/maas.ts
- packages/gen-ai/frontend/src/tests/cypress/cypress/pages/aiAssetsPage.ts
- packages/gen-ai/frontend/src/app/services/tests/llamaStackService.fixtures.ts
- packages/gen-ai/bff/internal/repositories/external_models.go
- packages/gen-ai/frontend/src/app/utilities/routes.ts
- packages/gen-ai/frontend/src/app/hooks/useMergedModels.ts
- packages/gen-ai/frontend/src/tests/cypress/cypress/tests/mocked/aiAssets/modelsTab.cy.ts
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/ModelsListToolbar.spec.tsx
- packages/gen-ai/bff/internal/api/maas_middleware_test.go
- packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/tests/ChatbotConfigurationTableRow.spec.tsx
- packages/gen-ai/bff/internal/repositories/maas_models.go
- packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/tests/ChatbotConfigurationModal.spec.tsx
- packages/gen-ai/frontend/src/app/types.ts
- packages/gen-ai/frontend/src/tests/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.ts
- packages/gen-ai/bff/README.md
- packages/gen-ai/frontend/src/tests/cypress/cypress/mocks/mockMaaSModels.ts
- packages/gen-ai/frontend/src/tests/cypress/cypress/pages/modelsTabPage.ts
- packages/gen-ai/frontend/src/app/hooks/useFetchAIModels.ts
packages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTable.tsx
Outdated
Show resolved
Hide resolved
packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx
Outdated
Show resolved
Hide resolved
Contributor
Author
Thanks for the feedback! I understand the concern about PR size, so let me add some context: The diff looks large (66 files), but 22 of those are pure deletions -- old MaaS-specific components and their tests being removed. The remaining frontend changes (data layer, UI, endpoint modal) are tightly coupled and need to land together for the Models page to function, so splitting them into separate PRs would break intermediate states and increase the time-to-deliver without a real review benefit. That said, to make it easier to review, I've reorganized the PR into 3 sequential commits:
I'd recommend reviewing commit-by-commit -- the BFF and Cypress commits are small, and the frontend commit's size is mostly the MaaS deletions. |
Contributor
|
@Lucifergene thanks for the PR. Overall the code changes looks good to me. Looking at the demo video I noticed that the model title and the help icon are not properly aligned when the model title is long and is looking weird. I would suggest to keep the icon inline with the first title row and let the title wrap into mulriple line if its long or we can truncate it and on hover show the full title.
|
NickGagan
requested changes
Mar 10, 2026
Contributor
NickGagan
left a comment
NickGagan
left a comment
There was a problem hiding this comment.
I didn't run it yet, but added a few comments from reading through the code.
packages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTable.tsx
Outdated
Show resolved
Hide resolved
Contributor
Author
@divyanshiGupta We can definitely work on this in a separate PR along with other cosmetic changes raised by UX. |
Lucifergene
force-pushed
the
unify-models-tab
branch
from
965dd5c to
178d378
Compare
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/gen-ai/bff/README.md (1)
317-334:⚠️ Potential issue | 🟡 MinorAdd
model_typeto these MaaS response examples.The PR now treats
model_typeas part of the MaaS payload, but these JSON samples still omit it. Anyone copying them for manual testing or mocks will end up with a stale response shape; these examples look like they should include"model_type": "llm".Also applies to: 1074-1107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/bff/README.md` around lines 317 - 334, The JSON example objects for the MaaS response are missing the new model_type field; update each model object (e.g., the entries with id "llama-2-7b-chat" and "mistral-7b-instruct") to include "model_type": "llm" (or the appropriate value) so the sample matches the current payload shape, and make the same addition in the other example block referenced (the similar JSON under the later example section).packages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTable.tsx (1)
33-45:⚠️ Potential issue | 🟡 MinorRefresh this popover copy for the unified table.
This text now says the page only shows model deployments editable from the Model deployments page, but the tab also includes MaaS models. If a MaaS entry is missing, this guidance sends users to the wrong place.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTable.tsx` around lines 33 - 45, Update the popover copy held in dontSeeModelPopoverContent to reflect that the unified table includes both model deployments and MaaS models: change the guidance so it tells users to edit a deployment from the Model deployments page and to manage MaaS entries from the MaaS (or Model registry) management page, so users missing a MaaS entry are directed to the correct place; locate and update the string literals inside the dontSeeModelPopoverContent React node.
♻️ Duplicate comments (1)
packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx (1)
131-145:⚠️ Potential issue | 🟠 MajorRemove asset is still a dead-end action.
This menu item only emits analytics; it never calls a remove/unpublish flow or updates the table. A destructive action with no effect is misleading—either wire it to the real handler or hide it until that flow exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx` around lines 131 - 145, The "Remove asset" menu item in AIModelTableRow currently only fires fireMiscTrackingEvent and does not perform any removal or UI update; update the ActionsColumn items array in AIModelTableRow so the onClick both fires the analytics and calls the actual removal flow (e.g., call a provided prop like onRemove(model.model_id) or invoke the existing unpublish/removeAsset API function), handle success/failure (optimistic UI remove or await API then trigger a table refresh/refetch), and only show the menu item when a removal handler exists (or hide it until implemented) to avoid a dead-end destructive action.
🧹 Nitpick comments (2)
packages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.ts (1)
41-46: Consider aligning the handling pattern for aiModels and maasModels.The maasModels intercept explicitly falls back to
mockEmptyList()when not provided (lines 43-46), while aiModels passesundefineddirectly tomockAAModels(line 41). This inconsistency impliesmockAAModelshas built-in default data when called without arguments, which could be non-obvious to future maintainers.♻️ Optional: Align patterns for consistency
If
mockAAModels()returns defaults when called without args, consider documenting this explicitly or aligning the pattern:- cy.interceptGenAi('GET /api/v1/aaa/models', mockAAModels(options.aiModels)).as('aaModels'); + cy.interceptGenAi( + 'GET /api/v1/aaa/models', + options.aiModels ? mockAAModels(options.aiModels) : mockAAModels(), + ).as('aaModels');Alternatively, if the current behavior is intentional, a brief comment clarifying that
mockAAModels()provides default models would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.ts` around lines 41 - 46, The aiModels and maasModels intercepts use different fallback patterns; update the aiModels call in the cy.interceptGenAi invocation that uses mockAAModels to mirror maasModels by explicitly passing options.aiModels ? mockAAModels(options.aiModels) : mockEmptyList(), or alternatively add a short comment next to the mockAAModels(...) call documenting that mockAAModels() returns a default set when called without args—adjust the code around the cy.interceptGenAi('GET /api/v1/aaa/models', ...) line and reference mockAAModels, mockMaaSModels, and mockEmptyList accordingly.packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationModal.spec.tsx (1)
349-398: Assert source metadata in the overlap tests.These cases only inspect
model_name, so they still pass if the MaaS copy loses its source identity during conversion. Please expose/assertisMaaSModelormaasModelIdfrom the mocked table so the tests actually cover the namespace-vs-MaaS behavior this PR is changing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationModal.spec.tsx` around lines 349 - 398, The tests only assert display/model_name and miss verifying source metadata, so update the test cases that use createAIModel/createMaaSModel and renderModal to also assert the source identity: ensure the mocked MaaS entries include maasModelId (or isMaaSModel) when created via createMaaSModel and then add assertions after renderModal that the rendered model objects (or the internal selected-model list returned by getSelectedModelNames or a new helper like getSelectedModels) expose maasModelId === '<id>' or isMaaSModel === true for MaaS models and isMaaSModel === false (or no maasModelId) for namespace models; update the three tests (duplicate model, MaaS alongside different namespace, multiple overlap) to assert these properties for the relevant items so the tests fail if MaaS identity is lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/gen-ai/frontend/src/app/AIAssets/components/__tests__/ModelsListToolbar.spec.tsx`:
- Around line 18-21: The test fixture's defaultProps.filterOptions only includes
AssetsFilterOptions.NAME and AssetsFilterOptions.USE_CASE but must include the
unified-model filters; update defaultProps.filterOptions in
ModelsListToolbar.spec.tsx to also include AssetsFilterOptions.SOURCE,
AssetsFilterOptions.STATUS, and AssetsFilterOptions.MODEL_TYPE with appropriate
labels so the spec exercises the unified dropdowns. Ensure the added options
mirror the shape used by AIModelsTable.tsx (same keys/labels) so tests covering
dropdown behavior, active-chip rendering, color application, and tracking for
SOURCE, STATUS, and MODEL_TYPE run against the correct props.
In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx`:
- Around line 24-25: The playground-membership checks are currently matching
only on model_id (props playgroundModels vs allModels), causing models with the
same id from different sources to be treated as the same; update all membership
lookups in AIModelTableRow (where playgroundModels is queried and where the “Try
in playground” routing is decided) to compare a composite key of model_id +
source (e.g., `${model.model_id}:${model.source}`) instead of model_id alone,
and ensure any created maps/sets (used for quick lookup) and route params use
the same composite identity so sibling rows from different sources are
distinguished.
In `@packages/gen-ai/frontend/src/app/AIAssets/hooks/useAIModelsFilter.ts`:
- Around line 50-70: The filter is trimming only as a guard but still uses the
original untrimmed strings in comparisons; update useAIModelsFilter.ts so you
normalize once (e.g., const nameFilter = (typeof
filterData[AssetsFilterOptions.NAME] === 'string' ?
filterData[AssetsFilterOptions.NAME].trim().toLowerCase() : '') ) and use that
normalized variable in the includes() check against
model.model_name.toLowerCase(), and do the same for the use case value (e.g.,
const useCaseFilter = ... and use
model.usecase.toLowerCase().includes(useCaseFilter)); likewise ensure source
comparisons use normalized labels if needed when comparing
getSourceLabel(model).
In
`@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationModal.tsx`:
- Around line 56-59: The maasAsAIModels mapping currently feeds every MaaS model
into allModels causing ID collisions with existingModels; update the
maasAsAIModels creation (where maasModels is mapped via
convertMaaSModelToAIModel) to either filter out MaaS models whose id matches any
model in existingModels or augment the converted AIModel with a source-aware
identifier and adjust selection state to use that source-aware id; target the
maasAsAIModels variable and the selection/save logic that reads
allModels/existingModels (so the update-path lookup at Line 70 no longer
ambiguously matches namespace vs MaaS copy).
In `@packages/gen-ai/frontend/src/app/hooks/useMergedModels.ts`:
- Around line 37-39: The refresh callback in useMergedModels.ts currently uses
Promise.all([refreshAI(), refreshMaaS()]) which rejects if either refresh fails;
change it to use Promise.allSettled([refreshAI(), refreshMaaS()]) and ignore
individual rejections so the callback resolves to undefined regardless of
partial failures, preserving the same “tolerate partial failures” contract used
during initial load; update the React.useCallback for refresh to call
Promise.allSettled, then return undefined (or map results to ensure no
rejection) while keeping dependencies [refreshAI, refreshMaaS].
---
Outside diff comments:
In `@packages/gen-ai/bff/README.md`:
- Around line 317-334: The JSON example objects for the MaaS response are
missing the new model_type field; update each model object (e.g., the entries
with id "llama-2-7b-chat" and "mistral-7b-instruct") to include "model_type":
"llm" (or the appropriate value) so the sample matches the current payload
shape, and make the same addition in the other example block referenced (the
similar JSON under the later example section).
In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTable.tsx`:
- Around line 33-45: Update the popover copy held in dontSeeModelPopoverContent
to reflect that the unified table includes both model deployments and MaaS
models: change the guidance so it tells users to edit a deployment from the
Model deployments page and to manage MaaS entries from the MaaS (or Model
registry) management page, so users missing a MaaS entry are directed to the
correct place; locate and update the string literals inside the
dontSeeModelPopoverContent React node.
---
Duplicate comments:
In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx`:
- Around line 131-145: The "Remove asset" menu item in AIModelTableRow currently
only fires fireMiscTrackingEvent and does not perform any removal or UI update;
update the ActionsColumn items array in AIModelTableRow so the onClick both
fires the analytics and calls the actual removal flow (e.g., call a provided
prop like onRemove(model.model_id) or invoke the existing unpublish/removeAsset
API function), handle success/failure (optimistic UI remove or await API then
trigger a table refresh/refetch), and only show the menu item when a removal
handler exists (or hide it until implemented) to avoid a dead-end destructive
action.
---
Nitpick comments:
In
`@packages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.ts`:
- Around line 41-46: The aiModels and maasModels intercepts use different
fallback patterns; update the aiModels call in the cy.interceptGenAi invocation
that uses mockAAModels to mirror maasModels by explicitly passing
options.aiModels ? mockAAModels(options.aiModels) : mockEmptyList(), or
alternatively add a short comment next to the mockAAModels(...) call documenting
that mockAAModels() returns a default set when called without args—adjust the
code around the cy.interceptGenAi('GET /api/v1/aaa/models', ...) line and
reference mockAAModels, mockMaaSModels, and mockEmptyList accordingly.
In
`@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationModal.spec.tsx`:
- Around line 349-398: The tests only assert display/model_name and miss
verifying source metadata, so update the test cases that use
createAIModel/createMaaSModel and renderModal to also assert the source
identity: ensure the mocked MaaS entries include maasModelId (or isMaaSModel)
when created via createMaaSModel and then add assertions after renderModal that
the rendered model objects (or the internal selected-model list returned by
getSelectedModelNames or a new helper like getSelectedModels) expose maasModelId
=== '<id>' or isMaaSModel === true for MaaS models and isMaaSModel === false (or
no maasModelId) for namespace models; update the three tests (duplicate model,
MaaS alongside different namespace, multiple overlap) to assert these properties
for the relevant items so the tests fail if MaaS identity is lost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ff6a419-b777-4cb5-8021-b4e7e88b74ba
📒 Files selected for processing (66)
packages/gen-ai/bff/README.mdpackages/gen-ai/bff/internal/api/external_models_handler_test.gopackages/gen-ai/bff/internal/api/maas_middleware_test.gopackages/gen-ai/bff/internal/api/maas_models_handler_test.gopackages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client.gopackages/gen-ai/bff/internal/integrations/maas/maasmocks/maas_client_mock.gopackages/gen-ai/bff/internal/models/aaa_models.gopackages/gen-ai/bff/internal/models/maas_models.gopackages/gen-ai/bff/internal/repositories/external_models.gopackages/gen-ai/bff/internal/repositories/maas_models.gopackages/gen-ai/bff/openapi/src/gen-ai.yamlpackages/gen-ai/frontend/config/moduleFederation.jspackages/gen-ai/frontend/src/__tests__/cypress/cypress/__mocks__/index.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/__mocks__/mockMaaSModels.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/pages/aiAssetsPage.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/pages/modelsTabPage.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/aiAssets/modelsTab.cy.tspackages/gen-ai/frontend/src/app/AIAssets/AIAssetsMaaSTab.tsxpackages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsxpackages/gen-ai/frontend/src/app/AIAssets/AIAssetsPage.tsxpackages/gen-ai/frontend/src/app/AIAssets/__tests__/AIAssetsMaaSTab.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/__tests__/AIAssetsModelsTab.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTable.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTableRowEndpoint.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRow.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRowEndpoint.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTable.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTableRowInfo.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/TierInfoPopover.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelTableRow.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelsTable.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelsTableRowEndpoint.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelsTableRowEndpoint.tracking.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/EndpointDetailModal.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelTableRow.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelTableRowEndpoint.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelTableRowEndpoint.tracking.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelsTable.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelsTableRowInfo.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/ModelsListToolbar.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/TierInfoPopover.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/data/columns.tspackages/gen-ai/frontend/src/app/AIAssets/data/columns.tsxpackages/gen-ai/frontend/src/app/AIAssets/data/filterOptions.tspackages/gen-ai/frontend/src/app/AIAssets/data/maasColumns.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useAIModelsFilter.spec.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useMaaSModelsFilter.spec.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/useAIModelsFilter.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/useMaaSModelsFilter.tspackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationModal.tsxpackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationTableRow.tsxpackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationModal.spec.tsxpackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationTableRow.spec.tsxpackages/gen-ai/frontend/src/app/hooks/__tests__/useMergedModels.spec.tspackages/gen-ai/frontend/src/app/hooks/useFetchAIModels.tspackages/gen-ai/frontend/src/app/hooks/useMergedModels.tspackages/gen-ai/frontend/src/app/services/__tests__/llamaStackService.fixtures.tspackages/gen-ai/frontend/src/app/types.tspackages/gen-ai/frontend/src/app/utilities/__tests__/utils.spec.tspackages/gen-ai/frontend/src/app/utilities/routes.tspackages/gen-ai/frontend/src/app/utilities/utils.tspackages/gen-ai/frontend/src/odh/extension-points/maas.ts
💤 Files with no reviewable changes (21)
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelTableRowEndpoint.tracking.spec.tsx
- packages/gen-ai/frontend/config/moduleFederation.js
- packages/gen-ai/frontend/src/app/AIAssets/AIAssetsMaaSTab.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRowEndpoint.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRow.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/AIModelsTableRowEndpoint.tracking.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/data/columns.ts
- packages/gen-ai/frontend/src/app/AIAssets/hooks/tests/useMaaSModelsFilter.spec.ts
- packages/gen-ai/frontend/src/app/AIAssets/tests/AIAssetsMaaSTab.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelTableRow.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelTableRowEndpoint.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/data/maasColumns.ts
- packages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTableRowEndpoint.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/TierInfoPopover.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelsTable.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/TierInfoPopover.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTable.tsx
- packages/gen-ai/frontend/src/app/AIAssets/hooks/useMaaSModelsFilter.ts
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTableRowInfo.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelsTableRowInfo.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/AIModelsTableRowEndpoint.spec.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/EndpointDetailModal.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/gen-ai/frontend/src/app/types.ts
- packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/tests/ChatbotConfigurationTableRow.spec.tsx
- packages/gen-ai/frontend/src/app/utilities/routes.ts
- packages/gen-ai/frontend/src/tests/cypress/cypress/pages/aiAssetsPage.ts
- packages/gen-ai/frontend/src/app/AIAssets/data/columns.tsx
- packages/gen-ai/frontend/src/odh/extension-points/maas.ts
- packages/gen-ai/bff/openapi/src/gen-ai.yaml
- packages/gen-ai/frontend/src/tests/cypress/cypress/tests/mocked/aiAssets/modelsTab.cy.ts
- packages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsx
- packages/gen-ai/bff/internal/api/maas_models_handler_test.go
- packages/gen-ai/frontend/src/tests/cypress/cypress/pages/modelsTabPage.ts
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/AIModelsTable.spec.tsx
- packages/gen-ai/frontend/src/tests/cypress/cypress/mocks/mockMaaSModels.ts
- packages/gen-ai/bff/internal/models/maas_models.go
packages/gen-ai/frontend/src/app/AIAssets/components/__tests__/ModelsListToolbar.spec.tsx
Show resolved
Hide resolved
packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx
Show resolved
Hide resolved
packages/gen-ai/frontend/src/app/AIAssets/hooks/useAIModelsFilter.ts
Outdated
Show resolved
Hide resolved
...en-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationModal.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/gen-ai/frontend/src/app/hooks/useMergedModels.ts (1)
32-35:⚠️ Potential issue | 🟠 MajorDeduplicate the merged list to prevent duplicate table rows.
The current implementation concatenates both sources without checking for duplicates. If the same model exists in AI models and MaaS (e.g., a MaaS model that was also registered locally), the unified table will render duplicate rows and may cause React key warnings.
🔧 Suggested fix using model_id as identity
const models = React.useMemo( - () => [...aiModels, ...maasModels.map(convertMaaSModelToAIModel)], + () => { + const seen = new Set<string>(); + const merged: AIModel[] = []; + // AI models take precedence + for (const model of aiModels) { + if (!seen.has(model.model_id)) { + seen.add(model.model_id); + merged.push(model); + } + } + for (const maasModel of maasModels) { + const converted = convertMaaSModelToAIModel(maasModel); + if (!seen.has(converted.model_id)) { + seen.add(converted.model_id); + merged.push(converted); + } + } + return merged; + }, [aiModels, maasModels], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/hooks/useMergedModels.ts` around lines 32 - 35, The merged models array in useMergedModels is built by simply concatenating aiModels and maasModels (after convertMaaSModelToAIModel), which can produce duplicate entries and duplicate table rows; update the React.useMemo that defines models to deduplicate by model_id (use model_id as the unique key), e.g. map items by model_id while merging convertMaaSModelToAIModel(maasModels) and aiModels so each model_id appears once (decide and document precedence: prefer aiModels over MaaS or vice versa) and return the deduplicated array; reference: useMergedModels, aiModels, maasModels, convertMaaSModelToAIModel, and the models constant.packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx (1)
56-59:⚠️ Potential issue | 🟠 MajorIntro copy promises authentication details that aren't shown for non-MaaS models.
The text states "Copy the endpoint URL below along with the authentication details" but for namespace and external_cluster models,
model.sa_tokenis never rendered—users only see endpoints. Either display the service account token for non-MaaS models when available, or adjust the copy to be conditional.🔧 Suggested copy adjustment
<Content component={ContentVariants.p}> - Use this endpoint to connect your application to this model. Copy the endpoint URL - below along with the authentication details to start making requests. + Use this endpoint to connect your application to this model. + {isMaaS + ? ' Generate an API token below to authenticate your requests.' + : ' Use your cluster credentials to authenticate requests.'} </Content>Alternatively, add a section that displays
model.sa_token.tokenfor non-MaaS models when it's available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx` around lines 56 - 59, The intro copy in EndpointDetailModal (the Content component) promises authentication details but model.sa_token is not shown for non-MaaS models (namespace and external_cluster); update EndpointDetailModal.tsx to either (A) conditionally render the sentence "along with the authentication details" only when model.sa_token?.token is present and model.type indicates a non-MaaS model, or (B) add a visible auth section that renders model.sa_token.token (styled and copyable) for namespace/external_cluster models when model.sa_token exists; reference the Content component and the model.sa_token property in your changes so the copy and displayed fields stay consistent.packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx (1)
132-145:⚠️ Potential issue | 🟠 MajorKeep playground actions source-aware.
This action state is still driven by a lookup/filter keyed only by
model_id. In the merged table, a namespace row and a MaaS row can share an id, so one source can light up the sibling row's actions or remove the wrong playground entry. Use the same composite identity here that you already use for the row key.Also applies to: 164-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx` around lines 132 - 145, The action handlers in ActionsColumn currently use only model.model_id (and enabledModel) which can collide across sources; change them to use the same composite identity you use for the row key (e.g., the existing composite id variable like rowKey / modelKey or `${source}-${model.model_id}`) for all action state and payloads: use the composite id instead of model.model_id when computing isDisabled/enabled state, when calling fireMiscTrackingEvent assetId, and when passing the id to the remove modal setter (setIsRemoveModalOpen usage/handler). Repeat the same replacement for the other action block at lines 164-169 so both actions are source-aware.
🧹 Nitpick comments (3)
packages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.ts (1)
13-23: Remove duplicateinterceptGenAitype declaration.This declaration duplicates the one already present in
genai.ts(see context snippet, lines 10-25), but with a narrower type signature. The implementation ingenai.tsacceptsGenAiOptionswhich includespath,query, andtimes, whereas this declaration only exposesquery.Having multiple declarations creates a maintenance burden and can cause confusion about the command's capabilities. The declaration in
genai.tsshould be the single source of truth.♻️ Proposed fix: Remove the duplicate declaration
-declare global { - // eslint-disable-next-line `@typescript-eslint/no-namespace` - namespace Cypress { - interface Chainable { - interceptGenAi: ( - type: string, - ...args: [{ query?: Record<string, string> } | null, unknown] | [unknown] - ) => Cypress.Chainable<null>; - } - } -} -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.ts` around lines 13 - 23, Remove the duplicate TypeScript global augmentation for Cypress.interceptGenAi from modelsTabTestHelpers.ts so the declaration in genai.ts remains the single source of truth; delete the declare global { namespace Cypress { interface Chainable { interceptGenAi: ... } } } block in modelsTabTestHelpers.ts and ensure any tests import or rely on the existing interceptGenAi signature (which accepts GenAiOptions including path, query, and times) from genai.ts instead of redefining it.packages/gen-ai/frontend/src/app/AIAssets/components/__tests__/ModelsListToolbar.spec.tsx (1)
58-98: Consider adding tests for select-type filter behavior.The current tests exercise text-input filters (NAME, USE_CASE) well, but the new select-based filters (SOURCE, STATUS, MODEL_TYPE) lack dedicated test coverage for:
- Opening the select dropdown
- Selecting/deselecting checkbox options
- Verifying multiple selections display in toggle text
- Tracking event emission for select-based filters
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/__tests__/ModelsListToolbar.spec.tsx` around lines 58 - 98, The tests are missing coverage for select-type filters (SOURCE, STATUS, MODEL_TYPE) in ModelsListToolbar; add specs in ModelsListToolbar.spec.tsx that render ModelsListToolbar with filterOptions including AssetsFilterOptions.SOURCE/STATUS/MODEL_TYPE, use userEvent to click the filter toggle (getByLabelText('Filter toggle')), open each select-style dropdown (find by role menu or button text), verify checkbox options appear, click to select and deselect options, assert the toggle label updates to reflect multiple selections, and assert the component emits the expected filter change event/handler (mock the onChange/onFilterChange prop) when options are selected/deselected.packages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelTableRow.spec.tsx (1)
163-171: Make this MaaS test exercise the MaaS-specific branch.
createMockAIModel()already supplies both endpoint fields, so this stays green even ifAIModelTableRowignoresisMaaSModelcompletely. Use a fixture that matches the real MaaS shape here so the new endpoint logic is actually covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelTableRow.spec.tsx` around lines 163 - 171, The test currently passes a model from createMockAIModel() which includes both endpoint variants so it doesn't exercise the MaaS-specific branch; update the test to supply a model shaped like a real MaaS model (set isMaaSModel: true and populate only the MaaS endpoint fields while removing or nulling the non-MaaS/local endpoint fields) and pass that into AIModelTableRow (using the same defaultProps) so the component's MaaS branch (the code that renders the element with test id 'endpoint-view-button') is actually executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsx`:
- Around line 114-149: The CTA and modal for registering external endpoints are
only rendered inside the models.length > 0 branch (see AIModelsTable and the
Button that calls setIsCreateEndpointModalOpen) so projects with zero models
cannot start the flow; move the isExternalModelsEnabled Button and the
CreateExternalEndpointModal out of that conditional (or also render the Button
inside the emptyState/ModelsEmptyState) so the Register external endpoint button
and CreateExternalEndpointModal (props: isOpen -> isCreateEndpointModalOpen,
onClose -> setIsCreateEndpointModalOpen(false), onSuccess ->
handleCreationSuccess, onSubmit -> handleCreateExternalEndpoint) are always
reachable regardless of models.length or warnings.
---
Duplicate comments:
In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx`:
- Around line 132-145: The action handlers in ActionsColumn currently use only
model.model_id (and enabledModel) which can collide across sources; change them
to use the same composite identity you use for the row key (e.g., the existing
composite id variable like rowKey / modelKey or `${source}-${model.model_id}`)
for all action state and payloads: use the composite id instead of
model.model_id when computing isDisabled/enabled state, when calling
fireMiscTrackingEvent assetId, and when passing the id to the remove modal
setter (setIsRemoveModalOpen usage/handler). Repeat the same replacement for the
other action block at lines 164-169 so both actions are source-aware.
In
`@packages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsx`:
- Around line 56-59: The intro copy in EndpointDetailModal (the Content
component) promises authentication details but model.sa_token is not shown for
non-MaaS models (namespace and external_cluster); update EndpointDetailModal.tsx
to either (A) conditionally render the sentence "along with the authentication
details" only when model.sa_token?.token is present and model.type indicates a
non-MaaS model, or (B) add a visible auth section that renders
model.sa_token.token (styled and copyable) for namespace/external_cluster models
when model.sa_token exists; reference the Content component and the
model.sa_token property in your changes so the copy and displayed fields stay
consistent.
In `@packages/gen-ai/frontend/src/app/hooks/useMergedModels.ts`:
- Around line 32-35: The merged models array in useMergedModels is built by
simply concatenating aiModels and maasModels (after convertMaaSModelToAIModel),
which can produce duplicate entries and duplicate table rows; update the
React.useMemo that defines models to deduplicate by model_id (use model_id as
the unique key), e.g. map items by model_id while merging
convertMaaSModelToAIModel(maasModels) and aiModels so each model_id appears once
(decide and document precedence: prefer aiModels over MaaS or vice versa) and
return the deduplicated array; reference: useMergedModels, aiModels, maasModels,
convertMaaSModelToAIModel, and the models constant.
---
Nitpick comments:
In
`@packages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.ts`:
- Around line 13-23: Remove the duplicate TypeScript global augmentation for
Cypress.interceptGenAi from modelsTabTestHelpers.ts so the declaration in
genai.ts remains the single source of truth; delete the declare global {
namespace Cypress { interface Chainable { interceptGenAi: ... } } } block in
modelsTabTestHelpers.ts and ensure any tests import or rely on the existing
interceptGenAi signature (which accepts GenAiOptions including path, query, and
times) from genai.ts instead of redefining it.
In
`@packages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelTableRow.spec.tsx`:
- Around line 163-171: The test currently passes a model from
createMockAIModel() which includes both endpoint variants so it doesn't exercise
the MaaS-specific branch; update the test to supply a model shaped like a real
MaaS model (set isMaaSModel: true and populate only the MaaS endpoint fields
while removing or nulling the non-MaaS/local endpoint fields) and pass that into
AIModelTableRow (using the same defaultProps) so the component's MaaS branch
(the code that renders the element with test id 'endpoint-view-button') is
actually executed.
In
`@packages/gen-ai/frontend/src/app/AIAssets/components/__tests__/ModelsListToolbar.spec.tsx`:
- Around line 58-98: The tests are missing coverage for select-type filters
(SOURCE, STATUS, MODEL_TYPE) in ModelsListToolbar; add specs in
ModelsListToolbar.spec.tsx that render ModelsListToolbar with filterOptions
including AssetsFilterOptions.SOURCE/STATUS/MODEL_TYPE, use userEvent to click
the filter toggle (getByLabelText('Filter toggle')), open each select-style
dropdown (find by role menu or button text), verify checkbox options appear,
click to select and deselect options, assert the toggle label updates to reflect
multiple selections, and assert the component emits the expected filter change
event/handler (mock the onChange/onFilterChange prop) when options are
selected/deselected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 922b38c7-4664-4c21-b85c-150b7e48c00a
📒 Files selected for processing (19)
packages/gen-ai/bff/README.mdpackages/gen-ai/bff/internal/models/aaa_models.gopackages/gen-ai/bff/internal/models/maas_models.gopackages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/aiAssets/modelsTab.cy.tspackages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTable.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelTableRow.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/ModelsListToolbar.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/data/columns.tsxpackages/gen-ai/frontend/src/app/AIAssets/hooks/useAIModelsFilter.tspackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationTableRow.spec.tsxpackages/gen-ai/frontend/src/app/hooks/__tests__/useMergedModels.spec.tspackages/gen-ai/frontend/src/app/hooks/useMergedModels.tspackages/gen-ai/frontend/src/app/utilities/__tests__/utils.spec.tspackages/gen-ai/frontend/src/app/utilities/utils.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/gen-ai/frontend/src/tests/cypress/cypress/tests/mocked/aiAssets/modelsTab.cy.ts
- packages/gen-ai/bff/internal/models/maas_models.go
- packages/gen-ai/bff/internal/models/aaa_models.go
- packages/gen-ai/frontend/src/app/AIAssets/data/columns.tsx
- packages/gen-ai/frontend/src/app/hooks/tests/useMergedModels.spec.ts
- packages/gen-ai/frontend/src/app/utilities/tests/utils.spec.ts
Contributor
|
I can test again once the merge conflicts are resolved, there are still ongoing conversations with design about this page, but that can be a FUP. Based on conversations I've had with @ederign, we'll need that logic to set an embedding model using a use case removed though. @Lucifergene how confident are you that there will be no regressions? This is a pretty big change to get in right before a code freeze. |
- Expose model_type (llm/embedding) on AAModel and MaaS models - Fetch model sources concurrently via errgroup - Update OpenAPI spec with model_type enum RHOAIENG-52234 Made-with: Cursor
- Add modelSource/model_type fields, useMergedModels hook, utility functions - Source/Model type columns, multi-select filters, unified toolbar - EndpointDetailModal replacing per-source endpoint popovers - Remove MaaS tab and ~3,000 lines of dead code RHOAIENG-52236 RHOAIENG-52237 RHOAIENG-52240 Made-with: Cursor
- MaaS model mock factories - ModelsTab page object and test helpers - Test spec validating unified table columns and data RHOAIENG-52241 Made-with: Cursor
Signed-off-by: Avik Kundu <47265560+Lucifergene@users.noreply.github.com>
Signed-off-by: Avik Kundu <47265560+Lucifergene@users.noreply.github.com>
Signed-off-by: Avik Kundu <47265560+Lucifergene@users.noreply.github.com>
Lucifergene
force-pushed
the
unify-models-tab
branch
from
bf900ab to
8a9eeef
Compare
Signed-off-by: Avik Kundu <47265560+Lucifergene@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/gen-ai/frontend/src/app/AIAssets/AIAssetsPage.tsx (1)
89-96:⚠️ Potential issue | 🟠 MajorNarrow the tab extension contract or keep forwarding
children.Lines 89-96 now render every
AIAssetsTabExtensionas a leaf component, butpackages/gen-ai/frontend/src/odh/extension-points/ai-assets.ts:1-15still defines the extension point asComponentType<{ children?: React.ReactNode }>. That leaves wrapper-style extensions in a bad state: they still type-check, but their composition hook disappears at runtime. Either restore the nested render path here or tighten the extension-point type/docs/tests so this becomes a compile-time break instead of a plugin regression.As per coding guidelines, "Architectural issues and anti-patterns" are a review priority.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/AIAssetsPage.tsx` around lines 89 - 96, The current render of LazyCodeRefComponent using extension.properties.component discards any children expected by AIAssetsTabExtension (declared ComponentType<{ children?: React.ReactNode }>), breaking wrapper-style extensions; either update this render to forward children (render LazyCodeRefComponent with nested children from the tab extension so the extension's composition hook runs) by passing extension.properties.children (or using <LazyCodeRefComponent ...>{extension.properties.children}</LazyCodeRefComponent>), or tighten the extension-point type/contract to remove children from AIAssetsTabExtension so mismatched wrappers fail at compile time—pick one approach and apply it consistently (update the call site in AIAssetsPage and/or the AIAssetsTabExtension type definition).packages/gen-ai/bff/openapi/src/gen-ai.yaml (1)
3981-3985:⚠️ Potential issue | 🟡 MinorFix the external-model example to use the labeled endpoint format.
CreateExternalModelnow returns endpoints as"external: <url>"or"internal: <url>", but this example still shows a bare URL. That drifts the generated docs away from the actual payload shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/bff/openapi/src/gen-ai.yaml` around lines 3981 - 3985, The example for external model endpoints in gen-ai.yaml is using a bare URL but CreateExternalModel now returns endpoints with labeled keys; update the endpoints entry under the external-model example to use the labeled format (e.g., "external: https://generativelanguage.googleapis.com/v1beta/openai/") so the example matches the actual payload shape and CreateExternalModel behavior.
♻️ Duplicate comments (2)
packages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client.go (1)
584-593:⚠️ Potential issue | 🟠 MajorStop swallowing external-model fetch failures.
This goroutine still returns
nilfor every error. A malformed, forbidden, or otherwise broken external-model ConfigMap becomes a200with silently missing rows, so callers cannot distinguish “no external models” from “external models failed to load.” Only the expected missing-ConfigMap case should fall back to[]; propagate everything else. 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 `@packages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client.go` around lines 584 - 593, The goroutine currently swallows all errors from kc.GetAAModelsFromExternalModels by always returning nil; change it to only fall back to aaModelsFromExternal = []models.AAModel{} when the error is the expected "not found" case, and return the error for any other failure so callers can handle it. Concretely, inside the g.Go closure that calls kc.GetAAModelsFromExternalModels(gCtx, identity, namespace) check the error with the appropriate Kubernetes helper (e.g., apierrors.IsNotFound(err) or the sentinel returned by GetAAModelsFromExternalModels) and only on that path set aaModelsFromExternal = []models.AAModel{} and log at Warn; for all other non-nil errors return err from the goroutine instead of nil. Ensure you reference aaModelsFromExternal, kc.GetAAModelsFromExternalModels, and kc.Logger.Warn when making the change.packages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsx (1)
124-148:⚠️ Potential issue | 🟠 MajorKeep external model registration reachable when the table is empty.
Register external endpointandCreateExternalEndpointModalstill only render in themodels.length > 0branch. A namespace with zero models cannot add its first external model, which blocks the main recovery path from the empty state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsx` around lines 124 - 148, The Register external endpoint button and CreateExternalEndpointModal are only rendered inside the branch gated by models.length > 0, preventing namespaces with zero models from registering the first external model; move the toolbarActions prop containing the Button (used to call setIsCreateEndpointModalOpen(true)) and the CreateExternalEndpointModal (using isCreateEndpointModalOpen, onClose -> setIsCreateEndpointModalOpen(false), onSuccess -> handleCreationSuccess, onSubmit -> handleCreateExternalEndpoint) out of the models.length check so they render whenever isExternalModelsEnabled is true, and keep AIModelsTable, isExternalModelsEnabled, setIsCreateEndpointModalOpen, handleCreationSuccess, and handleCreateExternalEndpoint references intact.
🧹 Nitpick comments (6)
packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationTableRow.tsx (1)
132-142: String literal comparison with utility constant.
sourceLabel !== 'Internal'relies on the string'Internal'matchingSOURCE_LABELS.namespacein utils.ts. Consider exporting a constant or using an enum to avoid magic string drift.Example
// In utils.ts export const SOURCE_LABEL_INTERNAL = 'Internal'; // In ChatbotConfigurationTableRow.tsx import { SOURCE_LABEL_INTERNAL } from '~/app/utilities/utils'; // ... {sourceLabel !== SOURCE_LABEL_INTERNAL && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationTableRow.tsx` around lines 132 - 142, The component uses a magic string check sourceLabel !== 'Internal' which can drift from the canonical value in utilities; replace the literal with an exported constant (or enum) from your utilities module (e.g., SOURCE_LABELS.namespace or a new SOURCE_LABEL_INTERNAL) and import it into ChatbotConfigurationTableRow.tsx, then change the condition to compare against that constant while keeping the rest of the rendering (including getSourceLabelColor and data-testid) unchanged.packages/gen-ai/frontend/src/app/AIAssets/components/__tests__/EndpointDetailModal.spec.tsx (1)
17-23: Mutable mock pattern works but could be cleaner.
mockUseGenerateMaaSTokenis declared withletand reassigned in individual tests. Consider usingmockReturnValuewithin each test instead of reassigning the entire mock function, which is more idiomatic Jest.Alternative pattern
// At module level const mockHookReturn = { isGenerating: false, tokenData: null, error: null, generateToken: mockGenerateToken, resetToken: mockResetToken, }; jest.mock('~/app/hooks/useGenerateMaaSToken', () => ({ __esModule: true, default: jest.fn(() => mockHookReturn), })); // In tests, mutate mockHookReturn properties directly: // mockHookReturn.tokenData = { token: 'generated-token-123' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/__tests__/EndpointDetailModal.spec.tsx` around lines 17 - 23, Replace the current mutable reassign pattern for mockUseGenerateMaaSToken with an idiomatic Jest per-test return strategy: keep mockUseGenerateMaaSToken as a single jest.fn() and in each test call mockUseGenerateMaaSToken.mockReturnValue(...) (or create a shared mutable object like mockHookReturn and have the module mock return jest.fn(() => mockHookReturn) then mutate mockHookReturn properties inside tests); update references to mockUseGenerateMaaSToken, mockGenerateToken, mockResetToken and any tests that reassign the variable so they instead call mockReturnValue or mutate the shared mockHookReturn object.packages/gen-ai/bff/internal/integrations/maas/maasmocks/maas_client_mock.go (1)
25-69: Consider settingModelTypeon existing LLM models for test coverage.The existing chat/instruct models (Llama-2-7B, Llama-2-13B, Mistral-7B, Granite-7B) don't set
ModelType. For comprehensive testing of the newmodel_typefiltering and labeling, consider addingModelType: "llm"to at least one or two of these models.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/bff/internal/integrations/maas/maasmocks/maas_client_mock.go` around lines 25 - 69, The mock MaaS model entries returned in the slice of models.MaaSModel are missing the ModelType field, so update at least one or two LLM entries (e.g., the items with ID "llama-2-7b-chat", "llama-2-13b-chat", "mistral-7b-instruct", or "granite-7b-lab") to include ModelType: "llm" so tests exercising model_type filtering/labeling hit LLM paths; modify the returned models in the maas_client_mock.go mock slice to add ModelType:"llm" for the chosen entries.packages/gen-ai/frontend/src/app/utilities/utils.ts (1)
139-142: Consider removing or gatingconsole.warnfor production.The warning for unknown model source types (line 141) is helpful during development but may pollute production logs if unexpected sources appear. Consider using a logging utility that can be disabled in production or removing the warning if the fallback behavior is sufficient.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/utilities/utils.ts` around lines 139 - 142, Replace the raw console.warn in packages/gen-ai/frontend/src/app/utilities/utils.ts (the block checking `if (!label)` that references `source` and `model.model_id`) with a gated or configurable logger: either call your app's logging utility (e.g., appLogger.warn or similar) so logs can be controlled centrally, or wrap the console warning behind an environment/dev check (e.g., only run when NODE_ENV !== "production" or when an isDev flag is true). Ensure the message still includes `source` and `model.model_id` for context.packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx (1)
158-158:ChatbotConfigurationModalreceivesaiModelsbut may expect separatemaasModelsprop.Per context snippet 3,
ChatbotConfigurationModaldefines bothaiModelsandmaasModelsas props and merges them internally (snippet 4, lines 55-64). PassingallModels(which already contains converted MaaS models) asaiModelswithout also settingmaasModels={[]}works becausemaasModelsdefaults to[], but the MaaS models will be re-converted viaconvertMaaSModelToAIModelif they appear inmaasModels.Since the unified
allModelsalready contains converted MaaS models, this is correct—but add a comment clarifying thatallModelsis pre-merged to prevent future confusion.Also applies to: 168-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx` at line 158, The ChatbotConfigurationModal is being passed aiModels={allModels} where allModels already contains MaaS models converted via convertMaaSModelToAIModel, so add an inline comment next to the aiModels prop call (and the similar occurrence at line 168) stating that allModels is pre-merged and contains converted MaaS models and that maasModels is intentionally omitted/left default to avoid double-conversion; optionally, to make intent explicit, pass maasModels={[]} alongside aiModels={allModels} so future readers/maintainers and the ChatbotConfigurationModal (which declares both aiModels and maasModels) cannot accidentally re-convert MaaS entries.packages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsx (1)
57-65: Sync effect may cause stale input on rapid filter type changes.The
useEffectsyncssearchValuefromfilterData[currentFilterType], but if a user types quickly and then switches filter types, the effect runs after render with the newcurrentFilterType, potentially overwriting input state. Consider adding a cleanup or debounce mechanism if this causes UX issues during testing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsx` around lines 57 - 65, The effect that syncs searchValue from filterData[currentFilterType] can overwrite a user’s rapid typing when currentFilterType changes; update the useEffect to avoid clobbering in-progress input by checking input focus or a recent edit timestamp before calling setSearchValue — e.g., introduce an inputFocused ref/state or a lastTypedAt timestamp updated by onChange and only apply the sync if the input is not focused or the filter change is newer/older than the last typed time; adjust the effect that references filterData, currentFilterType, isSelectFilter, and setSearchValue accordingly so it bails out when the user is actively typing (or add a short debounce) to prevent stale overwrites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client.go`:
- Around line 569-597: The namespace-backed model lists returned by
getAAModelsFromInferenceService and getAAModelsFromLLMInferenceService never set
the AAModel.ModelType, causing incomplete data after merging; update both
constructors to populate each models.AAModel's ModelType to the appropriate
namespace-backed constant (the same enum/value used by the external-model
branch) before returning their slices so the unified payload and Model Type
filter are correct.
In
`@packages/gen-ai/bff/internal/integrations/maas/maasmocks/maas_client_mock.go`:
- Around line 70-80: The mock model entry for "nomic-embed-text-v1" is missing
the ModelType field so embedding-specific code paths aren't exercised; update
the model literal in maas_client_mock.go (the map/array entry with ID
"nomic-embed-text-v1" and Usecase "Embedding, Semantic search") to include
ModelType: "embedding" so tests and model-type filtering/labeling logic hit the
embedding branch.
In `@packages/gen-ai/bff/openapi/src/gen-ai.yaml`:
- Around line 552-558: Update the OpenAPI schemas so model_type is mandatory in
both response models: add "model_type" to the required array of the MaaS item
schema and the AAModel schema (the schemas that currently declare a model_type
property with enum llm/embedding). Locate the schemas by name (MaaS item schema
and AAModel) and add "model_type" to their required lists so generated clients
treat it as non-optional and the contract prevents backend regressions.
In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx`:
- Line 36: The playground lookup and removal currently only compare
model.model_id (see playgroundModels.find and the filter at m.modelId !==
model.model_id), which can collide across sources; update logic to use a
composite key by including the model source (e.g., compare both m.modelId ===
model.model_id && m.source === modelSource or build a composite id) and ensure
the LlamaModel type includes a source field; change the enabledModel resolution
and the removal/filtering to check both model_id and source (or the composite
id) so selection and deletion are unambiguous.
In `@packages/gen-ai/frontend/src/app/utilities/__tests__/utils.spec.ts`:
- Around line 110-122: The test currently asserts convertMaaSModelToAIModel
returns model_type as undefined which codifies an old optional contract; remove
this test or move it into a new legacy-compat spec instead. Locate the test in
utilities/__tests__/utils.spec.ts referencing convertMaaSModelToAIModel and the
MaaSModel fixture and either delete this "should leave model_type undefined"
case or relocate it to a dedicated legacy test file; ensure remaining tests
assert the new contract (i.e., when BFF supplies model_type the converted AI
model has a defined model_type with the expected value).
---
Outside diff comments:
In `@packages/gen-ai/bff/openapi/src/gen-ai.yaml`:
- Around line 3981-3985: The example for external model endpoints in gen-ai.yaml
is using a bare URL but CreateExternalModel now returns endpoints with labeled
keys; update the endpoints entry under the external-model example to use the
labeled format (e.g., "external:
https://generativelanguage.googleapis.com/v1beta/openai/") so the example
matches the actual payload shape and CreateExternalModel behavior.
In `@packages/gen-ai/frontend/src/app/AIAssets/AIAssetsPage.tsx`:
- Around line 89-96: The current render of LazyCodeRefComponent using
extension.properties.component discards any children expected by
AIAssetsTabExtension (declared ComponentType<{ children?: React.ReactNode }>),
breaking wrapper-style extensions; either update this render to forward children
(render LazyCodeRefComponent with nested children from the tab extension so the
extension's composition hook runs) by passing extension.properties.children (or
using <LazyCodeRefComponent
...>{extension.properties.children}</LazyCodeRefComponent>), or tighten the
extension-point type/contract to remove children from AIAssetsTabExtension so
mismatched wrappers fail at compile time—pick one approach and apply it
consistently (update the call site in AIAssetsPage and/or the
AIAssetsTabExtension type definition).
---
Duplicate comments:
In `@packages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client.go`:
- Around line 584-593: The goroutine currently swallows all errors from
kc.GetAAModelsFromExternalModels by always returning nil; change it to only fall
back to aaModelsFromExternal = []models.AAModel{} when the error is the expected
"not found" case, and return the error for any other failure so callers can
handle it. Concretely, inside the g.Go closure that calls
kc.GetAAModelsFromExternalModels(gCtx, identity, namespace) check the error with
the appropriate Kubernetes helper (e.g., apierrors.IsNotFound(err) or the
sentinel returned by GetAAModelsFromExternalModels) and only on that path set
aaModelsFromExternal = []models.AAModel{} and log at Warn; for all other non-nil
errors return err from the goroutine instead of nil. Ensure you reference
aaModelsFromExternal, kc.GetAAModelsFromExternalModels, and kc.Logger.Warn when
making the change.
In `@packages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsx`:
- Around line 124-148: The Register external endpoint button and
CreateExternalEndpointModal are only rendered inside the branch gated by
models.length > 0, preventing namespaces with zero models from registering the
first external model; move the toolbarActions prop containing the Button (used
to call setIsCreateEndpointModalOpen(true)) and the CreateExternalEndpointModal
(using isCreateEndpointModalOpen, onClose ->
setIsCreateEndpointModalOpen(false), onSuccess -> handleCreationSuccess,
onSubmit -> handleCreateExternalEndpoint) out of the models.length check so they
render whenever isExternalModelsEnabled is true, and keep AIModelsTable,
isExternalModelsEnabled, setIsCreateEndpointModalOpen, handleCreationSuccess,
and handleCreateExternalEndpoint references intact.
---
Nitpick comments:
In
`@packages/gen-ai/bff/internal/integrations/maas/maasmocks/maas_client_mock.go`:
- Around line 25-69: The mock MaaS model entries returned in the slice of
models.MaaSModel are missing the ModelType field, so update at least one or two
LLM entries (e.g., the items with ID "llama-2-7b-chat", "llama-2-13b-chat",
"mistral-7b-instruct", or "granite-7b-lab") to include ModelType: "llm" so tests
exercising model_type filtering/labeling hit LLM paths; modify the returned
models in the maas_client_mock.go mock slice to add ModelType:"llm" for the
chosen entries.
In
`@packages/gen-ai/frontend/src/app/AIAssets/components/__tests__/EndpointDetailModal.spec.tsx`:
- Around line 17-23: Replace the current mutable reassign pattern for
mockUseGenerateMaaSToken with an idiomatic Jest per-test return strategy: keep
mockUseGenerateMaaSToken as a single jest.fn() and in each test call
mockUseGenerateMaaSToken.mockReturnValue(...) (or create a shared mutable object
like mockHookReturn and have the module mock return jest.fn(() =>
mockHookReturn) then mutate mockHookReturn properties inside tests); update
references to mockUseGenerateMaaSToken, mockGenerateToken, mockResetToken and
any tests that reassign the variable so they instead call mockReturnValue or
mutate the shared mockHookReturn object.
In `@packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx`:
- Line 158: The ChatbotConfigurationModal is being passed aiModels={allModels}
where allModels already contains MaaS models converted via
convertMaaSModelToAIModel, so add an inline comment next to the aiModels prop
call (and the similar occurrence at line 168) stating that allModels is
pre-merged and contains converted MaaS models and that maasModels is
intentionally omitted/left default to avoid double-conversion; optionally, to
make intent explicit, pass maasModels={[]} alongside aiModels={allModels} so
future readers/maintainers and the ChatbotConfigurationModal (which declares
both aiModels and maasModels) cannot accidentally re-convert MaaS entries.
In `@packages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsx`:
- Around line 57-65: The effect that syncs searchValue from
filterData[currentFilterType] can overwrite a user’s rapid typing when
currentFilterType changes; update the useEffect to avoid clobbering in-progress
input by checking input focus or a recent edit timestamp before calling
setSearchValue — e.g., introduce an inputFocused ref/state or a lastTypedAt
timestamp updated by onChange and only apply the sync if the input is not
focused or the filter change is newer/older than the last typed time; adjust the
effect that references filterData, currentFilterType, isSelectFilter, and
setSearchValue accordingly so it bails out when the user is actively typing (or
add a short debounce) to prevent stale overwrites.
In
`@packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationTableRow.tsx`:
- Around line 132-142: The component uses a magic string check sourceLabel !==
'Internal' which can drift from the canonical value in utilities; replace the
literal with an exported constant (or enum) from your utilities module (e.g.,
SOURCE_LABELS.namespace or a new SOURCE_LABEL_INTERNAL) and import it into
ChatbotConfigurationTableRow.tsx, then change the condition to compare against
that constant while keeping the rest of the rendering (including
getSourceLabelColor and data-testid) unchanged.
In `@packages/gen-ai/frontend/src/app/utilities/utils.ts`:
- Around line 139-142: Replace the raw console.warn in
packages/gen-ai/frontend/src/app/utilities/utils.ts (the block checking `if
(!label)` that references `source` and `model.model_id`) with a gated or
configurable logger: either call your app's logging utility (e.g.,
appLogger.warn or similar) so logs can be controlled centrally, or wrap the
console warning behind an environment/dev check (e.g., only run when NODE_ENV
!== "production" or when an isDev flag is true). Ensure the message still
includes `source` and `model.model_id` for context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 20d47cfe-071f-4b8f-acbe-fabad5edc9c3
📒 Files selected for processing (65)
packages/gen-ai/bff/README.mdpackages/gen-ai/bff/internal/api/external_models_handler_test.gopackages/gen-ai/bff/internal/api/maas_middleware_test.gopackages/gen-ai/bff/internal/api/maas_models_handler_test.gopackages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client.gopackages/gen-ai/bff/internal/integrations/maas/maasmocks/maas_client_mock.gopackages/gen-ai/bff/internal/models/aaa_models.gopackages/gen-ai/bff/internal/models/maas_models.gopackages/gen-ai/bff/internal/repositories/external_models.gopackages/gen-ai/bff/internal/repositories/maas_models.gopackages/gen-ai/bff/openapi/src/gen-ai.yamlpackages/gen-ai/frontend/config/moduleFederation.jspackages/gen-ai/frontend/src/__tests__/cypress/cypress/__mocks__/index.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/__mocks__/mockMaaSModels.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/pages/aiAssetsPage.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/pages/modelsTabPage.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.tspackages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/aiAssets/modelsTab.cy.tspackages/gen-ai/frontend/src/app/AIAssets/AIAssetsMaaSTab.tsxpackages/gen-ai/frontend/src/app/AIAssets/AIAssetsModelsTab.tsxpackages/gen-ai/frontend/src/app/AIAssets/AIAssetsPage.tsxpackages/gen-ai/frontend/src/app/AIAssets/__tests__/AIAssetsMaaSTab.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/__tests__/AIAssetsModelsTab.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTable.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTableRowEndpoint.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/EndpointDetailModal.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRow.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRowEndpoint.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTable.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTableRowInfo.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/ModelsListToolbar.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/TierInfoPopover.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelTableRow.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelsTable.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelsTableRowEndpoint.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/AIModelsTableRowEndpoint.tracking.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/EndpointDetailModal.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelTableRow.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelTableRowEndpoint.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelTableRowEndpoint.tracking.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelsTable.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/MaaSModelsTableRowInfo.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/ModelsListToolbar.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/components/__tests__/TierInfoPopover.spec.tsxpackages/gen-ai/frontend/src/app/AIAssets/data/columns.tspackages/gen-ai/frontend/src/app/AIAssets/data/columns.tsxpackages/gen-ai/frontend/src/app/AIAssets/data/filterOptions.tspackages/gen-ai/frontend/src/app/AIAssets/data/maasColumns.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useAIModelsFilter.spec.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/__tests__/useMaaSModelsFilter.spec.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/useAIModelsFilter.tspackages/gen-ai/frontend/src/app/AIAssets/hooks/useMaaSModelsFilter.tspackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationModal.tsxpackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationTableRow.tsxpackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationModal.spec.tsxpackages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/__tests__/ChatbotConfigurationTableRow.spec.tsxpackages/gen-ai/frontend/src/app/hooks/__tests__/useMergedModels.spec.tspackages/gen-ai/frontend/src/app/hooks/useFetchAIModels.tspackages/gen-ai/frontend/src/app/hooks/useMergedModels.tspackages/gen-ai/frontend/src/app/services/__tests__/llamaStackService.fixtures.tspackages/gen-ai/frontend/src/app/types.tspackages/gen-ai/frontend/src/app/utilities/__tests__/utils.spec.tspackages/gen-ai/frontend/src/app/utilities/routes.tspackages/gen-ai/frontend/src/app/utilities/utils.ts
💤 Files with no reviewable changes (22)
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTable.tsx
- packages/gen-ai/frontend/src/app/AIAssets/tests/AIAssetsMaaSTab.spec.tsx
- packages/gen-ai/frontend/config/moduleFederation.js
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelTableRowEndpoint.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelsTableRowInfo.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/AIModelsTableRowEndpoint.tracking.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelsTable.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/AIModelsTableRowEndpoint.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRow.tsx
- packages/gen-ai/frontend/src/app/AIAssets/hooks/tests/useMaaSModelsFilter.spec.ts
- packages/gen-ai/frontend/src/app/AIAssets/components/AIModelsTableRowEndpoint.tsx
- packages/gen-ai/frontend/src/app/AIAssets/AIAssetsMaaSTab.tsx
- packages/gen-ai/frontend/src/app/AIAssets/data/columns.ts
- packages/gen-ai/frontend/src/app/AIAssets/hooks/useMaaSModelsFilter.ts
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelTableRow.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/MaaSModelTableRowEndpoint.tracking.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelsTableRowInfo.tsx
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/TierInfoPopover.spec.tsx
- packages/gen-ai/frontend/src/app/AIAssets/data/maasColumns.ts
- packages/gen-ai/frontend/src/app/AIAssets/components/TierInfoPopover.tsx
- packages/gen-ai/bff/internal/repositories/maas_models.go
- packages/gen-ai/frontend/src/app/AIAssets/components/MaaSModelTableRowEndpoint.tsx
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/gen-ai/bff/internal/api/external_models_handler_test.go
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/AIModelsTable.spec.tsx
- packages/gen-ai/bff/internal/models/maas_models.go
- packages/gen-ai/frontend/src/tests/cypress/cypress/tests/mocked/aiAssets/modelsTab.cy.ts
- packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/tests/ChatbotConfigurationTableRow.spec.tsx
- packages/gen-ai/frontend/src/tests/cypress/cypress/support/helpers/modelsTab/modelsTabTestHelpers.ts
- packages/gen-ai/frontend/src/app/hooks/useFetchAIModels.ts
- packages/gen-ai/frontend/src/app/AIAssets/data/columns.tsx
- packages/gen-ai/frontend/src/tests/cypress/cypress/mocks/mockMaaSModels.ts
- packages/gen-ai/frontend/src/app/AIAssets/hooks/useAIModelsFilter.ts
- packages/gen-ai/bff/internal/api/maas_models_handler_test.go
- packages/gen-ai/frontend/src/app/types.ts
- packages/gen-ai/bff/internal/models/aaa_models.go
- packages/gen-ai/frontend/src/app/AIAssets/components/tests/ModelsListToolbar.spec.tsx
- packages/gen-ai/frontend/src/tests/cypress/cypress/mocks/index.ts
- packages/gen-ai/frontend/src/app/Chatbot/components/chatbotConfiguration/ChatbotConfigurationModal.tsx
- packages/gen-ai/frontend/src/tests/cypress/cypress/pages/modelsTabPage.ts
- packages/gen-ai/frontend/src/app/services/tests/llamaStackService.fixtures.ts
packages/gen-ai/bff/internal/integrations/kubernetes/token_k8s_client.go
Show resolved
Hide resolved
packages/gen-ai/bff/internal/integrations/maas/maasmocks/maas_client_mock.go
Show resolved
Hide resolved
packages/gen-ai/frontend/src/app/AIAssets/components/AIModelTableRow.tsx
Show resolved
Hide resolved
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
4 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.
Epic: RHOAIENG-52233
Spike: Design Doc
Demo:
Models.Page.Unification.mp4
Why is this PR large?
This PR consolidates the separate MaaS and AI Models tabs into a single unified Models table. The diff looks large (+2,413 / -3,508 across 66 files), but ~3,000 of those deleted lines are dead MaaS-specific components and their tests being removed (
MaaSModelsTable,MaaSModelTableRow,TierInfoPopover,useMaaSModelsFilter, etc.). The actual new logic is much smaller.To make it easier to review, the work is split into 3 sequential commits:
feat(gen-ai-bff)feat(gen-ai)test(gen-ai)What changed
BFF Backend (RHOAIENG-52234)
model_type(llm/embedding) on AAModel and MaaS model responseserrgroup(response time ~1.8s -> <1s)model_typeenumFrontend Data Layer (RHOAIENG-52236)
useMergedModelshook merging AI and MaaS models into a single listderiveModelType(),getSourceLabel(),getModelTypeLabel()Unified Models Tab UI (RHOAIENG-52237)
Endpoint Detail Modal (RHOAIENG-52240)
EndpointDetailModalreplacing separateAIModelsTableRowEndpointandMaaSModelTableRowEndpointpopoversCypress Mocked Tests (RHOAIENG-52241)
Summary by CodeRabbit
Release Notes
New Features
Improvements