Feat(RHOAIENG-51945): View subscription page#6909
Feat(RHOAIENG-51945): View subscription page#6909claudialphonse78 wants to merge 4 commits intoopendatahub-io:mainfrom
Conversation
|
[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 |
📝 WalkthroughWalkthroughThis PR introduces a view subscription details feature across the Cypress test framework, Go backend, and TypeScript frontend. Changes include: new Cypress page object and test suite for subscription views, backend model structs extended with Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Security & Architecture Issues1. Insufficient Input Validation on Route Parameter Action: Add explicit validation: 2. Unbounded String Fields in Backend Models Action: Add 3. Incomplete Response Validation in Action: Add explicit array-item validation: iterate 4. Missing Authorization Context in Frontend Action: Consider adding frontend capability checks (e.g., 5. Format String-like Risk in Action: Add a guard: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
claudialphonse78
requested review from
Griffin-Sullivan,
dpanshug and
katieperry4
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
packages/maas/frontend/src/app/pages/subscriptions/viewSubscription/__tests__/utils.spec.ts (1)
24-27: Tests assume US locale formatting.These assertions expect comma-separated thousands (
'5,000'), which is US locale-specific. If CI runs with a different locale, tests will fail. This couples to the implementation issue noted inutils.ts.If the implementation is updated to use explicit locale (
'en-US'), no test changes needed. Alternatively, mocktoLocaleStringor test against a regex pattern:Alternative approach if explicit locale isn't used
- expect(formatTokenLimits(refs, 'model-a')).toBe('5,000 / 1h'); + expect(formatTokenLimits(refs, 'model-a')).toMatch(/^5[,.\s]?000 \/ 1h$/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/maas/frontend/src/app/pages/subscriptions/viewSubscription/__tests__/utils.spec.ts` around lines 24 - 27, The test fails under non-US locales because formatTokenLimits (used in the test with makeRef and formatTokenLimits) relies on toLocaleString without a locale; update the implementation in utils.ts so the number formatting is deterministic by calling toLocaleString('en-US') (or an explicit locale) when producing the token amount string, or alternatively change this test to assert a locale-independent format (e.g., use a regex or compare the numeric value plus window) — reference the formatTokenLimits function and the test using makeRef('model-a', ...) to locate where to apply the fix.packages/maas/bff/internal/api/subscription_handlers_test.go (1)
240-245: Type assertions provide weak validation.
BeAssignableToTypeOf("")only verifies the field is a string type, not that it contains meaningful data. Since these are new API contract fields populated from annotations, consider asserting expected values or at minimumNot(BeEmpty())if annotations are set in test fixtures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/maas/bff/internal/api/subscription_handlers_test.go` around lines 240 - 245, Replace the weak type-only assertions for subscription and model display/description fields with meaningful content checks: update the assertions on actual.Subscription.DisplayName, actual.Subscription.Description, actual.ModelRefs[0].DisplayName, and actual.ModelRefs[0].Description to assert expected values (if your test fixtures set specific annotation values) or at minimum use Not(BeEmpty()) to ensure they contain data rather than just being a string type; keep the existing assertions for ModelRefs length and Name intact (e.g., leave Expect(actual.ModelRefs).To(HaveLen(1)) and Expect(actual.ModelRefs[0].Name).To(Equal("granite-3-8b-instruct")) as-is).packages/maas/frontend/src/app/pages/subscriptions/viewSubscription/utils.ts (1)
8-9: Locale-dependent formatting may cause test flakiness.
limit.toLocaleString()uses the runtime's default locale. In CI environments with different locale settings, this could produce"5.000"(German),"5 000"(French), or"5,000"(US). Tests assume US formatting.Consider using explicit locale:
Proposed fix
- return `${limit.toLocaleString()} / ${window}`; + return `${limit.toLocaleString('en-US')} / ${window}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/maas/frontend/src/app/pages/subscriptions/viewSubscription/utils.ts` around lines 8 - 9, The current formatting uses limit.toLocaleString() which depends on the runtime locale and can make tests flaky; update the usage in the token rate limits rendering (where limit is read from ref.tokenRateLimits[0]) to format explicitly with a stable locale—e.g., replace limit.toLocaleString() with an explicit 'en-US' formatting call or use Intl.NumberFormat('en-US').format(limit) so the returned string is deterministic across environments.packages/maas/frontend/src/app/hooks/__tests__/useGetSubscriptionInfo.spec.ts (1)
56-63: Pending promise pattern is correct but could be clearer.
new Promise(jest.fn())works becausejest.fn()as executor never calls resolve/reject. However, this is non-obvious. Consider a clearer alternative:More explicit pending promise
- mockGetSubscriptionInfo.mockReturnValue(() => new Promise(jest.fn())); + mockGetSubscriptionInfo.mockReturnValue(() => new Promise(() => { /* never resolves */ }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/maas/frontend/src/app/hooks/__tests__/useGetSubscriptionInfo.spec.ts` around lines 56 - 63, The test uses new Promise(jest.fn()) to create a pending promise which is unclear; update the mocked pending promise in the test for useGetSubscriptionInfo to be explicitly pending (e.g. return new Promise(() => {}) or a named pendingPromise helper) when setting mockGetSubscriptionInfo.mockReturnValue so the intent is obvious to readers and the rest of the test (testHook/useGetSubscriptionInfo/standardUseFetchState assertions) remains unchanged.packages/maas/frontend/src/app/hooks/useGetSubscriptionInfo.ts (1)
12-23: Type annotation mismatch with API contract.
getSubscriptionInforeturnsPromise<SubscriptionInfoResponse>(throws on failure, never returnsnull). The| nullunion inFetchStateCallbackPromise<SubscriptionInfoResponse | null>(line 15) is technically inaccurate for the callback's promise type—nullonly represents the initial state before the first fetch.Consider using
FetchStateCallbackPromise<SubscriptionInfoResponse>for the callback and keepingnullonly in the hook's return type to reflect the actual contract:Suggested fix
export const useGetSubscriptionInfo = ( name: string, ): FetchState<SubscriptionInfoResponse | null> => { - const callback = React.useCallback<FetchStateCallbackPromise<SubscriptionInfoResponse | null>>( + const callback = React.useCallback<FetchStateCallbackPromise<SubscriptionInfoResponse>>( (opts: APIOptions) => getSubscriptionInfo(name)(opts), [name], ); - return useFetchState<SubscriptionInfoResponse | null>(callback, null, { + return useFetchState(callback, null, { refreshRate: POLL_INTERVAL, }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/maas/frontend/src/app/hooks/useGetSubscriptionInfo.ts` around lines 12 - 23, The callback's promise type is overly broad—getSubscriptionInfo returns Promise<SubscriptionInfoResponse> (never null), so update the callback annotation from FetchStateCallbackPromise<SubscriptionInfoResponse | null> to FetchStateCallbackPromise<SubscriptionInfoResponse> in useGetSubscriptionInfo (the React.useCallback that calls getSubscriptionInfo(name)), while keeping the hook return type useFetchState<SubscriptionInfoResponse | null>(...) so the initial state can be null; adjust only the type parameter on the callback signature (referencing useGetSubscriptionInfo, getSubscriptionInfo, FetchStateCallbackPromise, useFetchState, and SubscriptionInfoResponse).packages/maas/frontend/src/app/pages/subscriptions/viewSubscription/SubscriptionDetailsSection.tsx (1)
46-51: Potential "Invalid Date" display if timestamp is malformed.
new Date(subscription.creationTimestamp)will produce an invalidDateobject if the string is empty or malformed, causing theTimestampcomponent to render "Invalid Date". Consider defensive handling:Optional defensive fix
<DescriptionListGroup> <DescriptionListTerm>Date created</DescriptionListTerm> <DescriptionListDescription> - <Timestamp - date={new Date(subscription.creationTimestamp)} - dateFormat="long" - timeFormat="short" - is12Hour - /> + {subscription.creationTimestamp ? ( + <Timestamp + date={new Date(subscription.creationTimestamp)} + dateFormat="long" + timeFormat="short" + is12Hour + /> + ) : ( + '—' + )} </DescriptionListDescription> </DescriptionListGroup>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/maas/frontend/src/app/pages/subscriptions/viewSubscription/SubscriptionDetailsSection.tsx` around lines 46 - 51, SubscriptionDetailsSection is passing new Date(subscription.creationTimestamp) directly to the Timestamp component which will render "Invalid Date" if the string is empty or malformed; instead validate the timestamp before rendering: parse subscription.creationTimestamp into a Date, check its validity (e.g., !isNaN(date.getTime())), and if valid pass that Date to the Timestamp, otherwise render a fallback (e.g., a placeholder string or nothing) or pass undefined to Timestamp so it shows a safe fallback; update the JSX around the Timestamp usage in SubscriptionDetailsSection.tsx to perform this guard using the subscription.creationTimestamp value.
🤖 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/maas/bff/internal/api/subscription_handlers_test.go`:
- Around line 249-252: The test loop over actual.AuthPolicies is a no-op when no
auth policies were created; update the test setup in the BeforeAll block that
creates the subscription to include CreateAuthPolicy: true (so AuthPolicies is
populated) or add an explicit assertion before the loop such as
Expect(len(actual.AuthPolicies)).To(Equal(expectedCount)) to assert the slice is
non-empty, then keep the existing Kind checks on actual.AuthPolicies; refer to
the BeforeAll subscription creation and the actual.AuthPolicies usage to apply
the change.
In `@packages/maas/frontend/src/app/api/subscriptions.ts`:
- Around line 58-64: The type guard isMaaSModelRefSummary is missing validation
for optional displayName and description fields; update isMaaSModelRefSummary to
also check (v.displayName === undefined || typeof v.displayName === 'string')
and (v.description === undefined || typeof v.description === 'string'),
mirroring the checks present in isMaaSSubscription so the shape matches the
MaaSModelRefSummary type.
In
`@packages/maas/frontend/src/app/pages/subscriptions/viewSubscription/SubscriptionModelsSection.tsx`:
- Around line 86-89: formatTokenLimits currently looks up quotas using only
modelRef.name and rows are keyed by name, which causes collisions for models
with the same name in different namespaces; update callers to pass both name and
namespace (pass modelRef.namespace into formatTokenLimits) and change
formatTokenLimits to use the composite identifier (namespace + name) when
searching subscriptionModelRefs, and also update the row key generation (where
rows are rendered) to use a composite key like
`${modelRef.namespace}:${modelRef.name}` instead of modelRef.name so lookups and
React reconciliation are namespace-safe; refer to formatTokenLimits,
subscriptionModelRefs, and modelRef.name/modelRef.namespace to locate changes.
---
Nitpick comments:
In `@packages/maas/bff/internal/api/subscription_handlers_test.go`:
- Around line 240-245: Replace the weak type-only assertions for subscription
and model display/description fields with meaningful content checks: update the
assertions on actual.Subscription.DisplayName, actual.Subscription.Description,
actual.ModelRefs[0].DisplayName, and actual.ModelRefs[0].Description to assert
expected values (if your test fixtures set specific annotation values) or at
minimum use Not(BeEmpty()) to ensure they contain data rather than just being a
string type; keep the existing assertions for ModelRefs length and Name intact
(e.g., leave Expect(actual.ModelRefs).To(HaveLen(1)) and
Expect(actual.ModelRefs[0].Name).To(Equal("granite-3-8b-instruct")) as-is).
In
`@packages/maas/frontend/src/app/hooks/__tests__/useGetSubscriptionInfo.spec.ts`:
- Around line 56-63: The test uses new Promise(jest.fn()) to create a pending
promise which is unclear; update the mocked pending promise in the test for
useGetSubscriptionInfo to be explicitly pending (e.g. return new Promise(() =>
{}) or a named pendingPromise helper) when setting
mockGetSubscriptionInfo.mockReturnValue so the intent is obvious to readers and
the rest of the test (testHook/useGetSubscriptionInfo/standardUseFetchState
assertions) remains unchanged.
In `@packages/maas/frontend/src/app/hooks/useGetSubscriptionInfo.ts`:
- Around line 12-23: The callback's promise type is overly
broad—getSubscriptionInfo returns Promise<SubscriptionInfoResponse> (never
null), so update the callback annotation from
FetchStateCallbackPromise<SubscriptionInfoResponse | null> to
FetchStateCallbackPromise<SubscriptionInfoResponse> in useGetSubscriptionInfo
(the React.useCallback that calls getSubscriptionInfo(name)), while keeping the
hook return type useFetchState<SubscriptionInfoResponse | null>(...) so the
initial state can be null; adjust only the type parameter on the callback
signature (referencing useGetSubscriptionInfo, getSubscriptionInfo,
FetchStateCallbackPromise, useFetchState, and SubscriptionInfoResponse).
In
`@packages/maas/frontend/src/app/pages/subscriptions/viewSubscription/__tests__/utils.spec.ts`:
- Around line 24-27: The test fails under non-US locales because
formatTokenLimits (used in the test with makeRef and formatTokenLimits) relies
on toLocaleString without a locale; update the implementation in utils.ts so the
number formatting is deterministic by calling toLocaleString('en-US') (or an
explicit locale) when producing the token amount string, or alternatively change
this test to assert a locale-independent format (e.g., use a regex or compare
the numeric value plus window) — reference the formatTokenLimits function and
the test using makeRef('model-a', ...) to locate where to apply the fix.
In
`@packages/maas/frontend/src/app/pages/subscriptions/viewSubscription/SubscriptionDetailsSection.tsx`:
- Around line 46-51: SubscriptionDetailsSection is passing new
Date(subscription.creationTimestamp) directly to the Timestamp component which
will render "Invalid Date" if the string is empty or malformed; instead validate
the timestamp before rendering: parse subscription.creationTimestamp into a
Date, check its validity (e.g., !isNaN(date.getTime())), and if valid pass that
Date to the Timestamp, otherwise render a fallback (e.g., a placeholder string
or nothing) or pass undefined to Timestamp so it shows a safe fallback; update
the JSX around the Timestamp usage in SubscriptionDetailsSection.tsx to perform
this guard using the subscription.creationTimestamp value.
In
`@packages/maas/frontend/src/app/pages/subscriptions/viewSubscription/utils.ts`:
- Around line 8-9: The current formatting uses limit.toLocaleString() which
depends on the runtime locale and can make tests flaky; update the usage in the
token rate limits rendering (where limit is read from ref.tokenRateLimits[0]) to
format explicitly with a stable locale—e.g., replace limit.toLocaleString() with
an explicit 'en-US' formatting call or use
Intl.NumberFormat('en-US').format(limit) so the returned string is deterministic
across environments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e290c80c-2605-452f-a106-b1b13fec78f0
📒 Files selected for processing (21)
packages/cypress/cypress/pages/modelsAsAService.tspackages/cypress/cypress/support/commands/odh.tspackages/cypress/cypress/tests/mocked/modelsAsAService/maasSubscriptions.cy.tspackages/cypress/cypress/utils/maasUtils.tspackages/maas/bff/internal/api/subscription_handlers_test.gopackages/maas/bff/internal/mocks/subscription_mock.gopackages/maas/bff/internal/models/subscription.gopackages/maas/bff/internal/repositories/subscriptions.gopackages/maas/bff/openapi.yamlpackages/maas/frontend/src/app/api/__tests__/subscriptions.spec.tspackages/maas/frontend/src/app/api/subscriptions.tspackages/maas/frontend/src/app/hooks/__tests__/useGetSubscriptionInfo.spec.tspackages/maas/frontend/src/app/hooks/useGetSubscriptionInfo.tspackages/maas/frontend/src/app/pages/subscriptions/ViewSubscriptionPage.tsxpackages/maas/frontend/src/app/pages/subscriptions/viewSubscription/SubscriptionDetailsSection.tsxpackages/maas/frontend/src/app/pages/subscriptions/viewSubscription/SubscriptionGroupsSection.tsxpackages/maas/frontend/src/app/pages/subscriptions/viewSubscription/SubscriptionModelsSection.tsxpackages/maas/frontend/src/app/pages/subscriptions/viewSubscription/SubscriptionPoliciesSection.tsxpackages/maas/frontend/src/app/pages/subscriptions/viewSubscription/__tests__/utils.spec.tspackages/maas/frontend/src/app/pages/subscriptions/viewSubscription/utils.tspackages/maas/frontend/src/app/types/subscriptions.ts
| // Kind is always populated from the GVR when a policy exists | ||
| for _, policy := range actual.AuthPolicies { | ||
| Expect(policy.Kind).To(Equal("MaaSAuthPolicy")) | ||
| } |
There was a problem hiding this comment.
Test may be a no-op if no auth policy was created.
The BeforeAll block at Line 201 creates a subscription without CreateAuthPolicy: true. The loop over actual.AuthPolicies will not execute any assertions if the slice is empty. Either add CreateAuthPolicy: true to the setup or add an explicit assertion that AuthPolicies has expected length.
Proposed fix
// Auth policies may be empty if not created with the subscription
- Expect(actual.AuthPolicies).NotTo(BeNil())
+ // For this test, we expect no auth policies since CreateAuthPolicy was not set
+ Expect(actual.AuthPolicies).To(BeEmpty())Or if you want to test with policies:
BeforeAll(func() {
_, rs, err := setupApiTest[models.CreateSubscriptionResponse](
// ...
- Priority: 5,
+ Priority: 5,
+ CreateAuthPolicy: true,
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/maas/bff/internal/api/subscription_handlers_test.go` around lines
249 - 252, The test loop over actual.AuthPolicies is a no-op when no auth
policies were created; update the test setup in the BeforeAll block that creates
the subscription to include CreateAuthPolicy: true (so AuthPolicies is
populated) or add an explicit assertion before the loop such as
Expect(len(actual.AuthPolicies)).To(Equal(expectedCount)) to assert the slice is
non-empty, then keep the existing Kind checks on actual.AuthPolicies; refer to
the BeforeAll subscription creation and the actual.AuthPolicies usage to apply
the change.
| const isMaaSModelRefSummary = (v: unknown): v is MaaSModelRefSummary => | ||
| isRecord(v) && | ||
| typeof v.name === 'string' && | ||
| typeof v.namespace === 'string' && | ||
| isModelReference(v.modelRef) && | ||
| (v.phase === undefined || typeof v.phase === 'string') && | ||
| (v.endpoint === undefined || typeof v.endpoint === 'string'); |
There was a problem hiding this comment.
Missing validation for displayName and description in isMaaSModelRefSummary.
The type guard validates name, namespace, modelRef, phase, and endpoint, but omits validation for optional displayName and description fields that are part of MaaSModelRefSummary. This is inconsistent with isMaaSSubscription (lines 39-40) which validates these same optional fields.
Suggested fix
const isMaaSModelRefSummary = (v: unknown): v is MaaSModelRefSummary =>
isRecord(v) &&
typeof v.name === 'string' &&
typeof v.namespace === 'string' &&
+ (v.displayName === undefined || typeof v.displayName === 'string') &&
+ (v.description === undefined || typeof v.description === 'string') &&
isModelReference(v.modelRef) &&
(v.phase === undefined || typeof v.phase === 'string') &&
(v.endpoint === undefined || typeof v.endpoint === 'string');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/maas/frontend/src/app/api/subscriptions.ts` around lines 58 - 64,
The type guard isMaaSModelRefSummary is missing validation for optional
displayName and description fields; update isMaaSModelRefSummary to also check
(v.displayName === undefined || typeof v.displayName === 'string') and
(v.description === undefined || typeof v.description === 'string'), mirroring
the checks present in isMaaSSubscription so the shape matches the
MaaSModelRefSummary type.
| tokenLimits: ( | ||
| <Td key="tokenLimits" dataLabel="Token limits"> | ||
| {formatTokenLimits(subscriptionModelRefs, modelRef.name)} | ||
| </Td> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the SubscriptionModelsSection.tsx file
fd -t f "SubscriptionModelsSection.tsx" packages/maas/frontend/srcRepository: opendatahub-io/odh-dashboard
Length of output: 169
🏁 Script executed:
# Read the file with context around the flagged lines
cat -n packages/maas/frontend/src/app/pages/subscriptions/viewSubscription/SubscriptionModelsSection.tsxRepository: opendatahub-io/odh-dashboard
Length of output: 4901
🏁 Script executed:
# Also search for the formatTokenLimits function definition
rg -n "formatTokenLimits" packages/maas/frontend/src --type ts --type tsx -A 10Repository: opendatahub-io/odh-dashboard
Length of output: 99
🏁 Script executed:
# Check type definitions for MaaSModelRefSummary and ModelSubscriptionRef
fd -t f "\.ts$\|\.tsx$" packages/maas/frontend/src | xargs rg -l "MaaSModelRefSummary|ModelSubscriptionRef" | head -5Repository: opendatahub-io/odh-dashboard
Length of output: 343
🏁 Script executed:
# Fix the rg search with proper flags
rg "formatTokenLimits" packages/maas/frontend/src -A 10 --type ts --type jsRepository: opendatahub-io/odh-dashboard
Length of output: 10441
🏁 Script executed:
# Look for the utils file with formatTokenLimits definition
fd "utils" packages/maas/frontend/src --type f -name "*.ts" -o -name "*.tsx" | xargs rg -l "formatTokenLimits" 2>/dev/null || fd "formatTokenLimits" packages/maas/frontend/srcRepository: opendatahub-io/odh-dashboard
Length of output: 586
🏁 Script executed:
# Search for type definitions
fd -e ts -e tsx packages/maas/frontend/src | xargs rg "type MaaSModelRefSummary|type ModelSubscriptionRef|interface MaaSModelRefSummary|interface ModelSubscriptionRef" -A 5 2>/dev/nullRepository: opendatahub-io/odh-dashboard
Length of output: 2713
🏁 Script executed:
# More general search for formatTokenLimits
rg "export.*formatTokenLimits|function formatTokenLimits" packages/maas/frontend/srcRepository: opendatahub-io/odh-dashboard
Length of output: 249
🏁 Script executed:
# Read the utils.spec.ts test file to see if tests would need updates for the fix
cat -n packages/maas/frontend/src/app/pages/subscriptions/viewSubscription/__tests__/utils.spec.tsRepository: opendatahub-io/odh-dashboard
Length of output: 2207
🏁 Script executed:
# Check if there are any other places where formatTokenLimits is called
rg "formatTokenLimits" packages/maas/frontend/src --type ts --type jsRepository: opendatahub-io/odh-dashboard
Length of output: 1959
Pass namespace to formatTokenLimits and use composite row key.
Line 88 resolves token limits by modelRef.name only; the function then searches subscriptionModelRefs using only name. Line 99 keys rows by name alone. Since both types are namespaced, two models with identical names in different namespaces will collide during lookup (returning the first match's quota) and share the same React key, breaking reconciliation.
Fix
- <Td key="tokenLimits" dataLabel="Token limits">
- {formatTokenLimits(subscriptionModelRefs, modelRef.name)}
- </Td>
+ <Td key="tokenLimits" dataLabel="Token limits">
+ {formatTokenLimits(
+ subscriptionModelRefs,
+ modelRef.name,
+ modelRef.namespace,
+ )}
+ </Td>- <Tr key={modelRef.name}>{visibleColumns.map((col) => cellRenderers[col.key])}</Tr>
+ <Tr key={`${modelRef.namespace}/${modelRef.name}`}>
+ {visibleColumns.map((col) => cellRenderers[col.key])}
+ </Tr>Update utils.ts:
export const formatTokenLimits = (
modelRefs: ModelSubscriptionRef[],
modelName: string,
+ modelNamespace: string,
): string => {
const ref = modelRefs.find(
- (r) => r.name === modelName,
+ (r) => r.name === modelName && r.namespace === modelNamespace,
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/maas/frontend/src/app/pages/subscriptions/viewSubscription/SubscriptionModelsSection.tsx`
around lines 86 - 89, formatTokenLimits currently looks up quotas using only
modelRef.name and rows are keyed by name, which causes collisions for models
with the same name in different namespaces; update callers to pass both name and
namespace (pass modelRef.namespace into formatTokenLimits) and change
formatTokenLimits to use the composite identifier (namespace + name) when
searching subscriptionModelRefs, and also update the row key generation (where
rows are rendered) to use a composite key like
`${modelRef.namespace}:${modelRef.name}` instead of modelRef.name so lookups and
React reconciliation are namespace-safe; refer to formatTokenLimits,
subscriptionModelRefs, and modelRef.name/modelRef.namespace to locate changes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6909 +/- ##
==========================================
+ Coverage 64.09% 64.25% +0.15%
==========================================
Files 2530 2541 +11
Lines 76695 77186 +491
Branches 19202 19284 +82
==========================================
+ Hits 49161 49596 +435
- Misses 27534 27590 +56
... and 41 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Closes RHOAIENG-51945
Description
Subscription_page_mock_mode.mp4
Details: Subscription Page in Mock mode
Subscription_federated_mode.mp4
Details: Subscription Page in Federated mode
This PR includes the following changes:
BFF:
DisplayNameandDescriptionfields toMaaSSubscriptionandMaaSModelRefSummary, read fromopenshift.io/display-nameandopenshift.io/descriptionK8s annotationsKindfield toMaaSAuthPolicy, populated fromobj.GetKind()Frontend:
ViewSubscriptionPagewith breadcrumb navigation, kebab actions menu(Edit / Delete)hideColumnsprop for reuse( for component reuse in the Create API Key Modal)kindfalls back to—when absentuseGetSubscriptionInfohook with polling viaPOLL_INTERVALHow Has This Been Tested?
npm run devfrom the repo rootmake dev-start-mock-federatedinpackages/maasto start with mock data or Runmake dev-start-federatedinpackages/maasin federated modeTest Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit
Release Notes
New Features