Update the subscription view page title to use display name#7155
Conversation
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughMock subscription objects now include a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Security reviewIssue: Unvalidated rendering of
Issue: Fallback to route parameter
No other actionable security issues are evident in the diff. 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cypress/cypress/tests/mocked/modelsAsAService/maasSubscriptions.cy.ts (1)
127-127: Add a test that exercises the missing-displayNamefallback path.These assertions cover only the display-name-present path. Add one case where
displayNameis absent/blank and assert the view title/details fall back tosubscriptionName(premium-team-sub).Proposed test addition
describe('View Subscription Page', () => { const subscriptionName = 'premium-team-sub'; @@ it('should display the page content with title, breadcrumb, details, groups, and models', () => { @@ }); + it('should fall back to resource name when displayName is missing', () => { + const response = mockSubscriptionInfo(subscriptionName); + response.subscription = { ...response.subscription, displayName: undefined }; + + cy.interceptOdh( + 'GET /maas/api/v1/subscription-info/:name', + { path: { name: subscriptionName } }, + { data: response }, + ); + + viewSubscriptionPage.visit(subscriptionName); + viewSubscriptionPage.findTitle().should('contain.text', subscriptionName); + viewSubscriptionPage + .findDetailsSection() + .should('contain.text', 'Resource name') + .and('contain.text', subscriptionName); + });Also applies to: 130-130, 134-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/mocked/modelsAsAService/maasSubscriptions.cy.ts` at line 127, Add a new test case in maasSubscriptions.cy.ts that covers the missing displayName fallback: create or mock a subscription record with displayName empty/undefined and subscriptionName 'premium-team-sub', then trigger the same UI flow used by subscriptionsPage.getRow('Premium Team Subscription').findKebabAction('View details').click() (and the related assertions at the other occurrences) and assert that the view title and details fall back to subscriptionName 'premium-team-sub' instead of displayName; update the selector usage (subscriptionsPage.getRow(...).findKebabAction('View details')) to target the row for the subscriptionName variant and add assertions verifying the displayed title equals 'premium-team-sub'.
🤖 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/frontend/src/app/pages/subscriptions/ViewSubscriptionPage.tsx`:
- Around line 74-76: Breadcrumb and title currently use the nullish coalescing
operator so an empty string in subscriptionInfo.subscription.displayName renders
as blank; update both occurrences in ViewSubscriptionPage (the BreadcrumbItem
and the title usage) to treat empty/whitespace-only displayName as missing by
trimming the string and falling back to subscriptionName (i.e., replace the
subscriptionInfo?.subscription.displayName ?? subscriptionName usage with a
trimmed-or-fallback check so "" or " " falls back to subscriptionName).
---
Nitpick comments:
In
`@packages/cypress/cypress/tests/mocked/modelsAsAService/maasSubscriptions.cy.ts`:
- Line 127: Add a new test case in maasSubscriptions.cy.ts that covers the
missing displayName fallback: create or mock a subscription record with
displayName empty/undefined and subscriptionName 'premium-team-sub', then
trigger the same UI flow used by subscriptionsPage.getRow('Premium Team
Subscription').findKebabAction('View details').click() (and the related
assertions at the other occurrences) and assert that the view title and details
fall back to subscriptionName 'premium-team-sub' instead of displayName; update
the selector usage (subscriptionsPage.getRow(...).findKebabAction('View
details')) to target the row for the subscriptionName variant and add assertions
verifying the displayed title equals 'premium-team-sub'.
🪄 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: a48538e0-64cf-4631-aebe-2bb42cd89a12
📒 Files selected for processing (4)
packages/cypress/cypress/tests/mocked/modelsAsAService/maasSubscriptions.cy.tspackages/cypress/cypress/utils/maasUtils.tspackages/maas/frontend/src/app/pages/subscriptions/ViewSubscriptionPage.tsxpackages/maas/frontend/src/app/pages/subscriptions/viewSubscription/SubscriptionDetailsSection.tsx
katieperry4
force-pushed
the
57319/subscription-view-title-bug
branch
from
6b67ad2 to
195173d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7155 +/- ##
==========================================
+ Coverage 64.80% 64.91% +0.10%
==========================================
Files 2441 2442 +1
Lines 75996 76016 +20
Branches 19158 19165 +7
==========================================
+ Hits 49253 49347 +94
+ Misses 26743 26669 -74
... and 20 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
katieperry4
requested review from
Griffin-Sullivan,
ashley-o0o,
claudialphonse78 and
dpanshug
and removed request for
ConorOM1
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/maas/frontend/src/app/pages/subscriptions/ViewSubscriptionPage.tsx (1)
66-67:⚠️ Potential issue | 🟡 Minor
displaySubscriptionNamestill does not fallback for blank values.Line [67] uses
??; whendisplayName?.trim()is'', breadcrumb/title remain blank.Suggested fix
- const displaySubscriptionName = - subscriptionInfo?.subscription.displayName?.trim() ?? subscriptionName; + const displaySubscriptionName = + subscriptionInfo?.subscription.displayName?.trim() || subscriptionName;#!/bin/bash # Verify the problematic operator pattern in the updated file. rg -nP "displayName\?\.\s*trim\(\)\s*\?\?" packages/maas/frontend/src/app/pages/subscriptions/ViewSubscriptionPage.tsx # Check whether fallback edge cases (blank/whitespace displayName) are covered in Cypress mocks/tests. rg -nP "displayName:\s*''|displayName:\s*'\\s+'" packages/cypress --glob '*.ts'🤖 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/ViewSubscriptionPage.tsx` around lines 66 - 67, Replace the nullish-coalescing fallback with a falsy fallback so blank/whitespace display names fall back to the subscriptionName: change the expression constructing displaySubscriptionName in ViewSubscriptionPage (the symbol displaySubscriptionName using subscriptionInfo?.subscription.displayName?.trim()) to use || (or explicitly coerce to string then .trim() and ||) instead of ??; also update/add Cypress mocks/tests that assert behavior for displayName: '' and displayName: ' ' to verify the fallback works.
🧹 Nitpick comments (1)
packages/cypress/cypress/utils/maasUtils.ts (1)
80-126: Add a fixture for missing/blankdisplayNameto cover fallback behavior.Current
mockSubscriptions()data only exercises the display-name-present path. Add one mock subscription with omitted (or whitespace-only)displayNameso tests can assert fallback toname.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/utils/maasUtils.ts` around lines 80 - 126, The mockSubscriptions() fixture only contains subscriptions with non-empty displayName; add an additional MaaSSubscription object to the returned array with displayName omitted or set to a whitespace-only string so tests exercise the fallback to name. Update the mockSubscriptions() return to include a subscription (e.g., name 'no-display-sub') with displayName: '' or absent, include other required fields (namespace, phase, owner, modelRefs, creationTimestamp) so consumers of mockSubscriptions() can assert that code paths in displayName fallback logic use the name field. Ensure the new entry follows the existing shape used by tests that consume mockSubscriptions().
🤖 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/frontend/src/app/pages/subscriptions/viewSubscription/SubscriptionDetailsSection.tsx`:
- Around line 30-33: In SubscriptionDetailsSection where the Name field
currently renders {subscription.displayName ?? subscription.name}, change the
fallback logic to treat empty or whitespace-only displayName as missing: check
subscription.displayName after trimming (e.g., if subscription.displayName &&
subscription.displayName.trim().length > 0) and render that; otherwise render
subscription.name. Update the JSX inside DescriptionListDescription to use this
trimmed check so empty strings or whitespace fall back to subscription.name.
---
Duplicate comments:
In `@packages/maas/frontend/src/app/pages/subscriptions/ViewSubscriptionPage.tsx`:
- Around line 66-67: Replace the nullish-coalescing fallback with a falsy
fallback so blank/whitespace display names fall back to the subscriptionName:
change the expression constructing displaySubscriptionName in
ViewSubscriptionPage (the symbol displaySubscriptionName using
subscriptionInfo?.subscription.displayName?.trim()) to use || (or explicitly
coerce to string then .trim() and ||) instead of ??; also update/add Cypress
mocks/tests that assert behavior for displayName: '' and displayName: ' ' to
verify the fallback works.
---
Nitpick comments:
In `@packages/cypress/cypress/utils/maasUtils.ts`:
- Around line 80-126: The mockSubscriptions() fixture only contains
subscriptions with non-empty displayName; add an additional MaaSSubscription
object to the returned array with displayName omitted or set to a
whitespace-only string so tests exercise the fallback to name. Update the
mockSubscriptions() return to include a subscription (e.g., name
'no-display-sub') with displayName: '' or absent, include other required fields
(namespace, phase, owner, modelRefs, creationTimestamp) so consumers of
mockSubscriptions() can assert that code paths in displayName fallback logic use
the name field. Ensure the new entry follows the existing shape used by tests
that consume mockSubscriptions().
🪄 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: 365d85fb-d7c2-4cd7-ba16-c2efd8146734
📒 Files selected for processing (4)
packages/cypress/cypress/tests/mocked/modelsAsAService/maasSubscriptions.cy.tspackages/cypress/cypress/utils/maasUtils.tspackages/maas/frontend/src/app/pages/subscriptions/ViewSubscriptionPage.tsxpackages/maas/frontend/src/app/pages/subscriptions/viewSubscription/SubscriptionDetailsSection.tsx
katieperry4
force-pushed
the
57319/subscription-view-title-bug
branch
from
684fcef to
b5ef77c
Compare
|
Verified the changes based on the content doc looks good Open.Data.Hub.2.mp4/lgtm |
| const [activeTab, setActiveTab] = React.useState<string | number>('details'); | ||
| const [subscriptionInfo, loaded, loadError] = useGetSubscriptionInfo(subscriptionName); | ||
| const displaySubscriptionName = | ||
| subscriptionInfo?.subscription.displayName?.trim() ?? subscriptionName; |
There was a problem hiding this comment.
The ?? will show an empty string from the display name instead of the subscription name
| subscriptionInfo?.subscription.displayName?.trim() ?? subscriptionName; | |
| subscriptionInfo?.subscription.displayName?.trim() || subscriptionName; |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: emilys314 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
katieperry4
force-pushed
the
57319/subscription-view-title-bug
branch
from
90e60ea to
1a60077
Compare
|
/lgtm |
openshift-merge-bot
Bot
merged commit 8bf6219
into
opendatahub-io:main
4 participants
Closes RHOAIENG-57319
Description
Just a quick change to the subscription view page
Changes the title to the display name instead of the resource name (same for the breadcrumb)
Updates some of the headers in the details section ->
Display nameto justNameandNametoResource nameScreen.Recording.2026-04-09.at.9.14.57.AM.mov
How Has This Been Tested?
Tested locally
to test:
Test Impact
Updated some mock tests to check for the display name
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
New Features
Tests