sbom upselling UI #1538
sbom upselling UI #1538
Conversation
WalkthroughThis pull request refines SBOM-related functionality across various components. The changes simplify the SBOM dashboard logic by removing unnecessary feature checks and incorporate persistent state management via local storage for support interactions. Additionally, new upselling components and associated styling are introduced to promote the SBOM feature. Routing and conditional rendering have been updated in the dashboard to reflect the SBOM enablement, and new translation entries have been added to support both English and Japanese locales. Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
npm warn config production Use ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying irenestaging with
|
| Latest commit: |
90be5d3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ef685429.irenestaging.pages.dev |
| Branch Preview URL: | https://pd-1717-sbom-upselling-ui.irenestaging.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Irene
|
||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
PD-1717-sbom-upselling-ui
|
| Run status |
|
| Run duration | 11m 20s |
| Commit |
|
| Committer | Avi Shah |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
32
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
avzz-19
force-pushed
the
PD-1717-sbom-upselling-ui
branch
from
abf7043 to
406ac98
Compare
avzz-19
force-pushed
the
PD-1717-sbom-upselling-ui
branch
from
406ac98 to
a2e8aba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/components/sbom/upselling/index.ts (4)
20-20: Externalize the endpoint.Storing the endpoint in a config file or environment variable could improve maintainability and facilitate environment-specific overrides (e.g., dev/staging/production).
- CONTACT_SUPPORT_ENDPOINT = 'v2/feature_request/sbom'; + CONTACT_SUPPORT_ENDPOINT = config.API_BASE_URL + '/feature_request/sbom';
22-28: Handle localStorage unavailability gracefully.Accessing
localStoragemight fail in private browsing or SSR contexts. Consider a defensive check or try/catch to ensure robust error handling in such cases.
30-37: Avoid hard-coding feature strings in code.Defining these feature descriptions as a higher-level constant or storing them in a separate config or translation file would increase maintainability and clarity.
39-43: Prevent multiple submission attempts.When the user clicks the contact CTA, consider disabling the button or showing a loading indicator until the task finishes. This prevents duplicate requests while the task is ongoing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/images/sbom-upselling.pngis excluded by!**/*.png
📒 Files selected for processing (18)
app/components/appknox-wrapper/index.ts(1 hunks)app/components/file/report-drawer/sbom-reports/sample/index.ts(2 hunks)app/components/sbom/upselling/index.hbs(1 hunks)app/components/sbom/upselling/index.ts(1 hunks)app/components/upselling-module/index.hbs(1 hunks)app/components/upselling-module/index.scss(1 hunks)app/components/upselling-module/index.ts(1 hunks)app/routes/authenticated/dashboard/sbom.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)app/styles/_icons.scss(1 hunks)app/styles/_theme.scss(1 hunks)app/templates/authenticated/dashboard/sbom.hbs(1 hunks)tests/integration/components/appknox-wrapper-test.js(2 hunks)tests/integration/components/file/report-drawer/sbom-reports/index-test.js(2 hunks)tests/integration/components/file/report-drawer/sbom-reports/sample-test.js(3 hunks)tests/integration/components/sbom/upselling-test.js(1 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- app/components/upselling-module/index.hbs
- app/templates/authenticated/dashboard/sbom.hbs
- app/components/upselling-module/index.ts
- app/components/sbom/upselling/index.hbs
- app/styles/_theme.scss
- app/components/appknox-wrapper/index.ts
- app/styles/_icons.scss
- app/components/file/report-drawer/sbom-reports/sample/index.ts
- translations/en.json
- translations/ja.json
- app/components/upselling-module/index.scss
- app/styles/_component-variables.scss
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run E2E Tests (5)
- GitHub Check: Run E2E Tests (4)
- GitHub Check: Run E2E Tests (3)
- GitHub Check: Run E2E Tests (2)
- GitHub Check: Run E2E Tests (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (13)
tests/integration/components/appknox-wrapper-test.js (2)
281-283: Explicit configuration setup looks good!Adding the configuration service lookup and explicitly setting
enterpriseto false makes the test setup more clear and better reflects the component's behavior, where SBOM visibility now depends on the enterprise property.
318-318: Hardcoded SBOM flag correctly matches component behavior.Changing from a dynamic value (
sectionConfig.sbom) to a hardcodedtruealigns with the updated component logic where SBOM visibility is now determined solely by the enterprise flag rather than a feature configuration.tests/integration/components/file/report-drawer/sbom-reports/index-test.js (1)
9-9: Good browser service setup for testing component interactions.The addition of
setupBrowserFakeswith window and localStorage parameters is a good practice for testing components that interact with browser APIs. Clearing localStorage before each test ensures test isolation.Also applies to: 26-26, 33-35
app/routes/authenticated/dashboard/sbom.ts (1)
10-14: Well-designed pattern change for upselling flow.This is a good architectural change from redirecting users without the SBOM feature to conditionally rendering content based on feature availability. The model now provides the
isSbomEnabledflag which can be used by the template to conditionally render either the feature or an upselling component.tests/integration/components/file/report-drawer/sbom-reports/sample-test.js (3)
22-36: Clean localStorage implementation for testing.Well-designed implementation of the localStorage interface using a Map for storing data during tests. This approach provides good isolation between tests and mimics the actual localStorage API.
72-82: Good test setup for the contact support feature.Proper setup of the server mock for the feature request endpoint and clearing localStorage before each test to ensure a clean state.
128-131: Good persistence verification.This assertion correctly verifies that the user's interest in SBOM is persisted to localStorage when they contact support.
tests/integration/components/sbom/upselling-test.js (3)
9-23: Well-structured test module setup.The test module is properly configured with the necessary test helpers and hooks. Good practice to clear localStorage before each test to ensure isolation.
24-65: Comprehensive UI rendering test.The test thoroughly verifies all UI elements of the upselling component, including the title, subtitle, features list, and call-to-action button. The assertions against translation keys ensure proper internationalization.
67-89: Complete interaction flow testing.This test effectively verifies both the user interaction (clicking the CTA button) and its side effects (localStorage persistence and UI changes). The success path is fully covered.
app/components/sbom/upselling/index.ts (3)
1-10: All import statements look good.No issues or redundancies are apparent. Everything is correctly imported and typed, including the concurrency and internationalization services.
13-16: Consider consistency and potential SSR usage.Declaring multiple services, including
window, works for client-side. If server-side rendering (SSR) is ever introduced, confirm that these services are guarded or stubbed to avoid undefined references tolocalStorage.
57-61: Registry declaration looks correct.The Glint registry entry is properly declared, enabling correct component usage in TypeScript-aware Ember setups.
avzz-19
force-pushed
the
PD-1717-sbom-upselling-ui
branch
from
a2e8aba to
39d7ad7
Compare
avzz-19
force-pushed
the
PD-1717-sbom-upselling-ui
branch
from
39d7ad7 to
d35fe5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/components/sbom/upselling/index.ts (1)
20-20: Consider externalizing the endpoint configuration.
Storing theCONTACT_SUPPORT_ENDPOINTin an environment-specific configuration or constants module helps ensure maintainability and flexibility, especially if the endpoint changes or needs to differ across environments.- CONTACT_SUPPORT_ENDPOINT = 'v2/feature_request/sbom'; + // For example, refer to an environment config or constants file + CONTACT_SUPPORT_ENDPOINT = ENV.APP.FEATURE_REQUEST_ENDPOINT || 'v2/feature_request/sbom';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/components/appknox-wrapper/index.ts(1 hunks)app/components/file/report-drawer/sbom-reports/sample/index.ts(2 hunks)app/components/sbom/upselling/index.hbs(1 hunks)app/components/sbom/upselling/index.ts(1 hunks)app/components/upselling-module/index.hbs(1 hunks)app/components/upselling-module/index.scss(1 hunks)app/components/upselling-module/index.ts(1 hunks)app/routes/authenticated/dashboard/sbom.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- app/components/appknox-wrapper/index.ts
- app/components/file/report-drawer/sbom-reports/sample/index.ts
- app/components/upselling-module/index.hbs
- app/routes/authenticated/dashboard/sbom.ts
- app/components/sbom/upselling/index.hbs
- app/components/upselling-module/index.scss
- app/components/upselling-module/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
app/components/sbom/upselling/index.ts (3)
1-6: All good here.
No issues found in the import statements.
44-54: Display a success notification to assure the user.
Currently, you only display an error notification when the request fails. Displaying a success message helps users understand that their request was processed.try { await waitForPromise(this.ajax.post(this.CONTACT_SUPPORT_ENDPOINT)); this.window.localStorage.setItem('sbomRequest', 'true'); this.hasContactedSupport = true; + this.notify.success( + this.intl.t('sbomModule.contactSupportSuccess') + ); } catch (err) { this.notify.error(parseError(err, this.intl.t('pleaseTryAgain'))); }
57-61: No issues with Glint registry declaration.
This ensures that the component is recognized by Glint’s type system.
avzz-19
force-pushed
the
PD-1717-sbom-upselling-ui
branch
from
d35fe5f to
1b2d197
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
translations/ja.json (1)
1290-1298: Ensure language consistency.
These new SBOM-related translations are presented in English under the Japanese locale file. Double-check if you intend to keep them in English or if you plan to localize them to Japanese for consistency across the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/images/sbom-upselling.pngis excluded by!**/*.png
📒 Files selected for processing (18)
app/components/appknox-wrapper/index.ts(1 hunks)app/components/file/report-drawer/sbom-reports/sample/index.ts(2 hunks)app/components/sbom/upselling/index.hbs(1 hunks)app/components/sbom/upselling/index.ts(1 hunks)app/components/upselling-module/index.hbs(1 hunks)app/components/upselling-module/index.scss(1 hunks)app/components/upselling-module/index.ts(1 hunks)app/routes/authenticated/dashboard/sbom.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)app/styles/_icons.scss(1 hunks)app/styles/_theme.scss(1 hunks)app/templates/authenticated/dashboard/sbom.hbs(1 hunks)tests/integration/components/appknox-wrapper-test.js(2 hunks)tests/integration/components/file/report-drawer/sbom-reports/index-test.js(2 hunks)tests/integration/components/file/report-drawer/sbom-reports/sample-test.js(3 hunks)tests/integration/components/sbom/upselling-test.js(1 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- app/styles/_icons.scss
- app/templates/authenticated/dashboard/sbom.hbs
- app/components/appknox-wrapper/index.ts
- tests/integration/components/file/report-drawer/sbom-reports/index-test.js
- app/components/file/report-drawer/sbom-reports/sample/index.ts
- app/styles/_theme.scss
- app/components/upselling-module/index.hbs
- app/components/upselling-module/index.scss
- app/components/sbom/upselling/index.hbs
- tests/integration/components/appknox-wrapper-test.js
- app/styles/_component-variables.scss
- app/components/upselling-module/index.ts
- app/routes/authenticated/dashboard/sbom.ts
- tests/integration/components/file/report-drawer/sbom-reports/sample-test.js
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Run Integration Tests - Part 8
- GitHub Check: Run Integration Tests - Part 7
- GitHub Check: Run Integration Tests - Part 6
- GitHub Check: Run Integration Tests - Part 5
- GitHub Check: Run Integration Tests - Part 4
- GitHub Check: Run Integration Tests - Part 3
- GitHub Check: Run Integration Tests - Part 2
- GitHub Check: Run Integration Tests - Part 1
- GitHub Check: Run Acceptance Tests
- GitHub Check: Run Unit Tests
- GitHub Check: Run Linting
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Cloudflare Pages
🔇 Additional comments (13)
translations/en.json (1)
1290-1298: Well-structured translations for SBOM upselling feature!The new translation keys for the SBOM upselling module are clear and descriptive. The feature descriptions provide good value propositions for end users.
tests/integration/components/sbom/upselling-test.js (4)
1-8: Good test setup with comprehensive importsThe imports cover all the necessary testing utilities for Ember, including internationalization support, Mirage for HTTP mocking, and browser fakes for localStorage testing.
9-23: Proper test module setup with appropriate hooksThe module setup includes all necessary hooks for rendering, internationalization, Mirage, and browser fakes. The
beforeEachhook correctly clears localStorage before each test to ensure test isolation.
24-65: Comprehensive rendering test with detailed assertionsThe first test thoroughly verifies that all elements of the upselling component render correctly, including images, title, subtitle, features, and CTA. Each assertion has a clear purpose and checks against expected translations.
67-89: Well-implemented user interaction testThis test effectively verifies the component's behavior when a user expresses interest in SBOM:
- Properly mocks the server response
- Verifies the UI change after clicking the CTA
- Checks localStorage is updated
- Confirms the success message appears
This ensures the feature's core functionality works correctly.
app/components/sbom/upselling/index.ts (8)
1-7: Imports look good.
No issues found in these import statements.
9-11: No concerns with these utility imports.
They appear correctly referenced and maintain clear code organization.
12-21: Confirm endpoint configuration.
Make sure the'v2/feature_request/sbom'path is correct and matches your backend’s expected route structure (leading slash vs. relative path, etc.).
22-29: Local storage usage is acceptable.
Initialization logic is straightforward. Just take note that localStorage might be unavailable in certain SSR or testing contexts.
30-37: Feature list generation looks correct.
Using theintlservice for translations is standard; the property is neatly returning the array for display.
39-43: CTA action is handled appropriately.
This action is concise and clean, simply invoking the task.
44-55: Consider displaying a success notification.
Currently, the code only shows errors to the user. Providing a success toast or message can help confirm that the request was received.
57-61: Glint environment declaration is properly structured.
No issues found in this registry module extension.
future-pirate-king
force-pushed
the
PD-1717-sbom-upselling-ui
branch
from
1b2d197 to
90be5d3
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
translations/en.json (1)
1290-1297: New SBOM Translation Keys Added
The new keys—introducingSBOM,sbomOverview, and the nested keys undersbomFeatures(i.e.oneClickAnalysis,identifyDependencies,uncoverVulnerabilities, andcomponentInsights)—are clear and descriptive. They effectively communicate the value of the SBOM feature to end users. Please ensure that:
- These keys are consistently referenced in the SBOM upselling UI.
- Equivalent translations for these keys are provided in other locale files (especially Japanese) to maintain localization consistency.
tests/integration/components/sbom/upselling-test.js (2)
67-89: Add error case testing for more robust coverageThe interaction test successfully verifies the happy path (successful API call), but doesn't test error scenarios. Consider adding a test case for when the server request fails.
test('it handles server errors gracefully', async function (assert) { this.server.post('/v2/feature_request/sbom', () => { return new Response(500, {}, { error: 'Server error' }); }); await render(hbs`<Sbom::Upselling />`); await click('[data-test-upselling-module-contact-cta]'); // Verify error handling behavior assert.dom('[data-test-error-message]').exists(); assert.dom('[data-test-upselling-module-contact-cta]').exists(); });
43-61: Consider simplifying feature text assertionsThe current approach to testing feature text is verbose. Consider using a loop with an array of expected translation keys for a more maintainable test.
- const featureElements = document.querySelectorAll( - '[data-test-upselling-module-feature]' - ); - - assert - .dom(featureElements[0]) - .hasText(t('sbomModule.sbomFeatures.oneClickAnalysis')); - - assert - .dom(featureElements[1]) - .hasText(t('sbomModule.sbomFeatures.identifyDependencies')); - - assert - .dom(featureElements[2]) - .hasText(t('sbomModule.sbomFeatures.uncoverVulnerabilities')); - - assert - .dom(featureElements[3]) - .hasText(t('sbomModule.sbomFeatures.componentInsights')); + const featureKeys = [ + 'sbomModule.sbomFeatures.oneClickAnalysis', + 'sbomModule.sbomFeatures.identifyDependencies', + 'sbomModule.sbomFeatures.uncoverVulnerabilities', + 'sbomModule.sbomFeatures.componentInsights' + ]; + + const featureElements = document.querySelectorAll('[data-test-upselling-module-feature]'); + featureKeys.forEach((key, index) => { + assert.dom(featureElements[index]).hasText(t(key)); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/images/sbom-upselling.pngis excluded by!**/*.png
📒 Files selected for processing (18)
app/components/appknox-wrapper/index.ts(1 hunks)app/components/file/report-drawer/sbom-reports/sample/index.ts(2 hunks)app/components/sbom/upselling/index.hbs(1 hunks)app/components/sbom/upselling/index.ts(1 hunks)app/components/upselling-module/index.hbs(1 hunks)app/components/upselling-module/index.scss(1 hunks)app/components/upselling-module/index.ts(1 hunks)app/routes/authenticated/dashboard/sbom.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)app/styles/_icons.scss(1 hunks)app/styles/_theme.scss(1 hunks)app/templates/authenticated/dashboard/sbom.hbs(1 hunks)tests/integration/components/appknox-wrapper-test.js(2 hunks)tests/integration/components/file/report-drawer/sbom-reports/index-test.js(2 hunks)tests/integration/components/file/report-drawer/sbom-reports/sample-test.js(3 hunks)tests/integration/components/sbom/upselling-test.js(1 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- app/templates/authenticated/dashboard/sbom.hbs
- app/styles/_icons.scss
- tests/integration/components/file/report-drawer/sbom-reports/index-test.js
- app/styles/_theme.scss
- app/components/upselling-module/index.hbs
- app/components/appknox-wrapper/index.ts
- app/components/upselling-module/index.ts
- app/routes/authenticated/dashboard/sbom.ts
- app/components/file/report-drawer/sbom-reports/sample/index.ts
- app/components/upselling-module/index.scss
- app/components/sbom/upselling/index.hbs
- app/styles/_component-variables.scss
- tests/integration/components/file/report-drawer/sbom-reports/sample-test.js
- app/components/sbom/upselling/index.ts
- translations/ja.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
tests/integration/components/appknox-wrapper-test.js (2)
355-357: Configuration service now explicitly defined for enterprise flag controlThis change adds explicit configuration of the enterprise flag which controls SBOM visibility. Setting
enterprisetofalsein the test configuration aligns with the simplification of SBOM dashboard logic mentioned in the PR summary.
392-392: SBOM menu item now always visible for non-enterprise usersThe SBOM menu item visibility is now hardcoded to
truerather than being dependent onsectionConfig.sbom. This change aligns with the updated architecture where SBOM visibility is now controlled solely by theenterpriseflag instead of through feature flags.tests/integration/components/sbom/upselling-test.js (4)
1-8: LGTM on the imports!All necessary imports for testing the component are properly included, with appropriate libraries for rendering, internationalization, Mirage for API mocking, and browser service testing.
9-23: Good setup of test environmentThe test module correctly sets up all required hooks for rendering, internationalization, Mirage, and browser fakes. The beforeEach hook properly clears localStorage to prevent test data leakage.
24-65: Well-structured rendering test with comprehensive assertionsThe test thoroughly verifies all aspects of the component's rendering:
- Presence of the upselling module
- Correct translations for titles and subtitles
- All four features with proper text content
- Initial state of UI elements (CTA exists, success message doesn't)
The use of data-test attributes for DOM selection is a good practice.
76-89: Good job on verifying localStorage interactionThe test effectively checks that user interaction updates localStorage and changes the UI accordingly. This is important for persistence of user preferences across page loads.
No description provided.